don't display platform app resources in normal browser windows

Don't display platform app resources in normal browser windows.

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

[Evan Stade, 2012-09-06 18:29:52]

http://codereview.chromium.org/10920084/diff/1/chrome/browser/ui/extensions/s...
File chrome/browser/ui/extensions/shell_window.cc (left):

http://codereview.chromium.org/10920084/diff/1/chrome/browser/ui/extensions/s...
chrome/browser/ui/extensions/shell_window.cc:222: // Don't allow the current tab
to be navigated. It would be nice to map all
you can view this change as a proposal. Here is a brief summary:

GOOD: all kinds of anchor links open a new tab in a browser
GOOD: normal anchor links to app resources throw an error and do nothing
BAD: target="_blank" anchor links to app resources open a new about:blank tab in
a browser window and do not throw an error
GOOD: meta refresh fails
MEH: meta redirect works if the url is valid
BAD: meta redirect creates the about:blank tab if the URL is app resource

note that even the BADs are better than the current behavior (which is to
actually load the app resource in the browser).

[Mihai Parparita -not on Chrome, 2012-09-06 18:40:35]

http://codereview.chromium.org/10920084/diff/1/chrome/browser/ui/extensions/s...
File chrome/browser/ui/extensions/shell_window.cc (left):

http://codereview.chromium.org/10920084/diff/1/chrome/browser/ui/extensions/s...
chrome/browser/ui/extensions/shell_window.cc:222: // Don't allow the current tab
to be navigated. It would be nice to map all
On 2012/09/06 18:29:52, Evan Stade wrote:
> you can view this change as a proposal. Here is a brief summary:
> 
> GOOD: all kinds of anchor links open a new tab in a browser
> GOOD: normal anchor links to app resources throw an error and do nothing
> BAD: target="_blank" anchor links to app resources open a new about:blank tab
in
> a browser window and do not throw an error
> GOOD: meta refresh fails
> MEH: meta redirect works if the url is valid
> BAD: meta redirect creates the about:blank tab if the URL is app resource
> 
> note that even the BADs are better than the current behavior (which is to
> actually load the app resource in the browser).

This seems OK to me. To make it more obvious in the BAD cases that the behavior
is intentional, could the URL be rewritten to chrome-extension://invalid/ (what
we for for extension resources that are not web accessible), instead of dropping
the navigation on the floor?

[Evan Stade, 2012-09-06 19:19:03]

On 2012/09/06 18:40:35, Mihai Parparita wrote:
>
http://codereview.chromium.org/10920084/diff/1/chrome/browser/ui/extensions/s...
> File chrome/browser/ui/extensions/shell_window.cc (left):
> 
>
http://codereview.chromium.org/10920084/diff/1/chrome/browser/ui/extensions/s...
> chrome/browser/ui/extensions/shell_window.cc:222: // Don't allow the current
tab
> to be navigated. It would be nice to map all
> On 2012/09/06 18:29:52, Evan Stade wrote:
> > you can view this change as a proposal. Here is a brief summary:
> > 
> > GOOD: all kinds of anchor links open a new tab in a browser
> > GOOD: normal anchor links to app resources throw an error and do nothing
> > BAD: target="_blank" anchor links to app resources open a new about:blank
tab
> in
> > a browser window and do not throw an error
> > GOOD: meta refresh fails
> > MEH: meta redirect works if the url is valid
> > BAD: meta redirect creates the about:blank tab if the URL is app resource
> > 
> > note that even the BADs are better than the current behavior (which is to
> > actually load the app resource in the browser).
> 
> This seems OK to me. To make it more obvious in the BAD cases that the
behavior
> is intentional, could the URL be rewritten to chrome-extension://invalid/
(what
> we for for extension resources that are not web accessible), instead of
dropping
> the navigation on the floor?

ok sgtm. I will get to this some time next week.

also I meant that the meta redirect "works" in that it creates a new browser
tab, like an anchor link would. (Not sure if that was clear.)

[Mihai Parparita -not on Chrome, 2012-09-06 19:57:16]

On Thu, Sep 6, 2012 at 12:19 PM, <estade@chromium.org> wrote:

> also I meant that the meta redirect "works" in that it creates a new
> browser
> tab, like an anchor link would. (Not sure if that was clear.)
>

Yeah, I figured. It'll be a weird way to open a popup, but after thinking
about it more, I don't think it's worth trying to prevent.

Mihai

[Mihai Parparita -not on Chrome, 2012-09-07 18:21:37]

BTW, http://codereview.chromium.org/10915047/ may affect this CL.

Mihai

On Thu, Sep 6, 2012 at 12:57 PM, Mihai Parparita <mihaip@chromium.org>wrote:

> On Thu, Sep 6, 2012 at 12:19 PM, <estade@chromium.org> wrote:
>
>> also I meant that the meta redirect "works" in that it creates a new
>> browser
>> tab, like an anchor link would. (Not sure if that was clear.)
>>
>
> Yeah, I figured. It'll be a weird way to open a popup, but after thinking
> about it more, I don't think it's worth trying to prevent.
>
> Mihai
>

[Evan Stade, 2012-09-11 16:24:36]

