Show location for bookmark apps that have navigated across origin.

Show location for bookmark apps that have navigated across origin.

It is important that this is shown for security reasons.

BUG=463405
Committed: https://crrev.com/919e79baf8bfcff29f44b8f9bd6ffa468d9f700d
Cr-Commit-Position: refs/heads/master@{#321297}

[benwells, 2015-03-17 06:36:06]

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

[Lei Zhang, 2015-03-17 07:19:55]

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

[Lei Zhang, 2015-03-17 07:19:56]

+meacer since this involves extensions + security.

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_ui_util.cc (right):

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.cc:73:
web_contents->GetURL().GetOrigin() != launch_url.GetOrigin();
GetURL() has a big "DO NOT USE" comment.

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_ui_util.h (right):

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.h:51: // viewed in the context of
the app represented by |extension|..
nit: extra period at the end.

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.h:53: content::WebContents*
web_contents);
this can probably be const as well.

[benwells, 2015-03-17 10:28:12]

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

[benwells, 2015-03-17 10:28:13]

+felt for security as she did the security review for this
meacer -> cc (but feel free to review if you want)

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_ui_util.cc (right):

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.cc:73:
web_contents->GetURL().GetOrigin() != launch_url.GetOrigin();
On 2015/03/17 07:19:56, Lei Zhang wrote:
> GetURL() has a big "DO NOT USE" comment.

So it does... Replaced with visible URL as that is what the users will see in
the location bar that is being shown to them.

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_ui_util.h (right):

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.h:51: // viewed in the context of
the app represented by |extension|..
On 2015/03/17 07:19:56, Lei Zhang wrote:
> nit: extra period at the end.

Done.

https://codereview.chromium.org/1006273002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.h:53: content::WebContents*
web_contents);
On 2015/03/17 07:19:56, Lei Zhang wrote:
> this can probably be const as well.

Done.

[felt, 2015-03-17 14:52:54]

https://codereview.chromium.org/1006273002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_ui_util.cc (right):

https://codereview.chromium.org/1006273002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.cc:69: return false;
Is it possible for either |extension| or |web_contents| to ever be null here in
normal browsing operations? If they should always have a value, would DCHECK be
better here?

https://codereview.chromium.org/1006273002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.cc:73:
web_contents->GetVisibleURL().GetOrigin() != launch_url.GetOrigin();
I think you might want to return true if either GetVisibleURL or
GetLastCommittedURL are different from the original origin.

[felt, 2015-03-17 14:54:21]

Note that this will show the origin if the user navigates between http and https
for the same hostname. That seems fine for simplicity's sake, but I wanted to
mention it. It seems to me like if the user goes from http->https on the same
hostname there's no reason to show the origin then.

[benwells, 2015-03-18 08:25:37]

Addressed feedback, added a test, and changed logic to keep the bar hidden if
this the page is just the https version of the app's origin.

https://codereview.chromium.org/1006273002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_ui_util.cc (right):

https://codereview.chromium.org/1006273002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.cc:69: return false;
On 2015/03/17 14:52:54, felt wrote:
> Is it possible for either |extension| or |web_contents| to ever be null here
in
> normal browsing operations? If they should always have a value, would DCHECK
be
> better here?

The extension is null when the window is for the devtools. The web_contents
returned by tab_strip_model_->GetActiveWebContents looks like it can be null
(based on the tab_strip_model.h comments and other usage). To be honest I'm not
sure when it can be null.

https://codereview.chromium.org/1006273002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_ui_util.cc:73:
web_contents->GetVisibleURL().GetOrigin() != launch_url.GetOrigin();
On 2015/03/17 14:52:54, felt wrote:
> I think you might want to return true if either GetVisibleURL or
> GetLastCommittedURL are different from the original origin.

Done.

[felt, 2015-03-18 14:42:03]

Thanks for adding a test. lgtm from security perspective

https://codereview.chromium.org/1006273002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_ui_util.cc (right):

https://codereview.chromium.org/1006273002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_ui_util.cc:79: GURL launch_url =
AppLaunchInfo::GetLaunchWebURL(extension);
Since launch_url isn't being used until after this if-statement, can you move it
down?

[benwells, 2015-03-19 01:23:43]

thestig - ready for owners review

https://codereview.chromium.org/1006273002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_ui_util.cc (right):

https://codereview.chromium.org/1006273002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_ui_util.cc:79: GURL launch_url =
AppLaunchInfo::GetLaunchWebURL(extension);
On 2015/03/18 14:42:03, felt wrote:
> Since launch_url isn't being used until after this if-statement, can you move
it
> down?

Done.

[Lei Zhang, 2015-03-19 01:31:25]

lgtm

https://codereview.chromium.org/1006273002/diff/140001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1006273002/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser_browsertest.cc:1437: } else if
(web_app::GetExtensionIdFromApplicationName((*it)->app_name()) ==
Just call GetExtensionIdFromApplicationName() once and check the result?

[benwells, 2015-03-19 03:51:22]

https://codereview.chromium.org/1006273002/diff/140001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1006273002/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser_browsertest.cc:1437: } else if
(web_app::GetExtensionIdFromApplicationName((*it)->app_name()) ==
On 2015/03/19 01:31:25, Lei Zhang wrote:
> Just call GetExtensionIdFromApplicationName() once and check the result?

Done.

[benwells, 2015-03-19 03:51:25]

The CQ bit was checked by benwells@chromium.org

[benwells, 2015-03-19 03:51:25]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1006273002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 03:51:49]

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

[commit-bot: I haz the power, 2015-03-19 04:58:33]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-19 05:00:13]

Patchset 9 (id:??) landed as
https://crrev.com/919e79baf8bfcff29f44b8f9bd6ffa468d9f700d
Cr-Commit-Position: refs/heads/master@{#321297}