"Redirecting URLs to Packaged Apps" implementation: revised

"Redirecting URLs to Packaged Apps" implementation: revised

Design proposal: https://docs.google.com/document/d/1r-RoOv2URfZBYrT_B6notQ6MeMqZRd1EP1AITuzJCAc/edit?usp=sharing

* Support for url_handlers in manifest
* New kind of onLaunched event with navigation info in launch data
* Intercept/redirect top-level browser-initiated navigations (bookmarks, omnibox, etc.)
* Intercept/redirect top-frame navigations in tabs and app windows. 

This is a rework of https://codereview.chromium.org/22944002/.

BUG=111422
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222235

[benwells, 2013-09-03 00:39:14]

You need to publish your CL (like this) for reviewers to see it.

[benwells, 2013-09-03 01:10:56]

https://codereview.chromium.org/23847004/diff/5001/apps/app_window_contents.cc
File apps/app_window_contents.cc (right):

https://codereview.chromium.org/23847004/diff/5001/apps/app_window_contents.c...
apps/app_window_contents.cc:172: UrlRedirector::MaybeLaunchAppWithUrl(
Can you just create a UrlRedirector tab helper thing like in
browser_tab_contents? Have a look at c/b/ui/apps/chrome_shell_window_delegate
InitWebContents.

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.cc
File apps/url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.cc#new...
apps/url_redirector.cc:37: bool UrlRedirector::MaybeLaunchAppWithUrl(
This is called from browser_navigator, and maybe other places. it is static so
the CHECK above isn't guaranteed to catch all uses.

What checks are in place to make sure it isn't called with an incognito profile?
Maybe it should just return false if it is?

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.cc#new...
apps/url_redirector.cc:80: MaybeLaunchAppWithUrl(profile_, url, referrer_url);
Just going by the names, I'm assuming if LaunchAppWithUrl is called, it is not a
Maybe. Can you dcheck that? i.e.
bool launched = Maybe....
DCHECK(launched)

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.h
File apps/url_redirector.h (right):

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.h#newc...
apps/url_redirector.h:30: // event with |url| and |referrer_url| available
through the launch data.
Tiny nit: document return value.

https://codereview.chromium.org/23847004/diff/5001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://codereview.chromium.org/23847004/diff/5001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_tab_contents.cc:121:
apps::UrlRedirector::CreateForWebContents(web_contents);
Won't this get called for incognito profiles and hit that CHECK?

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.h (right):

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/url_handlers/url_handlers_parser.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
Nit: copyright above. Please check all the new files and make the first line:
// Copyright 2013 The Chromium Authors. All rights reserved.

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/c...
File chrome/common/extensions/chrome_manifest_handlers.cc (right):

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/c...
chrome/common/extensions/chrome_manifest_handlers.cc:67: (new
UrlHandlersParser)->Register();
Nit: keep this list aphabetically sorted.
Also nitty: is Url the typical case? Below there is URLOverridesHandler.

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/e...
File chrome/common/extensions/extension_manifest_constants.h (right):

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/e...
chrome/common/extensions/extension_manifest_constants.h:235: extern const char
kInvalidUrlHandlers[];
previous nit continued: Note kInvalidURLPatternError and kLaunchURLRequried
versus kInvalidUrlHandler...

URL not Url.

[benwells, 2013-09-03 01:13:30]

Also, have you tested if links sent to chrome on the command line get redirected
properly? This should work for both cases:
- chrome is not running
- chrome is already running.

I'm assuming this is meant to work.

I'm also unclear on what the incognito behavior is meant to be: I assume links
in incognito windows should be redirected to any platform apps in the original
(non-incognito) profile. Is that right / what is implemented?

Tests would make the expected behavior clear (i.e. you'd have tests explicitly
checking what should happen in these cases).

[sergeygs, 2013-09-03 01:15:21]

On 2013/09/03 00:39:14, benwells wrote:
> You need to publish your CL (like this) for reviewers to see it.

It's a holiday over here, so I didn't feel a pressing need. Nevertheless, will
do.

[darin (slow to review), 2013-09-03 04:13:10]

Will there be some tests added to this CL?

[sergeygs1, 2013-09-03 04:48:59]

Definitely, after all the new classes and methods have been LGTMed.


On Mon, Sep 2, 2013 at 9:13 PM, <darin@chromium.org> wrote:

> Will there be some tests added to this CL?
>
>
https://codereview.chromium.**org/23847004/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sergeygs, 2013-09-03 04:57:30]

On 2013/09/03 04:48:59, sergeygs1 wrote:
> Definitely, after all the new classes and methods have been LGTMed.
> 
> 
> On Mon, Sep 2, 2013 at 9:13 PM, <mailto:darin@chromium.org> wrote:
> 
> > Will there be some tests added to this CL?
> >
> >
>
https://codereview.chromium.**org/23847004/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sorry, shouldn't have used that acronym in a comment.

Also, Darin: since you've added yourself, should I consider chrome/browser/ui
covered, or should I still add e.g. sky@?

[sergeygs, 2013-09-03 06:15:24]

https://codereview.chromium.org/23847004/diff/5001/apps/app_window_contents.cc
File apps/app_window_contents.cc (right):

https://codereview.chromium.org/23847004/diff/5001/apps/app_window_contents.c...
apps/app_window_contents.cc:172: UrlRedirector::MaybeLaunchAppWithUrl(
On 2013/09/03 01:10:56, benwells wrote:
> Can you just create a UrlRedirector tab helper thing like in
> browser_tab_contents? Have a look at c/b/ui/apps/chrome_shell_window_delegate
> InitWebContents.

Great advice, thanks, I just didn't find that on my own. Moved the IPC
dispatching around as suggested, and it works!

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.cc
File apps/url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.cc#new...
apps/url_redirector.cc:37: bool UrlRedirector::MaybeLaunchAppWithUrl(
On 2013/09/03 01:10:56, benwells wrote:
> This is called from browser_navigator, and maybe other places. it is static so
> the CHECK above isn't guaranteed to catch all uses.
> 
> What checks are in place to make sure it isn't called with an incognito
profile?
> Maybe it should just return false if it is?

Done, return false in incognito now. This is just an extra precaution, since
there can be no apps in incognito currently, hence nothing to send the URL to in
any case.

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.cc#new...
apps/url_redirector.cc:80: MaybeLaunchAppWithUrl(profile_, url, referrer_url);
On 2013/09/03 01:10:56, benwells wrote:
> Just going by the names, I'm assuming if LaunchAppWithUrl is called, it is not
a
> Maybe. Can you dcheck that? i.e.
> bool launched = Maybe....
> DCHECK(launched)

Done. Indeed, the sender should send the IPC only after making sure an app to
handle the URL exists. I was just a little unsure that the app could not
possibly disappear between sending and receiving the IPC, so left a little
backdoor here. Do you think that's not possible (app disappearing)?

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.h
File apps/url_redirector.h (right):

https://codereview.chromium.org/23847004/diff/5001/apps/url_redirector.h#newc...
apps/url_redirector.h:30: // event with |url| and |referrer_url| available
through the launch data.
On 2013/09/03 01:10:56, benwells wrote:
> Tiny nit: document return value.

Done.

https://codereview.chromium.org/23847004/diff/5001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://codereview.chromium.org/23847004/diff/5001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_tab_contents.cc:121:
apps::UrlRedirector::CreateForWebContents(web_contents);
On 2013/09/03 01:10:56, benwells wrote:
> Won't this get called for incognito profiles and hit that CHECK?

Nice catch here, added an 'if'.

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.h (right):

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/url_handlers/url_handlers_parser.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/09/03 01:10:56, benwells wrote:
> Nit: copyright above. Please check all the new files and make the first line:
> // Copyright 2013 The Chromium Authors. All rights reserved.

Done. Should I also remove (c) as you suggested for sources under apps/? Or
should its use be consistent per module?

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/c...
File chrome/common/extensions/chrome_manifest_handlers.cc (right):

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/c...
chrome/common/extensions/chrome_manifest_handlers.cc:67: (new
UrlHandlersParser)->Register();
On 2013/09/03 01:10:56, benwells wrote:
> Nit: keep this list aphabetically sorted.
> Also nitty: is Url the typical case? Below there is URLOverridesHandler.

Ah, an automated renaming artifact. Done. Regarding Url vs URL: I've talked to
the team, and the prevalent and now recommended usage is Url. This class comes
from a different module, which already has Url's (ShellWindow::SetAppIconUrl),
but no URL's (except for GURL).

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/e...
File chrome/common/extensions/extension_manifest_constants.h (right):

https://codereview.chromium.org/23847004/diff/5001/chrome/common/extensions/e...
chrome/common/extensions/extension_manifest_constants.h:235: extern const char
kInvalidUrlHandlers[];
On 2013/09/03 01:10:56, benwells wrote:
> previous nit continued: Note kInvalidURLPatternError and kLaunchURLRequried
> versus kInvalidUrlHandler...
> 
> URL not Url.

Agreed, here it probably makes sense, since all these originate from here, and
there are too many URLs around. A later URL->Url renaming will get everything in
one swoop.

[sergeygs, 2013-09-03 06:20:30]

On 2013/09/03 01:13:30, benwells wrote:
> Also, have you tested if links sent to chrome on the command line get
redirected
> properly? This should work for both cases:
> - chrome is not running
> - chrome is already running.
> 
> I'm assuming this is meant to work.
> 
> I'm also unclear on what the incognito behavior is meant to be: I assume links
> in incognito windows should be redirected to any platform apps in the original
> (non-incognito) profile. Is that right / what is implemented?
> 
> Tests would make the expected behavior clear (i.e. you'd have tests explicitly
> checking what should happen in these cases).

Command line: yes, I've tested that, and both cases work.

Incognito: I think links in incognito windows absolutely should not be
redirected to any apps at all, let alone in the original non-incognito profile.
That would create a leak of "incognito-ness", since the app will receive and
possibly siphon information about what URLs the user visits. I believe this is
one of the reasons it was decided not to support apps and extensions in
incognito in the first place.

[benwells, 2013-09-03 12:07:21]

On 2013/09/03 06:20:30, sergeygs wrote:
> On 2013/09/03 01:13:30, benwells wrote:
> > Also, have you tested if links sent to chrome on the command line get
> redirected
> > properly? This should work for both cases:
> > - chrome is not running
> > - chrome is already running.
> > 
> > I'm assuming this is meant to work.
> > 
> > I'm also unclear on what the incognito behavior is meant to be: I assume
links
> > in incognito windows should be redirected to any platform apps in the
original
> > (non-incognito) profile. Is that right / what is implemented?
> > 
> > Tests would make the expected behavior clear (i.e. you'd have tests
explicitly
> > checking what should happen in these cases).
> 
> Command line: yes, I've tested that, and both cases work.
> 
> Incognito: I think links in incognito windows absolutely should not be
> redirected to any apps at all, let alone in the original non-incognito
profile.
> That would create a leak of "incognito-ness", since the app will receive and
> possibly siphon information about what URLs the user visits. I believe this is
> one of the reasons it was decided not to support apps and extensions in
> incognito in the first place.

Thanks for explaining the incognito behaviour. External handlers (e.g. steam for
steam:// links) still get executed from incognito mode, as do
registerProtocolHandler handlers, so I don't know if it is clear cut. Your
suggestion is a good safe place to start.

Extensions are supported in incognito, under the user's control. Apps are not
supported because we didn't really know what it would mean :) Would apps not be
allowed to store anything to local state? I guess thinking about it more they
could be supported but without any access to APIs like identity, but we wouldn't
have any way via UI to make incognito apparent as the UI of an app is totally
under its control.

Anyhow ...

I didn't get around to reviewing this in more detail today, but I think you
should add some basic browser tests in this CL (i.e. as a requirement for lgtm,
not after). Unit tests can wait.

I know you're under time pressure, but adding a browser test or two should not
take too much time. You can use the tests in
chrome/browser/apps/app_browsertest.cc for inspiration.

Lastly, when are you planning to remove the experimental flag requirement? If
you want this out for M31 you'll need that to come off by the end of the week
(i.e. be enabled on dev by default).

[benwells, 2013-09-03 13:05:26]

On 2013/09/03 12:07:21, benwells wrote:
> On 2013/09/03 06:20:30, sergeygs wrote:
> > On 2013/09/03 01:13:30, benwells wrote:
> > > Also, have you tested if links sent to chrome on the command line get
> > redirected
> > > properly? This should work for both cases:
> > > - chrome is not running
> > > - chrome is already running.
> > > 
> > > I'm assuming this is meant to work.
> > > 
> > > I'm also unclear on what the incognito behavior is meant to be: I assume
> links
> > > in incognito windows should be redirected to any platform apps in the
> original
> > > (non-incognito) profile. Is that right / what is implemented?
> > > 
> > > Tests would make the expected behavior clear (i.e. you'd have tests
> explicitly
> > > checking what should happen in these cases).
> > 
> > Command line: yes, I've tested that, and both cases work.
> > 
> > Incognito: I think links in incognito windows absolutely should not be
> > redirected to any apps at all, let alone in the original non-incognito
> profile.
> > That would create a leak of "incognito-ness", since the app will receive and
> > possibly siphon information about what URLs the user visits. I believe this
is
> > one of the reasons it was decided not to support apps and extensions in
> > incognito in the first place.
> 
> Thanks for explaining the incognito behaviour. External handlers (e.g. steam
for
> steam:// links) still get executed from incognito mode, as do
> registerProtocolHandler handlers, so I don't know if it is clear cut. Your
> suggestion is a good safe place to start.
> 
> Extensions are supported in incognito, under the user's control. Apps are not
> supported because we didn't really know what it would mean :) Would apps not
be
> allowed to store anything to local state? I guess thinking about it more they
> could be supported but without any access to APIs like identity, but we
wouldn't
> have any way via UI to make incognito apparent as the UI of an app is totally
> under its control.
> 
> Anyhow ...
> 
> I didn't get around to reviewing this in more detail today, but I think you
> should add some basic browser tests in this CL (i.e. as a requirement for
lgtm,
> not after). Unit tests can wait.
> 
> I know you're under time pressure, but adding a browser test or two should not
> take too much time. You can use the tests in
> chrome/browser/apps/app_browsertest.cc for inspiration.
> 
> Lastly, when are you planning to remove the experimental flag requirement? If
> you want this out for M31 you'll need that to come off by the end of the week
> (i.e. be enabled on dev by default).

Oh, darn it, now I've used the magical acronym ... please don't take that as an
lg tm.

[jam, 2013-09-03 15:25:16]

https://codereview.chromium.org/23847004/diff/24001/apps/url_redirector.cc
File apps/url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/24001/apps/url_redirector.cc#ne...
apps/url_redirector.cc:15: #include "chrome/common/extensions/extension_set.h"
I'll defer to the apps folks, but it's not obvious to me what the benefit of
putting this file in src/apps is. It handles an IPC sent from src/chrome, that's
defined in src/chrome, using code in src/chrome.

[not at google - send to devlin, 2013-09-03 15:30:43]

Please at least unit test the classes you've added that are easily
unit-testable. This diff is over a thousand lines and relying on existing test
coverage is just not good enough (and I did ask you a while ago to add tests to
this CL).

Adding tests "after the CL has been lgtm'd" is not ideal because the structure
of the code may change, then reviewers will have to look again, which may result
in more rounds of reviews. Better to add them now.

[not at google - send to devlin, 2013-09-03 15:30:52]

bah not lgtm.

[not at google - send to devlin, 2013-09-03 15:31:24]

and I believe that benwells did not lgtm this either...

[sergeygs, 2013-09-03 16:12:11]

On 2013/09/03 15:30:43, kalman wrote:
> Please at least unit test the classes you've added that are easily
> unit-testable. This diff is over a thousand lines and relying on existing test
> coverage is just not good enough (and I did ask you a while ago to add tests
to
> this CL).
> 
> Adding tests "after the CL has been l g t m'd" is not ideal because the
structure
> of the code may change, then reviewers will have to look again, which may
result
> in more rounds of reviews. Better to add them now.

Ok, so do classes I've added look good enough and live in proper locations now?

[sergeygs, 2013-09-03 16:43:04]

https://codereview.chromium.org/23847004/diff/24001/apps/url_redirector.cc
File apps/url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/24001/apps/url_redirector.cc#ne...
apps/url_redirector.cc:15: #include "chrome/common/extensions/extension_set.h"
On 2013/09/03 15:25:16, jam wrote:
> I'll defer to the apps folks, but it's not obvious to me what the benefit of
> putting this file in src/apps is. It handles an IPC sent from src/chrome,
that's
> defined in src/chrome, using code in src/chrome.

This class launches v2 apps. chrome/, and even extension system parts that
extend beyond chrome/, are not supposed to know about apps any more. Apps live
in apss/ now. Ben (Wells): is my understanding correct?

[not at google - send to devlin, 2013-09-03 17:39:51]

https://codereview.chromium.org/23847004/diff/24001/apps/url_redirector.cc
File apps/url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/24001/apps/url_redirector.cc#ne...
apps/url_redirector.cc:15: #include "chrome/common/extensions/extension_set.h"
On 2013/09/03 16:43:04, sergeygs wrote:
> On 2013/09/03 15:25:16, jam wrote:
> > I'll defer to the apps folks, but it's not obvious to me what the benefit of
> > putting this file in src/apps is. It handles an IPC sent from src/chrome,
> that's
> > defined in src/chrome, using code in src/chrome.
> 
> This class launches v2 apps. chrome/, and even extension system parts that
> extend beyond chrome/, are not supposed to know about apps any more. Apps live
> in apss/ now. Ben (Wells): is my understanding correct?

Honestly I thought it was the other way around, that apps/ is a module included
by Chrome (similar to how content/ and extensions/ work) and as such apps/
should know nothing about chrome/ - but this split actually appears to be purely
a conceptual one.

I think jam has a good point. Routed IPCs have a well-defined endpoint - that
is, the sender of the IPC should know exactly where that IPC implementation is
ending up. It's an odd situation having an IPC sent from chrome/ being handled
from apps/.

[not at google - send to devlin, 2013-09-03 18:28:04]

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:944:
extensions::switches::kEnableExperimentalExtensionApis))
this seems unnecessary. Firstly you're gating this on being in dev channel not
experimental APIs. secondly try to only have a single place where features are
turned on/off. That place is at manifest parsing time.

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:23:
dispatch([]);
for consistency with the other comments should you enumerate the cases here?
Namely "Launched from the app launcher or...".

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:29: // NOTE:
Using the new-style dispatch.
this NOTE doesn't add anything.

[jschuh, 2013-09-04 00:25:43]

lgtm on ipc and parsing. you said @creis already signed off on navigation logic.

[benwells, 2013-09-04 00:26:34]

On 2013/09/03 18:28:04, kalman wrote:
>
https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/chrome_co...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/chrome_co...
> chrome/renderer/chrome_content_renderer_client.cc:944:
> extensions::switches::kEnableExperimentalExtensionApis))
> this seems unnecessary. Firstly you're gating this on being in dev channel not
> experimental APIs. secondly try to only have a single place where features are
> turned on/off. That place is at manifest parsing time.
> 
>
https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
> File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
> (right):
> 
>
https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
> chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:23:
> dispatch([]);
> for consistency with the other comments should you enumerate the cases here?
> Namely "Launched from the app launcher or...".
> 
>
https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
> chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:29: //
NOTE:
> Using the new-style dispatch.
> this NOTE doesn't add anything.

not lgtm to cancel my previous accidental lg tm.

About where this lives: jam@ and kalman@ are making sense. I need to think about
it a bit; I think URL redirection makes sense in a non-chrome apps platform so
it belongs in the apps component somehow. Just not sure how yet.

It would be nice in the meantime to have the redirector in
src/chrome/browser/apps and the sender in src/chrome/renderer/apps.

Failing that I would settle for leaving it all where it was in
src/chrome/browser/extensions/ and src/chrome/renderer/extensions.

[sergeygs, 2013-09-04 03:29:16]

https://codereview.chromium.org/23847004/diff/24001/apps/url_redirector.cc
File apps/url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/24001/apps/url_redirector.cc#ne...
apps/url_redirector.cc:15: #include "chrome/common/extensions/extension_set.h"
On 2013/09/03 17:39:52, kalman wrote:
> On 2013/09/03 16:43:04, sergeygs wrote:
> > On 2013/09/03 15:25:16, jam wrote:
> > > I'll defer to the apps folks, but it's not obvious to me what the benefit
of
> > > putting this file in src/apps is. It handles an IPC sent from src/chrome,
> > that's
> > > defined in src/chrome, using code in src/chrome.
> > 
> > This class launches v2 apps. chrome/, and even extension system parts that
> > extend beyond chrome/, are not supposed to know about apps any more. Apps
live
> > in apss/ now. Ben (Wells): is my understanding correct?
> 
> Honestly I thought it was the other way around, that apps/ is a module
included
> by Chrome (similar to how content/ and extensions/ work) and as such apps/
> should know nothing about chrome/ - but this split actually appears to be
purely
> a conceptual one.
> 
> I think jam has a good point. Routed IPCs have a well-defined endpoint - that
> is, the sender of the IPC should know exactly where that IPC implementation is
> ending up. It's an odd situation having an IPC sent from chrome/ being handled
> from apps/.

As discussed offline, you are right. I'm going to move this class into
c/b/apps/app_url_redirector.h (benwells@ agreed).

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:944:
extensions::switches::kEnableExperimentalExtensionApis))
On 2013/09/03 18:28:04, kalman wrote:
> this seems unnecessary. Firstly you're gating this on being in dev channel not
> experimental APIs. secondly try to only have a single place where features are
> turned on/off. That place is at manifest parsing time.