updated. Did you literally mean chrome-extension://invalid? Is this used
somewhere else? It seems this should be defined in url_constants.h/cc if so.
Also, if we want to send clearer messaging we could create an error page that is
more specific than

"""
This webpage is not available
The webpage at chrome-extension://invalid/ might be temporarily down or it may
have moved permanently to a new web address.
Error 2 (net::ERR_FAILED): Unknown error.
"""

[Evan Stade, 2012-09-12 08:57:29]

ping

[Evan Stade, 2012-09-13 14:28:13]

ping

[jeremya, 2012-09-13 16:58:59]

Removing myself as a reviewer, since I don't know enough about the area. I think
Mihai wrote this code originally :)

[Mihai Parparita -not on Chrome, 2012-09-16 05:37:04]

Sorry for the delayed response, the hackathon made me be really behind on email.

PlatformAppBrowserTest.DisallowNavigation may need to be updated, and this new
behavior needs its own test.

http://codereview.chromium.org/10920084/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10920084/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2274: *url =
GURL(std::string(chrome::kExtensionScheme) + "://invalid/");
chrome-extension://invalid is used by
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/chrome_con....

Having a constant for it is a good idea. A prettier error page would be nice
too, though that can probably be done in a follow-up CL (generally, only
developers should see it).

[Evan Stade, 2012-09-26 13:30:49]

updated. I reverted part of the change; non-_blank anchor links will continue to
fail. This is because other navigations, like those in
PlatformAppBrowserTest.DisallowNavigation, were causing invalid tabs to pop up.
This wasn't making the test fail, but it seems much better to just fail right
away on window.location =, rather than loading it as a tab in the browser. I
also updated the test to make sure _blank navigations work as expected.

[Mihai Parparita -not on Chrome, 2012-09-26 21:59:06]

LGTM

[commit-bot: I haz the power, 2012-09-27 15:15:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/10920084/19001

[commit-bot: I haz the power, 2012-09-27 15:15:13]

Presubmit check for 10920084-19001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui

Presubmit checks took 3.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Evan Stade, 2012-09-28 10:03:09]

+sky for OWNERS review of ui/
+thestig for OWNERS review of chrome/

[sky, 2012-09-28 15:49:12]

LGTM

[Lei Zhang, 2012-09-28 18:50:31]

lgtm

http://codereview.chromium.org/10920084/diff/19001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10920084/diff/19001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:105: class
TabsAddedNotificationObserver :
nit: I think the linter will be happier if you put the colon on the next line.

[commit-bot: I haz the power, 2012-10-01 10:01:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/10920084/23001

[commit-bot: I haz the power, 2012-10-01 10:04:58]

Retried try job too often for step(s) crypto_unittests, unit_tests,
cacheinvalidation_unittests, remoting_unittests, jingle_unittests,
nacl_integration, ipc_tests, interactive_ui_tests, browser_tests, net_unittests,
content_unittests, gpu_unittests, base_unittests, sync_integration_tests,
media_unittests, safe_browsing_tests, content_browsertests, sql_unittests,
printing_unittests, sync_unit_tests, check_deps

[Mihai Parparita -not on Chrome, 2012-10-05 05:14:41]

On Wed, Sep 26, 2012 at 6:30 AM, <estade@chromium.org> wrote:

> updated. I reverted part of the change; non-_blank anchor links will
> continue to
> fail. This is because other navigations, like those in
> PlatformAppBrowserTest.**DisallowNavigation, were causing invalid tabs to
> pop up.
> This wasn't making the test fail, but it seems much better to just fail
> right
> away on window.location =, rather than loading it as a tab in the browser.
> I
> also updated the test to make sure _blank navigations work as expected.
>

One alternative that occurred to me would be to insert the equivalent of
<base target="_blank"> into document that's loaded into ShellWindow. That
way links would load in a new window, but window.location.href assignments
would still try to load in the same window (and fail).

[Evan Stade, 2012-10-05 08:26:35]

On 2012/10/05 05:14:41, Mihai Parparita wrote:
> On Wed, Sep 26, 2012 at 6:30 AM, <mailto:estade@chromium.org> wrote:
> 
> > updated. I reverted part of the change; non-_blank anchor links will
> > continue to
> > fail. This is because other navigations, like those in
> > PlatformAppBrowserTest.**DisallowNavigation, were causing invalid tabs to
> > pop up.
> > This wasn't making the test fail, but it seems much better to just fail
> > right
> > away on window.location =, rather than loading it as a tab in the browser.
> > I
> > also updated the test to make sure _blank navigations work as expected.
> >
> 
> One alternative that occurred to me would be to insert the equivalent of
> <base target="_blank"> into document that's loaded into ShellWindow. That
> way links would load in a new window, but window.location.href assignments
> would still try to load in the same window (and fail).

that is a good idea. Sorry I haven't landed this yet, I've been distracted by
other work, but I've now got it green on all platforms (except for a stubborn
windows compilation failure).

[commit-bot: I haz the power, 2012-10-05 10:16:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/10920084/41004

[commit-bot: I haz the power, 2012-10-05 12:42:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-05 16:43:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/10920084/36005

[commit-bot: I haz the power, 2012-10-05 17:38:27]

Retried try job too often for step(s) interactive_ui_tests