Show origin for hosted apps that navigate away from their start origin.

Show origin for hosted apps that navigate away from their start origin.

This is only for hosted apps that run in their own window.

BUG=467844
Committed: https://crrev.com/11bd6ed1f622cd9421ed4cdb53454e08d689e342
Cr-Commit-Position: refs/heads/master@{#333009}

[benwells, 2015-06-03 07:51:06]

benwells@chromium.org changed reviewers:
+ felt@chromium.org

[felt, 2015-06-04 19:21:34]

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:33: www +
app_url.host() == page_url.host()) &&
why single out www as the one allowable subdomain? if you want to support
subdomains, should it be all subdomains?

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:63: extension &&
extension->from_bookmark() &&
should this be a DCHECK(extension)? can extension ever be a nullptr here?

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right):

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:206: 
maybe check the subdomain properties you care about too

[benwells, 2015-06-04 23:54:42]

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:33: www +
app_url.host() == page_url.host()) &&
On 2015/06/04 19:21:34, felt wrote:
> why single out www as the one allowable subdomain? if you want to support
> subdomains, should it be all subdomains?

It shoudn't be all subdomains as benfredwells.github.io is different to
adrifelt.github.io.

I did www. as it is so common to redirect from a bare host name to www.hostname,
and the first hosted app I tested on did this :/

We could not bother with this and 'break' those hosted apps that do it (where
break == end up showing the location bar).

[felt, 2015-06-04 23:58:37]

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:33: www +
app_url.host() == page_url.host()) &&
On 2015/06/04 23:54:42, benwells wrote:
> On 2015/06/04 19:21:34, felt wrote:
> > why single out www as the one allowable subdomain? if you want to support
> > subdomains, should it be all subdomains?
> 
> It shoudn't be all subdomains as benfredwells.github.io is different to
> adrifelt.github.io.
> 
> I did www. as it is so common to redirect from a bare host name to
http://www.hostname,
> and the first hosted app I tested on did this :/
> 
> We could not bother with this and 'break' those hosted apps that do it (where
> break == end up showing the location bar).

ok, special casing www sounds reasonable to me. (although i'm puzzled about how
someone could end up bookmarking https://hostname when it always immediately
redirects to https://www.hostname)

[benwells, 2015-06-05 00:06:39]

On 2015/06/04 23:58:37, felt wrote:
>
https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
> File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):
> 
>
https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
> chrome/browser/ui/extensions/hosted_app_browser_controller.cc:33: www +
> app_url.host() == page_url.host()) &&
> On 2015/06/04 23:54:42, benwells wrote:
> > On 2015/06/04 19:21:34, felt wrote:
> > > why single out www as the one allowable subdomain? if you want to support
> > > subdomains, should it be all subdomains?
> > 
> > It shoudn't be all subdomains as benfredwells.github.io is different to
> > adrifelt.github.io.
> > 
> > I did www. as it is so common to redirect from a bare host name to
> http://www.hostname,
> > and the first hosted app I tested on did this :/
> > 
> > We could not bother with this and 'break' those hosted apps that do it
(where
> > break == end up showing the location bar).
> 
> ok, special casing www sounds reasonable to me. (although i'm puzzled about
how
> someone could end up bookmarking https://hostname when it always immediately
> redirects to https://www.hostname)

When it was just for bookmark apps it wasn;t a problem. Now that it is being
extended to hosted apps, where people manually type in a start url, it is.

[benwells, 2015-06-05 01:20:44]

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:63: extension &&
extension->from_bookmark() &&
On 2015/06/04 19:21:33, felt wrote:
> should this be a DCHECK(extension)? can extension ever be a nullptr here?

Done.

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right):

https://codereview.chromium.org/1164873003/diff/1/chrome/browser/ui/extension...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:206: 
On 2015/06/04 19:21:34, felt wrote:
> maybe check the subdomain properties you care about too

Done.

[felt, 2015-06-05 01:30:52]

lgtm

[benwells, 2015-06-05 01:35:18]

benwells@chromium.org changed reviewers:
+ thestig@chromium.org

[benwells, 2015-06-05 01:35:19]

+thestig for chrome/browser/ui/browser* stuff

[Lei Zhang, 2015-06-05 01:51:50]

lgtm

The rest are all nits.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:58:
should_use_web_app_frame_(false) {
You don't really need this since you always initialize it inside the ctor
anyway.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:64: extension &&
extension->from_bookmark() &&
No need to check for |extension| since you already DCHECKed above. If it fails,
we'd like to know about it by letting it crash.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right):

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:174:
test_data_dir_.AppendASCII("app/"), 1, extensions::Manifest::INTERNAL);
Do you need the trailing path separate in "app/" ? I realize it's copy+pasted.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:185: Browser* app_browser
= NULL;
nit: nullptr in new code.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:189: if (*it ==
browser()) {
You can check this first before setting app_id.

if (*it == browser()) {
  continue;
}
std::string app_id = ...
if (app_id == hosted_app->id()) {
  ...
}

I also realize this block has been copy+pasted many times. Refactor into its own
method?

[benwells, 2015-06-05 03:19:47]

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/hosted_app_browser_controller.cc (right):

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:58:
should_use_web_app_frame_(false) {
On 2015/06/05 01:51:50, Lei Zhang wrote:
> You don't really need this since you always initialize it inside the ctor
> anyway.

Done.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browser_controller.cc:64: extension &&
extension->from_bookmark() &&
On 2015/06/05 01:51:50, Lei Zhang wrote:
> No need to check for |extension| since you already DCHECKed above. If it
fails,
> we'd like to know about it by letting it crash.

Done.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right):

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:174:
test_data_dir_.AppendASCII("app/"), 1, extensions::Manifest::INTERNAL);
On 2015/06/05 01:51:50, Lei Zhang wrote:
> Do you need the trailing path separate in "app/" ? I realize it's copy+pasted.

Done.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:185: Browser* app_browser
= NULL;
On 2015/06/05 01:51:50, Lei Zhang wrote:
> nit: nullptr in new code.

Done.

https://codereview.chromium.org/1164873003/diff/20001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:189: if (*it ==
browser()) {
On 2015/06/05 01:51:50, Lei Zhang wrote:
> You can check this first before setting app_id.
> 
> if (*it == browser()) {
>   continue;
> }
> std::string app_id = ...
> if (app_id == hosted_app->id()) {
>   ...
> }
> 
> I also realize this block has been copy+pasted many times. Refactor into its
own
> method?

Done.

[Lei Zhang, 2015-06-05 03:25:04]

lgtm++

[benwells, 2015-06-05 03:52:08]

The CQ bit was checked by benwells@chromium.org

[benwells, 2015-06-05 03:52:08]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org
Link to the patchset: https://codereview.chromium.org/1164873003/#ps60001
(title: "feedback for the stig")

[commit-bot: I haz the power, 2015-06-05 03:52:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164873003/60001

[commit-bot: I haz the power, 2015-06-05 04:20:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-05 04:21:37]

Patchset 4 (id:??) landed as
https://crrev.com/11bd6ed1f622cd9421ed4cdb53454e08d689e342
Cr-Commit-Position: refs/heads/master@{#333009}