This switch was only temporary (for debugging). Removed in the next patch.

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:23:
dispatch([]);
On 2013/09/03 18:28:04, kalman wrote:
> for consistency with the other comments should you enumerate the cases here?
> Namely "Launched from the app launcher or...".

Done.

https://codereview.chromium.org/23847004/diff/24001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:29: // NOTE:
Using the new-style dispatch.
On 2013/09/03 18:28:04, kalman wrote:
> this NOTE doesn't add anything.

I wasn't the one who added it initially, so I didn't want to remove it. I don't
really know what it means. benwells@?

[not at google - send to devlin, 2013-09-04 15:37:20]

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:32: // static
See comment at caller in the other file (can't remember the name of it).

Make this non-static and if you wish add a static Get method to this class. Then
remove the comment and the IsOffTheRecord guard below.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:44: if (!service)
I wish this check wasn't necessary and that we can arrange for the
AppUrlRedirector to be created after we know there's an ExtensionService. Eh.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:75: // to check that an appropriate
app to handle |url| indeed exists, so
s/indeed //

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
File chrome/browser/ui/apps/chrome_shell_window_delegate.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
chrome/browser/ui/apps/chrome_shell_window_delegate.cc:72: if
(!profile->IsOffTheRecord())
why do you need to intercept navigations from shell windows?

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:44: #include
"extensions/common/switches.h"
unused?

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:492: params->initiating_profile,
Just grab the AppUrlRedirector from the initiating profile (using that
profile-keyed-service stuff) and call MaybeLaunchAppWithUrl on it. Making
MaybeLaunchAppWithUrl static is awkward and leads to unnecessary checks and
comments such as the one in that file.

However, you could add a static Get method to AppUrlRedirector if you wish
(similar to how ExtensionSystem has a Get method).

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1593: if (IsWithinPlatformApp()) {
I didn't think you could navigate within platform app processes like this. It's
probably a bug if you can?

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1628: if (IsWithinPlatformApp()) {
likewise

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1633:
content::RenderView::FromWebView(frame->view());
does this mean that you redirect the opener frame rather than the opened one?

[benwells, 2013-09-04 23:54:20]

For the stuff I'm looking at, this is getting very close. However you still need
tests.

Note for other reviewers: I'm not looking at the manifest parsing or the
dispatching.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.h (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.h:11: class ExtensionSet;
Nit: forward decl. not used any more.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.h:43: // The normal profile associated
with this ExtensionService.
Nit: s/ExtensionService/AppUrlRedirectory/

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
File chrome/browser/ui/apps/chrome_shell_window_delegate.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
chrome/browser/ui/apps/chrome_shell_window_delegate.cc:72: if
(!profile->IsOffTheRecord())
On 2013/09/04 15:37:21, kalman wrote:
> why do you need to intercept navigations from shell windows?

Platform apps can navigate which would normally open the link in the users
default browser, if there is an app handler it should be used instead.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:483: extensions::ExtensionSystem* system
=
It's unfortunate this code has to be here. This function is already complicated
and now it has another special case. Is there no other, more systematic way to
do this?

If this is here why do we need the other code to redirect to platform apps?

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:492: params->initiating_profile,
On 2013/09/04 15:37:21, kalman wrote:
> Just grab the AppUrlRedirector from the initiating profile (using that
> profile-keyed-service stuff) and call MaybeLaunchAppWithUrl on it. Making
> MaybeLaunchAppWithUrl static is awkward and leads to unnecessary checks and
> comments such as the one in that file.
> 
> However, you could add a static Get method to AppUrlRedirector if you wish
> (similar to how ExtensionSystem has a Get method).

At the moment the redirector is a tab helper (or whatever they are called now),
not a PKS.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_tab_contents.cc:117: // for that. See b/240879, which
tracks incognito support for v2 apps.
Nit: crbug.com/ instead of b/. b/ is a different, google internal thing.

https://codereview.chromium.org/23847004/diff/42001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.h (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
Tiny nit: no (c).

The style changed from 2012 to 2013 for some reason.

https://codereview.chromium.org/23847004/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.h:19: struct
UrlHandlerInfo {
Nit: These should be URLHandlerInfo...

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1573: bool
Dispatcher::MaybeRedirectUrlToApp(
It doesn't feel good to have this code here. Did you log a bug / add a TODO to
move it to chrome/browser/apps? Feels like we need an apps_dispatcher in chrome.

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:31: // NOTE:
Using the new-style dispatch.
To answer the earlier comment: I have no idea what this note is communicating.
Maybe I added it and forgot, or maybe someone else added it. It is probably not
relevant (maybe new-style dispatch is now the current-style dispatch?)

https://codereview.chromium.org/23847004/diff/42001/extensions/common/manifes...
File extensions/common/manifest_constants.h (right):

https://codereview.chromium.org/23847004/diff/42001/extensions/common/manifes...
extensions/common/manifest_constants.h:151: extern const char kUrlHandlers[];
Nit: Url -> URL

[not at google - send to devlin, 2013-09-05 00:01:52]

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
File chrome/browser/ui/apps/chrome_shell_window_delegate.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
chrome/browser/ui/apps/chrome_shell_window_delegate.cc:72: if
(!profile->IsOffTheRecord())
On 2013/09/04 23:54:20, benwells wrote:
> On 2013/09/04 15:37:21, kalman wrote:
> > why do you need to intercept navigations from shell windows?
> 
> Platform apps can navigate which would normally open the link in the users
> default browser, if there is an app handler it should be used instead.

It seems like there are a whole bunch of other places where you'd need to
intercept navigations from, then. I was assuming that the Navigate method would
catch them all.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:492: params->initiating_profile,
On 2013/09/04 23:54:20, benwells wrote:
> On 2013/09/04 15:37:21, kalman wrote:
> > Just grab the AppUrlRedirector from the initiating profile (using that
> > profile-keyed-service stuff) and call MaybeLaunchAppWithUrl on it. Making
> > MaybeLaunchAppWithUrl static is awkward and leads to unnecessary checks and
> > comments such as the one in that file.
> > 
> > However, you could add a static Get method to AppUrlRedirector if you wish
> > (similar to how ExtensionSystem has a Get method).
> 
> At the moment the redirector is a tab helper (or whatever they are called
now),
> not a PKS.

Yes, realised this later after chatting to Sergey. I suggested that these
actually be profile-scoped and that it should be responsible for listening to
WebContents creation and destruction (via WebContents::AddCreatedCallback). That
keeps the fact that these are Tab-scoped as an implementation detail, which is
what they should be.

[sergeygs, 2013-09-05 09:19:03]

After talking with Darin (thanks!), I've reworked the whole interception
mechanism to use components/ResourceThrottle concept instead intercepting
navigations in several places and sending IPCs. See patch #7 (I hit enter too
soon, so it's untitled). I hope that finally addresses all of the (serious)
previous concerns now. I'm going to start adding tests.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:32: // static
On 2013/09/04 15:37:21, kalman wrote:
> See comment at caller in the other file (can't remember the name of it).
> 
> Make this non-static and if you wish add a static Get method to this class.
Then
> remove the comment and the IsOffTheRecord guard below.

Obsolete.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:44: if (!service)
On 2013/09/04 15:37:21, kalman wrote:
> I wish this check wasn't necessary and that we can arrange for the
> AppUrlRedirector to be created after we know there's an ExtensionService. Eh.

Obsolete.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:75: // to check that an appropriate
app to handle |url| indeed exists, so
On 2013/09/04 15:37:21, kalman wrote:
> s/indeed //

Obsolete.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.h (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.h:11: class ExtensionSet;
On 2013/09/04 23:54:20, benwells wrote:
> Nit: forward decl. not used any more.

Obsolete.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.h:43: // The normal profile associated
with this ExtensionService.
On 2013/09/04 23:54:20, benwells wrote:
> Nit: s/ExtensionService/AppUrlRedirectory/

Obsolete.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
File chrome/browser/ui/apps/chrome_shell_window_delegate.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
chrome/browser/ui/apps/chrome_shell_window_delegate.cc:72: if
(!profile->IsOffTheRecord())
On 2013/09/04 15:37:21, kalman wrote:
> why do you need to intercept navigations from shell windows?

To intercept target=_blank and window.open() navigations in apps. I believe this
was already reviewed and indirectly (by requesting me to move this code from one
class to another) blessed by benwells@.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/apps/ch...
chrome/browser/ui/apps/chrome_shell_window_delegate.cc:72: if
(!profile->IsOffTheRecord())
On 2013/09/05 00:01:52, kalman wrote:
> On 2013/09/04 23:54:20, benwells wrote:
> > On 2013/09/04 15:37:21, kalman wrote:
> > > why do you need to intercept navigations from shell windows?
> > 
> > Platform apps can navigate which would normally open the link in the users
> > default browser, if there is an app handler it should be used instead.
> 
> It seems like there are a whole bunch of other places where you'd need to
> intercept navigations from, then. I was assuming that the Navigate method
would
> catch them all.

Not exactly a whole bunch, according to I found, and I think I was covering them
all, but anyway: this is now obsolete (see the latest patch).

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:44: #include
"extensions/common/switches.h"
On 2013/09/04 15:37:21, kalman wrote:
> unused?

Done.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:483: extensions::ExtensionSystem* system
=
On 2013/09/04 23:54:20, benwells wrote:
> It's unfortunate this code has to be here. This function is already
complicated
> and now it has another special case. Is there no other, more systematic way to
> do this?
> 
> If this is here why do we need the other code to redirect to platform apps?

To answer the last question: because this deals only with top-level,
browser-initiated navigations. If you click a link which doesn't have
target='_blank' in a tab, WebKit does most of the job. And if you don't stop it
early enough, it will navigate.

But long story short: this is now obsoleted by the latest patch.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:492: params->initiating_profile,
On 2013/09/05 00:01:52, kalman wrote:
> On 2013/09/04 23:54:20, benwells wrote:
> > On 2013/09/04 15:37:21, kalman wrote:
> > > Just grab the AppUrlRedirector from the initiating profile (using that
> > > profile-keyed-service stuff) and call MaybeLaunchAppWithUrl on it. Making
> > > MaybeLaunchAppWithUrl static is awkward and leads to unnecessary checks
and
> > > comments such as the one in that file.
> > > 
> > > However, you could add a static Get method to AppUrlRedirector if you wish
> > > (similar to how ExtensionSystem has a Get method).
> > 
> > At the moment the redirector is a tab helper (or whatever they are called
> now),
> > not a PKS.
> 
> Yes, realised this later after chatting to Sergey. I suggested that these
> actually be profile-scoped and that it should be responsible for listening to
> WebContents creation and destruction (via WebContents::AddCreatedCallback).
That
> keeps the fact that these are Tab-scoped as an implementation detail, which is
> what they should be.

Obsolete.

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_tab_contents.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/browser/ui/browser...
chrome/browser/ui/browser_tab_contents.cc:117: // for that. See b/240879, which
tracks incognito support for v2 apps.
On 2013/09/04 23:54:20, benwells wrote:
> Nit: crbug.com/ instead of b/. b/ is a different, google internal thing.

Done.

https://codereview.chromium.org/23847004/diff/42001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.h (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2013/09/04 23:54:20, benwells wrote:
> Tiny nit: no (c).
> 
> The style changed from 2012 to 2013 for some reason.

Shouldn't we do one mass replacement and be done with it then? Otherwise it's
inconsistent: all neighboring files have (c). But anyway: done.

https://codereview.chromium.org/23847004/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.h:19: struct
UrlHandlerInfo {
On 2013/09/04 23:54:20, benwells wrote:
> Nit: These should be URLHandlerInfo...

This is a recurring theme. I've talked to the people around, and the current
recommendation is Url. Again, a mass replacement would be nice. Until then,
there will unavoidably be classes that will appear in both old (URL) and new
(Url) worlds.

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1573: bool
Dispatcher::MaybeRedirectUrlToApp(
On 2013/09/04 23:54:20, benwells wrote:
> It doesn't feel good to have this code here. Did you log a bug / add a TODO to
> move it to chrome/browser/apps? Feels like we need an apps_dispatcher in
chrome.

Obsolete.

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1593: if (IsWithinPlatformApp()) {
On 2013/09/04 15:37:21, kalman wrote:
> I didn't think you could navigate within platform app processes like this.
It's
> probably a bug if you can?

To answer the question: this is ultimately called from WebKit, so those
navigation filters haven't been applied yet when we reach here. But yes, you are
not allowed to navigate via regular link clicks within apps (as the comment
below also mentions), and that case is explicitly handled here by omitting the
WebNavigationTypeLinkClicked case (compare to the 'else' for non-apps). So if a
link is clicked within an app, we return false, and that diagnostic gets printed
as usual. However, you are allowed to click links with target=_blank within
apps,  and those go through here too.

But anyway, this is now obsoleted by the latest patch.

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1628: if (IsWithinPlatformApp()) {
On 2013/09/04 15:37:21, kalman wrote:
> likewise

See the answer above.

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1633:
content::RenderView::FromWebView(frame->view());
On 2013/09/04 15:37:21, kalman wrote:
> does this mean that you redirect the opener frame rather than the opened one?

I used the opener frame's routing ID to reach the correct AppUrlRedirector. But
again: obsolete.

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/23847004/diff/42001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:31: // NOTE:
Using the new-style dispatch.
On 2013/09/04 23:54:20, benwells wrote:
> To answer the earlier comment: I have no idea what this note is communicating.
> Maybe I added it and forgot, or maybe someone else added it. It is probably
not
> relevant (maybe new-style dispatch is now the current-style dispatch?)

Done.

https://codereview.chromium.org/23847004/diff/42001/extensions/common/manifes...
File extensions/common/manifest_constants.h (right):

https://codereview.chromium.org/23847004/diff/42001/extensions/common/manifes...
extensions/common/manifest_constants.h:151: extern const char kUrlHandlers[];
On 2013/09/04 23:54:20, benwells wrote:
> Nit: Url -> URL

Already replied in a different file.

[not at google - send to devlin, 2013-09-05 15:14:21]

That looks much nicer, thanks. Looking forward to the tests.

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:34: DCHECK(source);
DCHECK(source) isn't useful

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:53: if (!profile ||
profile->IsOffTheRecord())
|profile| can't be null, no need to guard.

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:79: } // namespace
2 spaces

[darin (slow to review), 2013-09-05 22:52:14]

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:41: if (params.is_post())
You could probably filter this earlier too, to avoid bouncing over the UI
thread.

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:269:
throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
You are installing this throttle for each and every URL request, whether for
top-level frames, subframes or subresources.  Since you are adding a delay to
route through the UI thread, this can add up to a lot of wasted cycles.

I think you should probably only register your throttle for MAIN_FRAME resource
loads, and presumably not for pre-rendering requests. You probably need to make
sure these are not pre-rendered in some other way, right?

I think you should just duplicate the check that is done below in the OS_ANDROID
case.

[sergeygs, 2013-09-05 23:46:57]

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:34: DCHECK(source);
On 2013/09/05 15:14:22, kalman wrote:
> DCHECK(source) isn't useful

Done. (but it's like that at least in some other Throttles).

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:41: if (params.is_post())
On 2013/09/05 22:52:15, darin wrote:
> You could probably filter this earlier too, to avoid bouncing over the UI
> thread.

Yes, you're right. Done in ChromeResourceDispatcherHostDelegate now too. I'd
leave these checks here, though, for extra protection. Or make them DCHECKs?
What do you think?

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:53: if (!profile ||
profile->IsOffTheRecord())
On 2013/09/05 15:14:22, kalman wrote:
> |profile| can't be null, no need to guard.

Done.

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:79: } // namespace
On 2013/09/05 15:14:22, kalman wrote:
> 2 spaces

Done.

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/23847004/diff/56001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:269:
throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
On 2013/09/05 22:52:15, darin wrote:
> You are installing this throttle for each and every URL request, whether for
> top-level frames, subframes or subresources.  Since you are adding a delay to
> route through the UI thread, this can add up to a lot of wasted cycles.
> 
> I think you should probably only register your throttle for MAIN_FRAME
resource
> loads, and presumably not for pre-rendering requests. You probably need to
make
> sure these are not pre-rendered in some other way, right?
> 
> I think you should just duplicate the check that is done below in the
OS_ANDROID
> case.

Done. I meant to do that, but somehow left it like this after debugging back and
forth. I've also #ifdefed this out of ANDROID and IOS now.

[darin (slow to review), 2013-09-05 23:54:25]

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:41: if (params.is_post())
I would just use DCHECKs for anything that is redundant. I don't know if
IsSubframe() is really needed. I would guess that it is, but I'm not certain.

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:272: if
(!is_prerendering && resource_type == ResourceType::MAIN_FRAME &&
I don't think this file should be built in the OS_IOS build. Have you found
otherwise?

Wouldn't it be better to share the first two checks with OS_ANDROID?

  if (!is_prerendering && resource_type == ResourceType::MAIN_FRAME) {
#if defined(OS_ANDROID)
    ...
#else
    if (request->method() == "GET")
      throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
#endif
  }

^^^ Seems better to test for GET instead of POST. Since you are only filtering
on navigations it doesn't strictly matter, but in case some other kind of HTTP
method ends up being used for navigations, it might be nice to not accidentally
route those as well to the app.

[not at google - send to devlin, 2013-09-06 00:51:29]

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:274:
throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
come to think of it, you should only need to register this if there is an app
that has a URL handler, which in the common case isn't true.

make sure to test adding/removing apps with URL handler(s) though.

[sergeygs, 2013-09-06 02:51:13]

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:41: if (params.is_post())
On 2013/09/05 23:54:25, darin wrote:
> I would just use DCHECKs for anything that is redundant. I don't know if
> IsSubframe() is really needed. I would guess that it is, but I'm not certain.

Done. IsSubframe() is needed. MAIN_FRAME check around where this throttle is
created doesn't work for webviews.

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:272: if
(!is_prerendering && resource_type == ResourceType::MAIN_FRAME &&
On 2013/09/05 23:54:25, darin wrote:
> I don't think this file should be built in the OS_IOS build. Have you found
> otherwise?
> 
> Wouldn't it be better to share the first two checks with OS_ANDROID?
> 
>   if (!is_prerendering && resource_type == ResourceType::MAIN_FRAME) {
> #if defined(OS_ANDROID)
>     ...
> #else
>     if (request->method() == "GET")
>       throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
> #endif
>   }
> 
> ^^^ Seems better to test for GET instead of POST. Since you are only filtering
> on navigations it doesn't strictly matter, but in case some other kind of HTTP
> method ends up being used for navigations, it might be nice to not
accidentally
> route those as well to the app.

Done. I guess you're right about this file not build for OS_IOS.

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:274:
throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
On 2013/09/06 00:51:30, kalman wrote:
> come to think of it, you should only need to register this if there is an app
> that has a URL handler, which in the common case isn't true.
> 
> make sure to test adding/removing apps with URL handler(s) though.

Sure, I'll test that case. This is registered for top-level navigations only, so
it shouldn't be very expensive overall. It could be possible to prefind a
handling app before creating a throttle, but as discussed in person, this seems
to be difficult right now. Leaving as is.

What I can do here easily (just noticed), is skip adding a throttle in
incognito: I'll do that.

[not at google - send to devlin, 2013-09-06 03:02:10]

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:274:
throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
On 2013/09/06 02:51:14, sergeygs wrote:
> On 2013/09/06 00:51:30, kalman wrote:
> > come to think of it, you should only need to register this if there is an
app
> > that has a URL handler, which in the common case isn't true.
> > 
> > make sure to test adding/removing apps with URL handler(s) though.
> 
> Sure, I'll test that case. This is registered for top-level navigations only,
so
> it shouldn't be very expensive overall. It could be possible to prefind a
> handling app before creating a throttle, but as discussed in person, this
seems
> to be difficult right now. Leaving as is.

As more recently discussed in person, the ExtensionInfoMap is already available
to this class so this can actually be simplified some more.

> 
> What I can do here easily (just noticed), is skip adding a throttle in
> incognito: I'll do that.

[sergeygs, 2013-09-06 03:44:52]

On 2013/09/06 03:02:10, kalman wrote:
>
https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
> File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/23847004/diff/80001/chrome/browser/renderer_h...
> chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:274:
> throttles->push_back(AppUrlRedirector::CreateThrottleFor(request));
> On 2013/09/06 02:51:14, sergeygs wrote:
> > On 2013/09/06 00:51:30, kalman wrote:
> > > come to think of it, you should only need to register this if there is an
> app
> > > that has a URL handler, which in the common case isn't true.
> > > 
> > > make sure to test adding/removing apps with URL handler(s) though.
> > 
> > Sure, I'll test that case. This is registered for top-level navigations
only,
> so
> > it shouldn't be very expensive overall. It could be possible to prefind a
> > handling app before creating a throttle, but as discussed in person, this
> seems
> > to be difficult right now. Leaving as is.
> 
> As more recently discussed in person, the ExtensionInfoMap is already
available
> to this class so this can actually be simplified some more.
> 
> > 
> > What I can do here easily (just noticed), is skip adding a throttle in
> > incognito: I'll do that.

Patch 14 moves the whole logic of finding a handling app to the IO side, in
AppUrlRedirector::MaybeCreateThrottleFor(). The created throttle, if any, stores
 the found extension and all else that's needed to launch it. The UI part in
LaunchAppWithUrl() is now basically a relay. If a request has no matching apps,
not throttle gets created, so no bouncing off of the UI thread.

[not at google - send to devlin, 2013-09-06 03:53:59]

sweet. 1 little comment.

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:32: const extensions::Extension* app,
pass a scoped_refptr here

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:41:
DCHECK(extensions::UrlHandlers::CanExtensionHandleUrl(app, params.url()));
if you want

[darin (slow to review), 2013-09-06 04:18:47]

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:81: if
(render_view_host->IsSubframe())
it is not valid to call RenderViewHost methods on the IO thread.

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:269:
bool incognito = io_data->is_incognito();
it doesn't look like you need to add the 'incognito' variable here. it is only
used in one place below. you could just leave it where it was.

[darin (slow to review), 2013-09-06 04:19:05]

On 2013/09/06 04:18:47, darin wrote:
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
> File chrome/browser/apps/app_url_redirector.cc (right):
> 
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
> chrome/browser/apps/app_url_redirector.cc:81: if
> (render_view_host->IsSubframe())
> it is not valid to call RenderViewHost methods on the IO thread.
> 
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/renderer_h...
> File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/renderer_h...
> chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:269:
> bool incognito = io_data->is_incognito();
> it doesn't look like you need to add the 'incognito' variable here. it is only
> used in one place below. you could just leave it where it was.

RenderViewHost is not a thread-safe class.

[sergeygs, 2013-09-06 05:46:45]

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:32: const extensions::Extension* app,
On 2013/09/06 03:53:59, kalman wrote:
> pass a scoped_refptr here

Done.

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:41:
DCHECK(extensions::UrlHandlers::CanExtensionHandleUrl(app, params.url()));
On 2013/09/06 03:53:59, kalman wrote:
> if you want

?

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:81: if
(render_view_host->IsSubframe())
On 2013/09/06 04:18:48, darin wrote:
> it is not valid to call RenderViewHost methods on the IO thread.

Noted and done.

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:269:
bool incognito = io_data->is_incognito();
On 2013/09/06 04:18:48, darin wrote:
> it doesn't look like you need to add the 'incognito' variable here. it is only
> used in one place below. you could just leave it where it was.

Yes, right. I originally moved it to reuse it in the 'if' around the new
throttle, but that 'if' has disappeared since then.

[not at google - send to devlin, 2013-09-06 14:11:32]

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
chrome/browser/apps/app_url_redirector.cc:41:
DCHECK(extensions::UrlHandlers::CanExtensionHandleUrl(app, params.url()));
On 2013/09/06 05:46:46, sergeygs wrote:
> On 2013/09/06 03:53:59, kalman wrote:
> > if you want
> 
> ?

Sorry, I was being unnecessarily cryptic.

What I mean to say is: I've said before that (in my opinion) DCHECKs aren't very
effective (the strong exception I make to this rule is
BrowserThread::CurrentlyOn DCHECKs).

I don't think there's much point in them here, but... leave them in if you
really want.

[sergeygs, 2013-09-06 17:24:50]

On 2013/09/06 14:11:32, kalman wrote:
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
> File chrome/browser/apps/app_url_redirector.cc (right):
> 
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
> chrome/browser/apps/app_url_redirector.cc:41:
> DCHECK(extensions::UrlHandlers::CanExtensionHandleUrl(app, params.url()));
> On 2013/09/06 05:46:46, sergeygs wrote:
> > On 2013/09/06 03:53:59, kalman wrote:
> > > if you want
> > 
> > ?
> 
> Sorry, I was being unnecessarily cryptic.
> 
> What I mean to say is: I've said before that (in my opinion) DCHECKs aren't
very
> effective (the strong exception I make to this rule is
> BrowserThread::CurrentlyOn DCHECKs).
> 
> I don't think there's much point in them here, but... leave them in if you
> really want.

This particular DCHECK helped me a lot yesterday during testing, so I'd prefer
to leave it in.

[not at google - send to devlin, 2013-09-06 17:26:00]

On 2013/09/06 17:24:50, sergeygs wrote:
> On 2013/09/06 14:11:32, kalman wrote:
> >
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
> > File chrome/browser/apps/app_url_redirector.cc (right):
> > 
> >
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
> > chrome/browser/apps/app_url_redirector.cc:41:
> > DCHECK(extensions::UrlHandlers::CanExtensionHandleUrl(app, params.url()));
> > On 2013/09/06 05:46:46, sergeygs wrote:
> > > On 2013/09/06 03:53:59, kalman wrote:
> > > > if you want
> > > 
> > > ?
> > 
> > Sorry, I was being unnecessarily cryptic.
> > 
> > What I mean to say is: I've said before that (in my opinion) DCHECKs aren't
> very
> > effective (the strong exception I make to this rule is
> > BrowserThread::CurrentlyOn DCHECKs).
> > 
> > I don't think there's much point in them here, but... leave them in if you
> > really want.
> 
> This particular DCHECK helped me a lot yesterday during testing, so I'd prefer
> to leave it in.

Which DCHECK?

I mean, LOGs are also helpful, but you don't leave those in...

[sergeygs, 2013-09-06 18:14:48]

On 2013/09/06 17:26:00, kalman wrote:
> On 2013/09/06 17:24:50, sergeygs wrote:
> > On 2013/09/06 14:11:32, kalman wrote:
> > >
> >
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
> > > File chrome/browser/apps/app_url_redirector.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23847004/diff/15001/chrome/browser/apps/app_u...
> > > chrome/browser/apps/app_url_redirector.cc:41:
> > > DCHECK(extensions::UrlHandlers::CanExtensionHandleUrl(app, params.url()));
> > > On 2013/09/06 05:46:46, sergeygs wrote:
> > > > On 2013/09/06 03:53:59, kalman wrote:
> > > > > if you want
> > > > 
> > > > ?
> > > 
> > > Sorry, I was being unnecessarily cryptic.
> > > 
> > > What I mean to say is: I've said before that (in my opinion) DCHECKs
aren't
> > very
> > > effective (the strong exception I make to this rule is
> > > BrowserThread::CurrentlyOn DCHECKs).
> > > 
> > > I don't think there's much point in them here, but... leave them in if you
> > > really want.
> > 
> > This particular DCHECK helped me a lot yesterday during testing, so I'd
prefer
> > to leave it in.
> 
> Which DCHECK?
> 
> I mean, LOGs are also helpful, but you don't leave those in...

The one that checks that the app stored on this particular throttle can indeed
handle |request.url|.

[darin (slow to review), 2013-09-06 19:10:17]

LGTM (I don't have a strong opinion on the DCHECK debate)

[not at google - send to devlin, 2013-09-06 19:12:30]

https://codereview.chromium.org/23847004/diff/113001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest_util.h (right):

https://codereview.chromium.org/23847004/diff/113001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.h:33: const Extension*
InstallPlatformApp(const char* name);
noticed this change. did you mean to add a test as well?

[sergeygs1, 2013-09-06 20:57:37]

Adding several new tests as we speak.


On Fri, Sep 6, 2013 at 12:12 PM, <kalman@chromium.org> wrote:

>
> https://codereview.chromium.**org/23847004/diff/113001/**
>
chrome/browser/apps/app_**browsertest_util.h<https://codereview.chromium.org/23847004/diff/113001/chrome/browser/apps/app_browsertest_util.h>
> File chrome/browser/apps/app_**browsertest_util.h (right):
>
> https://codereview.chromium.**org/23847004/diff/113001/**
>
chrome/browser/apps/app_**browsertest_util.h#newcode33<https://codereview.chromium.org/23847004/diff/113001/chrome/browser/apps/app_browsertest_util.h#newcode33>
> chrome/browser/apps/app_**browsertest_util.h:33: const Extension*
> InstallPlatformApp(const char* name);
> noticed this change. did you mean to add a test as well?
>
>
https://codereview.chromium.**org/23847004/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sergeygs, 2013-09-07 00:15:09]

On 2013/09/06 23:57:44, sergeygs wrote:

Added browser tests for:
- interception of a regular link click in a tab
- interception of a target=_blank link click in a tab
- interception of window.open() from a tab
- interception of a target=_blank link click in an app's window
- navigating a webview in an app's window to a url that the same app is a
handler of (should not create an endless interception loop: the webview should
just navigate)

[not at google - send to devlin, 2013-09-07 00:54:55]

had a look at the tests. I understand that you need to get this landed before
the feature cutoff, so use your judgement how much of my suggestions to
implement. but we need to get there eventually, the tests are missing a lot of
cases and seem spread a little too thin. also the parsing code needs unit tests,
but I suppose you can add those next week too.

I don't think I can commit to reviewing any more today, kind of burnt out, and
not over the weekend either. I can have a look first thing on Monday though.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:642:
UrlHandlersNavigationsInTabIntercepted) {
can you put these tests in a different file? This one is already too big.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:656: };
might as well use const char*s here rather than std::string, and perhaps
StringPrintf below.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:658: for (size_t i = 0; i < sizeof(hrefs)
/ sizeof(*hrefs); ++i) {
use arraysize(hrefs) or ARRAYSIZE_UNSAFE if that doesn't work.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:673: ASSERT_EQ(3U,
GetShellWindowCount());
asserting based on the number of shell windows open is a bit weak. better would
be to use chrome.test.sendMessage from the onLaunched event, intercept it from
here, then assert about the contents. rather than from the background page. the
test is driven from here; assertions should be made from here too.

if you make the apps dumb and bring the logic into here you can write more
complex tests. for example, you haven't tested subdomains not matching, http vs
https, incognito mode, POST requests, XHR...

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:73: if
(!request->url().SchemeIsHTTPOrHTTPS())
ok - but make sure that you don't allow http or https in the match patterns
then. warn if they have them. otherwise it will mysteriously fail here.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:93: scoped_refptr<const
Extension>(*iter),
make_scoped_refptr(*iter) should do it.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/url_handlers/handler/test.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler/test.js:21:
"Wrong launchData.url");
better would be to send a message back to the test with the launch data, like
chrome.test.sendMessage(JSON.stringify(launchData)) and assert the contents in
there. that brings the knowledge of the environment out of the app and into the
test, so it's localised.

there's not much needed here besides an app that dumbly accepts launch events
and echoes them back to the test. you can then execute code directly in the app
to do things like open links etc.

one reference is the externally connectable tests which do something similar,
and actually test with real domains (it would be good to do that rather than
testing localhost and an IP address, so that you can test things like subdomains
*not* redirecting):

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

you can then execute scripts in the background page with this:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

for example:

std::string script =
"var a = document.createElement('a');\n"
"a.href = 'http://some-domain.com';\n"
"a.click();\n";
ExecuteScriptInBackgroundPage(profile(), id, script);

which is all you need. most of the HTML is unnecessary.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js:17:
var webview = this.document.getElementById('wv');
no point in having |this|?

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js:24:
}, 3000);
is there some kind of event you can listen to? setTimeout will either be flaky,
slow, or both.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/url_handlers/launching_tabs/navigate.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/launching_tabs/navigate.js:17:
link.dispatchEvent(clickEvent);
does link.click() not work?

[darin (slow to review), 2013-09-07 06:15:19]

On 2013/09/06 19:10:17, darin wrote:
> LGTM (I don't have a strong opinion on the DCHECK debate)

I should qualify that my LGTM was for the core interception bits.

[benwells, 2013-09-09 05:48:38]

This is looking really good, the bits I am interested in seem really close. I
haven't looked at the test data files at all yet and am too tired to look
further (got off a plane a few hours ago).

https://codereview.chromium.org/23847004/diff/149001/apps/launcher.h
File apps/launcher.h (right):

https://codereview.chromium.org/23847004/diff/149001/apps/launcher.h#newcode64
apps/launcher.h:64: }  // namespace apps
Nit: restore blank line above.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest_util.h (right):

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.h:93: class
PlatformAppUrlRedirectorBrowserTest : public PlatformAppBrowserTest {
Is there a reason this class is in this module? It would normally go in the file
that defines the tests, unless it is used in multiple test files.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:24: 
Nit: remove extra blank line.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:66: content::ResourceThrottle*
(not a comment to affect this CL, just a comment) ... If ResourceThrottles are
meant to be used for redirection like this (or registerProtocolHandler for
example), maybe the name should change - this doesn't feel like throttling.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:5: #include
"chrome/browser/apps/app_browsertest_util.h"
You also need tests to cover navigation with a url_handler app installed, that
should not go to the app.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:16: 1U);
The InTab variants always have 1U, and the InApp variants always have 2U. Do
these need to be parameters? The app path and message don't look like they need
to be parameterized either.

[sergeygs, 2013-09-09 09:55:35]

Added a lot of new tests including:
- navigate by clicking a mismatching link
- redirect/navigate by entering a matching/mismatching URL in the omnibar
- not navigate when sending an XHR, even with a matching URL.
- not navigate on a form submission with a matching URL.
- etc.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:642:
UrlHandlersNavigationsInTabIntercepted) {
On 2013/09/07 00:54:56, kalman wrote:
> can you put these tests in a different file? This one is already too big.

Done. If we start in this direction, it would be nice to split this file further
based on groups of tests (e.g. there is something like a dozen file_handlers
tests).

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:656: };
On 2013/09/07 00:54:56, kalman wrote:
> might as well use const char*s here rather than std::string, and perhaps
> StringPrintf below.

Done.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:658: for (size_t i = 0; i < sizeof(hrefs)
/ sizeof(*hrefs); ++i) {
On 2013/09/07 00:54:56, kalman wrote:
> use arraysize(hrefs) or ARRAYSIZE_UNSAFE if that doesn't work.

Obsolete.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:673: ASSERT_EQ(3U,
GetShellWindowCount());
On 2013/09/07 00:54:56, kalman wrote:
> asserting based on the number of shell windows open is a bit weak. better
would
> be to use chrome.test.sendMessage from the onLaunched event, intercept it from
> here, then assert about the contents. rather than from the background page.
the
> test is driven from here; assertions should be made from here too.
> 
> if you make the apps dumb and bring the logic into here you can write more
> complex tests. for example, you haven't tested subdomains not matching, http
vs
> https, incognito mode, POST requests, XHR...

chrome.test.sendMessage _is_ used here, see the end of the loop. The ASSERT_EQ
here is just to test that no extra windows have been opened.

Your point about driving and asserting in the same place is valid in general,
but here it can't be fully applied in any case. One of the reasons is that these
tests verify a declarative manifest entry: I need to have a specific URL pattern
in the handler app's manifest. It makes sense then to have a matching concrete
URL somewhere nearby, e.g. in the launcher app.

Also, this approach is used all around the tests: I just followed the suit.
Mentioning that not as a justification, but as an explanation.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:73: if
(!request->url().SchemeIsHTTPOrHTTPS())
On 2013/09/07 00:54:56, kalman wrote:
> ok - but make sure that you don't allow http or https in the match patterns
> then. warn if they have them. otherwise it will mysteriously fail here.

That has been enforced in the manifest parser since the beginning, see
url_handlers_parser.cc:117.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:93: scoped_refptr<const
Extension>(*iter),
On 2013/09/07 00:54:56, kalman wrote:
> make_scoped_refptr(*iter) should do it.

Any real advantage?

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/url_handlers/handler/test.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler/test.js:21:
"Wrong launchData.url");
On 2013/09/07 00:54:56, kalman wrote:
> better would be to send a message back to the test with the launch data, like
> chrome.test.sendMessage(JSON.stringify(launchData)) and assert the contents in
> there. that brings the knowledge of the environment out of the app and into
the
> test, so it's localised.
> 
> there's not much needed here besides an app that dumbly accepts launch events
> and echoes them back to the test. you can then execute code directly in the
app
> to do things like open links etc.
> 
> one reference is the externally connectable tests which do something similar,
> and actually test with real domains (it would be good to do that rather than
> testing localhost and an IP address, so that you can test things like
subdomains
> *not* redirecting):
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> you can then execute scripts in the background page with this:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> for example:
> 
> std::string script =
> "var a = document.createElement('a');\n"
> "a.href = 'http://some-domain.com';\n"
> "a.click();\n";
> ExecuteScriptInBackgroundPage(profile(), id, script);
> 
> which is all you need. most of the HTML is unnecessary.

I'll consider this for the next round of test improvements. From what I can
tell, this is a pretty drastic departure from how things are done in the
majority of the tests within the area I've been adding my tests to.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js:17:
var webview = this.document.getElementById('wv');
On 2013/09/07 00:54:56, kalman wrote:
> no point in having |this|?

Done.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js:24:
}, 3000);
On 2013/09/07 00:54:56, kalman wrote:
> is there some kind of event you can listen to? setTimeout will either be
flaky,
> slow, or both.

Indeed, there is. Added. But I've still kept a small delay after I receive that
event, to really make sure no new delayed windows have been opened.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/url_handlers/launching_tabs/navigate.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/launching_tabs/navigate.js:17:
link.dispatchEvent(clickEvent);
On 2013/09/07 00:54:56, kalman wrote:
> does link.click() not work?

It does, but MouseEvents gives me control over modifier keys and other
attributes of the click (I'm going to use that when I'm extending these tests).

https://codereview.chromium.org/23847004/diff/149001/apps/launcher.h
File apps/launcher.h (right):

https://codereview.chromium.org/23847004/diff/149001/apps/launcher.h#newcode64
apps/launcher.h:64: }  // namespace apps
On 2013/09/09 05:48:39, benwells wrote:
> Nit: restore blank line above.

Done. Not sure how it disappeared.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest_util.h (right):

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.h:93: class
PlatformAppUrlRedirectorBrowserTest : public PlatformAppBrowserTest {
On 2013/09/09 05:48:39, benwells wrote:
> Is there a reason this class is in this module? It would normally go in the
file
> that defines the tests, unless it is used in multiple test files.

No real reason, it just seemed to make sense. Moved.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:24: 
On 2013/09/09 05:48:39, benwells wrote:
> Nit: remove extra blank line.

Done.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:66: content::ResourceThrottle*
On 2013/09/09 05:48:39, benwells wrote:
> (not a comment to affect this CL, just a comment) ... If ResourceThrottles are
> meant to be used for redirection like this (or registerProtocolHandler for
> example), maybe the name should change - this doesn't feel like throttling.

I agree: that was one of the confusing things about this classes when I first
looked at them.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:5: #include
"chrome/browser/apps/app_browsertest_util.h"
On 2013/09/09 05:48:39, benwells wrote:
> You also need tests to cover navigation with a url_handler app installed, that
> should not go to the app.

Already did. See the most recent patch with new tests.

https://codereview.chromium.org/23847004/diff/149001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:16: 1U);
On 2013/09/09 05:48:39, benwells wrote:
> The InTab variants always have 1U, and the InApp variants always have 2U. Do
> these need to be parameters? The app path and message don't look like they
need
> to be parameterized either.
> 

Already (re)done in a different way.

[not at google - send to devlin, 2013-09-09 19:01:11]

Commenting on your comments. Looking at the new version of the CL after this.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:642:
UrlHandlersNavigationsInTabIntercepted) {
On 2013/09/09 09:55:36, sergeygs wrote:
> On 2013/09/07 00:54:56, kalman wrote:
> > can you put these tests in a different file? This one is already too big.
> 
> Done. If we start in this direction, it would be nice to split this file
further
> based on groups of tests (e.g. there is something like a dozen file_handlers
> tests).

Absolutely.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:673: ASSERT_EQ(3U,
GetShellWindowCount());
On 2013/09/09 09:55:36, sergeygs wrote:
> On 2013/09/07 00:54:56, kalman wrote:
> > asserting based on the number of shell windows open is a bit weak. better
> would
> > be to use chrome.test.sendMessage from the onLaunched event, intercept it
from
> > here, then assert about the contents. rather than from the background page.
> the
> > test is driven from here; assertions should be made from here too.
> > 
> > if you make the apps dumb and bring the logic into here you can write more
> > complex tests. for example, you haven't tested subdomains not matching, http
> vs
> > https, incognito mode, POST requests, XHR...
> 
> chrome.test.sendMessage _is_ used here, see the end of the loop. The ASSERT_EQ
> here is just to test that no extra windows have been opened.

Ah, I see. Thanks.

> 
> Your point about driving and asserting in the same place is valid in general,
> but here it can't be fully applied in any case. One of the reasons is that
these
> tests verify a declarative manifest entry: I need to have a specific URL
pattern
> in the handler app's manifest. It makes sense then to have a matching concrete
> URL somewhere nearby, e.g. in the launcher app.

externally_connectable has the exact same problem - see those tests. Basically
there is a class called TestExtensionDir which lets you write your own manifest
files. See:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

> 
> Also, this approach is used all around the tests: I just followed the suit.
> Mentioning that not as a justification, but as an explanation.

Right. Agreed. Test cleanup is a slow process but particularly with this in its
own file, breaking untidy test idioms is not really an issue. Then further tests
which base themselves off your tests will be better.

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:93: scoped_refptr<const
Extension>(*iter),
On 2013/09/09 09:55:36, sergeygs wrote:
> On 2013/09/07 00:54:56, kalman wrote:
> > make_scoped_refptr(*iter) should do it.
> 
> Any real advantage?

In as much as it's (a) idiomatic (b) less code, so yes.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/url_handlers/handler/test.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler/test.js:21:
"Wrong launchData.url");
On 2013/09/09 09:55:36, sergeygs wrote:
> On 2013/09/07 00:54:56, kalman wrote:
> > better would be to send a message back to the test with the launch data,
like
> > chrome.test.sendMessage(JSON.stringify(launchData)) and assert the contents
in
> > there. that brings the knowledge of the environment out of the app and into
> the
> > test, so it's localised.
> > 
> > there's not much needed here besides an app that dumbly accepts launch
events
> > and echoes them back to the test. you can then execute code directly in the
> app
> > to do things like open links etc.
> > 
> > one reference is the externally connectable tests which do something
similar,
> > and actually test with real domains (it would be good to do that rather than
> > testing localhost and an IP address, so that you can test things like
> subdomains
> > *not* redirecting):
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > 
> > you can then execute scripts in the background page with this:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > 
> > for example:
> > 
> > std::string script =
> > "var a = document.createElement('a');\n"
> > "a.href = 'http://some-domain.com';\n"
> > "a.click();\n";
> > ExecuteScriptInBackgroundPage(profile(), id, script);
> > 
> > which is all you need. most of the HTML is unnecessary.
> 
> I'll consider this for the next round of test improvements. From what I can
> tell, this is a pretty drastic departure from how things are done in the
> majority of the tests within the area I've been adding my tests to.

Ok. See other comment I made about test improvements.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js:24:
}, 3000);
On 2013/09/09 09:55:36, sergeygs wrote:
> On 2013/09/07 00:54:56, kalman wrote:
> > is there some kind of event you can listen to? setTimeout will either be
> flaky,
> > slow, or both.
> 
> Indeed, there is. Added. But I've still kept a small delay after I receive
that
> event, to really make sure no new delayed windows have been opened.

Thanks.

Except it's still going to flaky and make the test slower? If "delayed windows"
have been opened then you should be able to detect that by flushing the browser
event loop or something and asserting the correct number of windows.

[not at google - send to devlin, 2013-09-09 19:27:20]

rushing off to lunch, made 1 more comment, will continue after then.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:39: void
TestMismatchingNavigationInTab(const char* launcher_page,
I haven't seen this idiom before. Why is this different to the
IN_PROC_BROWSER_TEST versions?

[sergeygs, 2013-09-09 20:33:02]

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector.cc (right):

https://codereview.chromium.org/23847004/diff/120001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector.cc:93: scoped_refptr<const
Extension>(*iter),
On 2013/09/09 19:01:13, kalman wrote:
> On 2013/09/09 09:55:36, sergeygs wrote:
> > On 2013/09/07 00:54:56, kalman wrote:
> > > make_scoped_refptr(*iter) should do it.
> > 
> > Any real advantage?
> 
> In as much as it's (a) idiomatic (b) less code, so yes.

Actually, all of this is unnecessary. *iter already is a scoped_refptr<const
Extension>, and it's pushed on the stack by value. So I'm leaving just *iter
here.

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js
(right):

https://codereview.chromium.org/23847004/diff/120001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/url_handlers/handler_with_webview/test.js:24:
}, 3000);
On 2013/09/09 19:01:13, kalman wrote:
> On 2013/09/09 09:55:36, sergeygs wrote:
> > On 2013/09/07 00:54:56, kalman wrote:
> > > is there some kind of event you can listen to? setTimeout will either be
> > flaky,
> > > slow, or both.
> > 
> > Indeed, there is. Added. But I've still kept a small delay after I receive
> that
> > event, to really make sure no new delayed windows have been opened.
> 
> Thanks.
> 
> Except it's still going to flaky and make the test slower? If "delayed
windows"
> have been opened then you should be able to detect that by flushing the
browser
> event loop or something and asserting the correct number of windows.

Ok, removed the delay.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:39: void
TestMismatchingNavigationInTab(const char* launcher_page,
On 2013/09/09 19:27:22, kalman wrote:
> I haven't seen this idiom before. Why is this different to the
> IN_PROC_BROWSER_TEST versions?

What do you mean by "different"? This is just an internal implementation class
that facilitates easier writing of IN_PROC_BROWSER_TEST tests at the bottom of
this file. Which reminds me that I intended to put it in an anonymous namespace,
but forgot. Will do it now.

[not at google - send to devlin, 2013-09-09 21:01:04]

A day of sporadic interruptions, sorry; have a meeting now, so a few more
comments for now. That should be enough to go on for the next hour though.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest_util.cc (right):

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.cc:12: #include
"base/strings/utf_string_conversions.h"
not used

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.cc:26: using base::ASCIIToUTF16;
not used

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:39: void
TestMismatchingNavigationInTab(const char* launcher_page,
On 2013/09/09 20:33:03, sergeygs wrote:
> On 2013/09/09 19:27:22, kalman wrote:
> > I haven't seen this idiom before. Why is this different to the
> > IN_PROC_BROWSER_TEST versions?
> 
> What do you mean by "different"? This is just an internal implementation class
> that facilitates easier writing of IN_PROC_BROWSER_TEST tests at the bottom of
> this file. Which reminds me that I intended to put it in an anonymous
namespace,
> but forgot. Will do it now.

Ok - I picked the only example here that wasn't actually used, and assumed they
all were like this, and on purpose.

So my comment should have been: Unused.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:81: void
TestNegativeNavigationInApp(const char* launcher,
also unused

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:104: void
TestMismatchingNavigationInBrowser(const char* mismatching_target_page,
this "Negative" vs "Mismatching" naming is confusing. Could you name them more
closely to what they do? Like: TestNavigatesFrom{Tab,Browser,..},
TestDoesNotNavigateFrom{Tab,Browser,...}. Then you can delete the Mismatching
names since all they do is call the Negative version.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:133:
ASSERT_TRUE(handler_listener.WaitUntilSatisfied());
it's better to return testing::AssertionErrors from these methods so that you
preserve line numbers and (if applicable) failure messages. So these would look
like

if (!handler_listener.WaitUntilSatisfied())
  return testing::AssertionFailure();
if (GetShellWindowCount() != 1U)
  return testing::AssertionFailure() << "Expected 1, got " <<
GetShellWindowCount();

etc

and call site,

EXPECT_TRUE(TestNavigationInTab(...));

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:285: }
Generally speaking the Negative and Non-Negative versions of these are all so
similar. You should be able to pull the Navigation core into a shared function.

[sergeygs, 2013-09-09 21:53:48]

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest_util.cc (right):

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.cc:12: #include
"base/strings/utf_string_conversions.h"
On 2013/09/09 21:01:05, kalman wrote:
> not used

Done.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.cc:26: using base::ASCIIToUTF16;
On 2013/09/09 21:01:05, kalman wrote:
> not used

Done.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
File chrome/browser/apps/app_url_redirector_browsertest.cc (right):

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:39: void
TestMismatchingNavigationInTab(const char* launcher_page,
On 2013/09/09 21:01:05, kalman wrote:
> On 2013/09/09 20:33:03, sergeygs wrote:
> > On 2013/09/09 19:27:22, kalman wrote:
> > > I haven't seen this idiom before. Why is this different to the
> > > IN_PROC_BROWSER_TEST versions?
> > 
> > What do you mean by "different"? This is just an internal implementation
class
> > that facilitates easier writing of IN_PROC_BROWSER_TEST tests at the bottom
of
> > this file. Which reminds me that I intended to put it in an anonymous
> namespace,
> > but forgot. Will do it now.
> 
> Ok - I picked the only example here that wasn't actually used, and assumed
they
> all were like this, and on purpose.
> 
> So my comment should have been: Unused.

Good catch. Just added a new test that uses it.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:81: void
TestNegativeNavigationInApp(const char* launcher,
On 2013/09/09 21:01:05, kalman wrote:
> also unused

Done. Added a test to use it.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:104: void
TestMismatchingNavigationInBrowser(const char* mismatching_target_page,
On 2013/09/09 21:01:05, kalman wrote:
> this "Negative" vs "Mismatching" naming is confusing. Could you name them more
> closely to what they do? Like: TestNavigatesFrom{Tab,Browser,..},
> TestDoesNotNavigateFrom{Tab,Browser,...}. Then you can delete the Mismatching
> names since all they do is call the Negative version.

Added a TODO for post-M31.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:133:
ASSERT_TRUE(handler_listener.WaitUntilSatisfied());
On 2013/09/09 21:01:05, kalman wrote:
> it's better to return testing::AssertionErrors from these methods so that you
> preserve line numbers and (if applicable) failure messages. So these would
look
> like
> 
> if (!handler_listener.WaitUntilSatisfied())
>   return testing::AssertionFailure();
> if (GetShellWindowCount() != 1U)
>   return testing::AssertionFailure() << "Expected 1, got " <<
> GetShellWindowCount();
> 
> etc
> 
> and call site,
> 
> EXPECT_TRUE(TestNavigationInTab(...));

Added a TODO for post-M31.

https://codereview.chromium.org/23847004/diff/166001/chrome/browser/apps/app_...
chrome/browser/apps/app_url_redirector_browsertest.cc:285: }
On 2013/09/09 21:01:05, kalman wrote:
> Generally speaking the Negative and Non-Negative versions of these are all so
> similar. You should be able to pull the Navigation core into a shared
function.

Added a TODO for post-M31.

[not at google - send to devlin, 2013-09-09 22:00:53]

ok. I am not happy with these tests but lgtm to get this on dev channel. fix
tests before flipping the flag to enable this past dev channel.

[benwells, 2013-09-09 22:06:05]

Nice, lgtm.

Leaving thorough testing review to kalman, I'm happy with the overall structure
and coverage.

[not at google - send to devlin, 2013-09-09 22:11:22]

lgtm to try to make my name show up green instead of red

[not at google - send to devlin, 2013-09-09 22:25:34]

and then follow them up.

https://codereview.chromium.org/23847004/diff/167001/chrome/common/extensions...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):

https://codereview.chromium.org/23847004/diff/167001/chrome/common/extensions...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:124:
handler.patterns.AddPattern(pattern);
Please also add TODO(..) to restrict this feature to non-TLDs, like
externally_connectable does.

and then, another TODO to further restrict it to only what the webstore tells
it.

[sergeygs, 2013-09-09 23:39:56]

On 2013/09/09 22:25:34, kalman wrote:
> and then follow them up.
> 
>
https://codereview.chromium.org/23847004/diff/167001/chrome/common/extensions...
> File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):
> 
>
https://codereview.chromium.org/23847004/diff/167001/chrome/common/extensions...
> chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:124:
> handler.patterns.AddPattern(pattern);
> Please also add TODO(..) to restrict this feature to non-TLDs, like
> externally_connectable does.
> 
> and then, another TODO to further restrict it to only what the webstore tells
> it.

Added TODOs.

[sergeygs, 2013-09-09 23:41:38]

https://codereview.chromium.org/23847004/diff/167001/chrome/common/extensions...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):

https://codereview.chromium.org/23847004/diff/167001/chrome/common/extensions...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:124:
handler.patterns.AddPattern(pattern);
On 2013/09/09 22:25:35, kalman wrote:
> Please also add TODO(..) to restrict this feature to non-TLDs, like
> externally_connectable does.
> 
> and then, another TODO to further restrict it to only what the webstore tells
> it.

Done.

[commit-bot: I haz the power, 2013-09-10 00:12:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeygs@chromium.org/23847004/192001

[commit-bot: I haz the power, 2013-09-10 03:53:03]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-10 06:00:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeygs@chromium.org/23847004/192001

[commit-bot: I haz the power, 2013-09-10 09:11:42]

Change committed as 222235