Test URL instead of VirtualURL in [BVC isTabNativePage]

Test URL instead of VirtualURL in [BVC isTabNativePage]

The "native" character is linked to the loaded URL and not to the
displayed URL. Test the URL instead of the VirtualURL.

BUG=696628
Review-Url: https://codereview.chromium.org/2723683002
Cr-Commit-Position: refs/heads/master@{#453933}
Committed: https://chromium.googlesource.com/chromium/src/+/889af53fad72676272dd249f039a12aec8845f8c

[Olivier, 2017-02-28 08:54:56]

Description was changed from

==========
Test URL instead of VirtualURL in [BVC isTabNativePage]

The "native" character is linked to the loaded URL and not to the
displayed URL. Test the URL instead of the VirtualURL.

BUG=
==========

to

==========
Test URL instead of VirtualURL in [BVC isTabNativePage]

The "native" character is linked to the loaded URL and not to the
displayed URL. Test the URL instead of the VirtualURL.

BUG=696628
==========

[Olivier, 2017-02-28 08:54:56]

olivierrobin@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Olivier, 2017-02-28 08:57:10]

PTAL.

This change slightly the URL considered, moving from CurrentURL to VisibleURL.
Do you think this is acceptable?
I don't know how to access CurrentURL with public API without duplicating the
logic in [CRWSessionController currentEntry]

[Eugene But (OOO till 7-30), 2017-02-28 16:32:59]

eugenebut@chromium.org changed reviewers:
+ kkhorimoto@chromium.org

[Eugene But (OOO till 7-30), 2017-02-28 16:33:00]

I think using URL instead of VirtualURL should be fine for NTP. Also the fact
that you stopped using Tab in BVC is great. lgtm, but it will not hurt if Kurt
takes a look.

https://codereview.chromium.org/2723683002/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2723683002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:2057: return tab &&
tab.webState && tab.webState->GetNavigationManager() &&
|tab && | check is redundant, because if tab is nil, then |tab.webState| will be
nil.

https://codereview.chromium.org/2723683002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:2057: return tab &&
tab.webState && tab.webState->GetNavigationManager() &&
Optional nit: How about using local variables and early returns?:

web::WebState* webState = tab.webState;
if (!webState)
  return NO;
web::NavigationManager* navigationManager = webState->GetNavigationManager();
if (!navigationManager)
  return NO;
web::NavigationItem* visibleItem = navigationManager->GetVisibleItem();
if (!visibleItem)
  return NO;
return visibleItem->GetURL().SchemeIs(kChromeUIScheme);

[kkhorimoto, 2017-02-28 18:50:29]

lgtm with a question: Is reader mode the only case where the URL is chrome:
scheme but the VirtualURL is http?

https://codereview.chromium.org/2723683002/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2723683002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:2057: return tab &&
tab.webState && tab.webState->GetNavigationManager() &&
On 2017/02/28 16:33:00, Eugene But wrote:
> |tab && | check is redundant, because if tab is nil, then |tab.webState| will
be
> nil.

Can we use GetWebClient()->IsAppSpecificURL() here?  It's checking against the
same scheme, but it'd be better not to duplicate code, especially since that
function is used as the source-of-truth of native vs. web pages from within the
web// layer.

[Olivier, 2017-03-01 14:46:14]

I think this case only happens for Reading List offline, as Virtual URL are only
used in this case.

https://codereview.chromium.org/2723683002/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2723683002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:2057: return tab &&
tab.webState && tab.webState->GetNavigationManager() &&
On 2017/02/28 18:50:29, kkhorimoto_ wrote:
> On 2017/02/28 16:33:00, Eugene But wrote:
> > |tab && | check is redundant, because if tab is nil, then |tab.webState|
will
> be
> > nil.
> 
> Can we use GetWebClient()->IsAppSpecificURL() here?  It's checking against the
> same scheme, but it'd be better not to duplicate code, especially since that
> function is used as the source-of-truth of native vs. web pages from within
the
> web// layer.

Done.

[Olivier, 2017-03-01 14:46:20]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-03-01 14:46:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2723683002/#ps40001
(title: "comment")

[commit-bot: I haz the power, 2017-03-01 14:46:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 14:56:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488379580788230,
"parent_rev": "8b8b5e3692189301795e9fb21fec7f371f19b7f0", "commit_rev":
"889af53fad72676272dd249f039a12aec8845f8c"}

[commit-bot: I haz the power, 2017-03-01 14:57:10]

Description was changed from

==========
Test URL instead of VirtualURL in [BVC isTabNativePage]

The "native" character is linked to the loaded URL and not to the
displayed URL. Test the URL instead of the VirtualURL.

BUG=696628
==========

to

==========
Test URL instead of VirtualURL in [BVC isTabNativePage]

The "native" character is linked to the loaded URL and not to the
displayed URL. Test the URL instead of the VirtualURL.

BUG=696628

Review-Url: https://codereview.chromium.org/2723683002
Cr-Commit-Position: refs/heads/master@{#453933}
Committed:
https://chromium.googlesource.com/chromium/src/+/889af53fad72676272dd249f039a...
==========

[commit-bot: I haz the power, 2017-03-01 14:57:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/889af53fad72676272dd249f039a...