Reland of (Set)IsOverridingUserAgent should be called on VisibleItem

Reland of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset #9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for reland:
Fixed the bugs.

Original issues description:
Revert of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset #9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for revert:
Causes ToolsPopupMenuTestCase.testToolsMenuRequestDesktopNetwork to fail with error:
../../ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:109: error: -[ToolsPopupMenuTestCase testToolsMenuRequestDesktopNetwork] : Exception: NoMatchingElementException

Original issue's description:
> (Set)IsOverridingUserAgent should be called on VisibleItem
> 
> As explained in the associated bug, it always makes sense to call
> IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item
> instead of current item. Thus this CL changes CurrentItem to VisibleItem
> wherever necessary.
> 
> Besides, this CL also does the following refactoring to have 
> NavigationManagerImpl encapsulate VisibleItem for calling
> (Set)IsOverridingUserAgent() to provide one more layer of abstraction.
> 
> BUG=693794
> 
> Review-Url: https://codereview.chromium.org/2711683002
> Cr-Commit-Position: refs/heads/master@{#452570}
> Committed: https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc250bdf301fa85

TBR=eugenebut@chromium.org,kkhorimoto@chromium.org,liaoyuke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=693794
Review-Url: https://codereview.chromium.org/2711733007
Cr-Commit-Position: refs/heads/master@{#452713}
Committed: https://chromium.googlesource.com/chromium/src/+/55a8e3f7414855960a20135217d32de930466f3d

BUG=

Review-Url: https://codereview.chromium.org/2710593010
Cr-Commit-Position: refs/heads/master@{#452945}
Committed: https://chromium.googlesource.com/chromium/src/+/b8453e1f8d0bd721540c5d4741f3b75e782eaf1d

[liaoyuke, 2017-02-24 18:03:06]

Description was changed from

==========
Revert "Revert of (Set)IsOverridingUserAgent should be called on VisibleItem
(patchset #9 id:180001 of https://codereview.chromium.org/2711683002/ )"

This reverts commit 55a8e3f7414855960a20135217d32de930466f3d.

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

to

==========
Reland of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Original issues description:
Revert of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for revert:
Causes ToolsPopupMenuTestCase.testToolsMenuRequestDesktopNetwork to fail with
error:
../../ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:109: error:
-[ToolsPopupMenuTestCase testToolsMenuRequestDesktopNetwork] : Exception:
NoMatchingElementException

Original issue's description:
> (Set)IsOverridingUserAgent should be called on VisibleItem
> 
> As explained in the associated bug, it always makes sense to call
> IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item
> instead of current item. Thus this CL changes CurrentItem to VisibleItem
> wherever necessary.
> 
> Besides, this CL also does the following refactoring to have 
> NavigationManagerImpl encapsulate VisibleItem for calling
> (Set)IsOverridingUserAgent() to provide one more layer of abstraction.
> 
> BUG=693794
> 
> Review-Url: https://codereview.chromium.org/2711683002
> Cr-Commit-Position: refs/heads/master@{#452570}
> Committed:
https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc2...

TBR=eugenebut@chromium.org,kkhorimoto@chromium.org,liaoyuke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=693794
Review-Url: https://codereview.chromium.org/2711733007
Cr-Commit-Position: refs/heads/master@{#452713}
Committed:
https://chromium.googlesource.com/chromium/src/+/55a8e3f7414855960a20135217d3...

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

[liaoyuke, 2017-02-24 18:03:34]

Description was changed from

==========
Reland of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Original issues description:
Revert of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for revert:
Causes ToolsPopupMenuTestCase.testToolsMenuRequestDesktopNetwork to fail with
error:
../../ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:109: error:
-[ToolsPopupMenuTestCase testToolsMenuRequestDesktopNetwork] : Exception:
NoMatchingElementException

Original issue's description:
> (Set)IsOverridingUserAgent should be called on VisibleItem
> 
> As explained in the associated bug, it always makes sense to call
> IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item
> instead of current item. Thus this CL changes CurrentItem to VisibleItem
> wherever necessary.
> 
> Besides, this CL also does the following refactoring to have 
> NavigationManagerImpl encapsulate VisibleItem for calling
> (Set)IsOverridingUserAgent() to provide one more layer of abstraction.
> 
> BUG=693794
> 
> Review-Url: https://codereview.chromium.org/2711683002
> Cr-Commit-Position: refs/heads/master@{#452570}
> Committed:
https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc2...

TBR=eugenebut@chromium.org,kkhorimoto@chromium.org,liaoyuke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=693794
Review-Url: https://codereview.chromium.org/2711733007
Cr-Commit-Position: refs/heads/master@{#452713}
Committed:
https://chromium.googlesource.com/chromium/src/+/55a8e3f7414855960a20135217d3...

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

to

==========
Reland of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for reland:
Fixed the bugs.

Original issues description:
Revert of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for revert:
Causes ToolsPopupMenuTestCase.testToolsMenuRequestDesktopNetwork to fail with
error:
../../ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:109: error:
-[ToolsPopupMenuTestCase testToolsMenuRequestDesktopNetwork] : Exception:
NoMatchingElementException

Original issue's description:
> (Set)IsOverridingUserAgent should be called on VisibleItem
> 
> As explained in the associated bug, it always makes sense to call
> IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item
> instead of current item. Thus this CL changes CurrentItem to VisibleItem
> wherever necessary.
> 
> Besides, this CL also does the following refactoring to have 
> NavigationManagerImpl encapsulate VisibleItem for calling
> (Set)IsOverridingUserAgent() to provide one more layer of abstraction.
> 
> BUG=693794
> 
> Review-Url: https://codereview.chromium.org/2711683002
> Cr-Commit-Position: refs/heads/master@{#452570}
> Committed:
https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc2...

TBR=eugenebut@chromium.org,kkhorimoto@chromium.org,liaoyuke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=693794
Review-Url: https://codereview.chromium.org/2711733007
Cr-Commit-Position: refs/heads/master@{#452713}
Committed:
https://chromium.googlesource.com/chromium/src/+/55a8e3f7414855960a20135217d3...

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

[liaoyuke, 2017-02-24 18:06:01]

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

[liaoyuke, 2017-02-24 18:06:02]

Hey Eugene, Kurt,

PTAL.

Thank you very much!

[Eugene But (OOO till 7-30), 2017-02-24 18:26:32]

lgtm

[kkhorimoto, 2017-02-24 19:21:27]

https://codereview.chromium.org/2710593010/diff/40001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2710593010/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:463: @property(nonatomic, readonly)
BOOL usesDesktopUserAgent;
Again, I think that it would be better to fold this logic directly into
|-webViewWithConfiguration:|.  The concept of a "current item" is going away in
the refactor, so exposing a property whose semantics are tightly knit with that
API is a bad idea.  When future users of the NavigationManager API need that
functionality, they will need to implement it themselves (and add a comment
explaining why that logic is needed, rather than using an existing
NavigationItem getter). Exposing it in a property like this invites misuse, as
someone working on this file in the future may use it without fully thinking
through whether the flag from the "current item" is needed.

[Eugene But (OOO till 7-30), 2017-02-24 19:49:18]

https://codereview.chromium.org/2710593010/diff/40001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2710593010/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:463: @property(nonatomic, readonly)
BOOL usesDesktopUserAgent;
On 2017/02/24 19:21:27, kkhorimoto_ wrote:
> Again, I think that it would be better to fold this logic directly into
> |-webViewWithConfiguration:|.  The concept of a "current item" is going away
in
> the refactor, so exposing a property whose semantics are tightly knit with
that
> API is a bad idea.  When future users of the NavigationManager API need that
> functionality, they will need to implement it themselves (and add a comment
> explaining why that logic is needed, rather than using an existing
> NavigationItem getter). Exposing it in a property like this invites misuse, as
> someone working on this file in the future may use it without fully thinking
> through whether the flag from the "current item" is needed.
This is existing property which will be fully removed. There is no need to pass
self.usesDesktopUserAgent to BuildWKWebView, this logic is UIWebView cruft. With
UIWebView the only way to set UserAgent was changing NSUserDefaults before
UIWebView creation. WKWebView has API to set user agent, so there is no need to
recreate it when desktop UA is requested. I think we should leave this code as
it is because we will throw it away anyway.

[liaoyuke, 2017-02-24 20:01:07]

On 2017/02/24 19:49:18, Eugene But wrote:
>
https://codereview.chromium.org/2710593010/diff/40001/ios/web/web_state/ui/cr...
> File ios/web/web_state/ui/crw_web_controller.mm (right):
> 
>
https://codereview.chromium.org/2710593010/diff/40001/ios/web/web_state/ui/cr...
> ios/web/web_state/ui/crw_web_controller.mm:463: @property(nonatomic, readonly)
> BOOL usesDesktopUserAgent;
> On 2017/02/24 19:21:27, kkhorimoto_ wrote:
> > Again, I think that it would be better to fold this logic directly into
> > |-webViewWithConfiguration:|.  The concept of a "current item" is going away
> in
> > the refactor, so exposing a property whose semantics are tightly knit with
> that
> > API is a bad idea.  When future users of the NavigationManager API need that
> > functionality, they will need to implement it themselves (and add a comment
> > explaining why that logic is needed, rather than using an existing
> > NavigationItem getter). Exposing it in a property like this invites misuse,
as
> > someone working on this file in the future may use it without fully thinking
> > through whether the flag from the "current item" is needed.
> This is existing property which will be fully removed. There is no need to
pass
> self.usesDesktopUserAgent to BuildWKWebView, this logic is UIWebView cruft.
With
> UIWebView the only way to set UserAgent was changing NSUserDefaults before
> UIWebView creation. WKWebView has API to set user agent, so there is no need
to
> recreate it when desktop UA is requested. I think we should leave this code as
> it is because we will throw it away anyway.

Agree with Eugene's statement, and in my next CL, part of the work will be
refactoring
how user agent is set to WKWebView, which would get rid of the currentItem. What
do you think Kurt?

Thanks!

[kkhorimoto, 2017-02-24 21:06:47]

On 2017/02/24 20:01:07, liaoyuke wrote:
> On 2017/02/24 19:49:18, Eugene But wrote:
> >
>
https://codereview.chromium.org/2710593010/diff/40001/ios/web/web_state/ui/cr...
> > File ios/web/web_state/ui/crw_web_controller.mm (right):
> > 
> >
>
https://codereview.chromium.org/2710593010/diff/40001/ios/web/web_state/ui/cr...
> > ios/web/web_state/ui/crw_web_controller.mm:463: @property(nonatomic,
readonly)
> > BOOL usesDesktopUserAgent;
> > On 2017/02/24 19:21:27, kkhorimoto_ wrote:
> > > Again, I think that it would be better to fold this logic directly into
> > > |-webViewWithConfiguration:|.  The concept of a "current item" is going
away
> > in
> > > the refactor, so exposing a property whose semantics are tightly knit with
> > that
> > > API is a bad idea.  When future users of the NavigationManager API need
that
> > > functionality, they will need to implement it themselves (and add a
comment
> > > explaining why that logic is needed, rather than using an existing
> > > NavigationItem getter). Exposing it in a property like this invites
misuse,
> as
> > > someone working on this file in the future may use it without fully
thinking
> > > through whether the flag from the "current item" is needed.
> > This is existing property which will be fully removed. There is no need to
> pass
> > self.usesDesktopUserAgent to BuildWKWebView, this logic is UIWebView cruft.
> With
> > UIWebView the only way to set UserAgent was changing NSUserDefaults before
> > UIWebView creation. WKWebView has API to set user agent, so there is no need
> to
> > recreate it when desktop UA is requested. I think we should leave this code
as
> > it is because we will throw it away anyway.
> 
> Agree with Eugene's statement, and in my next CL, part of the work will be
> refactoring
> how user agent is set to WKWebView, which would get rid of the currentItem.
What
> do you think Kurt?
> 
> Thanks!

Oh whoops, I mistakenly thought that we still needed to recreate the web view
for WKWebView.  In that case, i'm fine with leaving the property as is, since
it'll be removed in one of your later CLs.  lgtm!

[liaoyuke, 2017-02-24 22:00:07]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 22:00:35]

Dry run: 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-02-24 22:00:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 22:00:49]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-02-24 22:02:10]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 22:03:26]

Dry run: 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-02-24 22:03:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 22:03:58]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-02-24 22:04:19]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-02-24 22:05:14]

The CQ bit was unchecked by liaoyuke@chromium.org

[liaoyuke, 2017-02-24 22:05:15]

The CQ bit was checked by liaoyuke@chromium.org

[commit-bot: I haz the power, 2017-02-24 22:05:41]

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-02-24 22:08:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487973915850370,
"parent_rev": "b693f94a81551da0334f9fb297813e617d9539c0", "commit_rev":
"b8453e1f8d0bd721540c5d4741f3b75e782eaf1d"}

[commit-bot: I haz the power, 2017-02-24 22:09:28]

Description was changed from

==========
Reland of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for reland:
Fixed the bugs.

Original issues description:
Revert of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for revert:
Causes ToolsPopupMenuTestCase.testToolsMenuRequestDesktopNetwork to fail with
error:
../../ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:109: error:
-[ToolsPopupMenuTestCase testToolsMenuRequestDesktopNetwork] : Exception:
NoMatchingElementException

Original issue's description:
> (Set)IsOverridingUserAgent should be called on VisibleItem
> 
> As explained in the associated bug, it always makes sense to call
> IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item
> instead of current item. Thus this CL changes CurrentItem to VisibleItem
> wherever necessary.
> 
> Besides, this CL also does the following refactoring to have 
> NavigationManagerImpl encapsulate VisibleItem for calling
> (Set)IsOverridingUserAgent() to provide one more layer of abstraction.
> 
> BUG=693794
> 
> Review-Url: https://codereview.chromium.org/2711683002
> Cr-Commit-Position: refs/heads/master@{#452570}
> Committed:
https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc2...

TBR=eugenebut@chromium.org,kkhorimoto@chromium.org,liaoyuke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=693794
Review-Url: https://codereview.chromium.org/2711733007
Cr-Commit-Position: refs/heads/master@{#452713}
Committed:
https://chromium.googlesource.com/chromium/src/+/55a8e3f7414855960a20135217d3...

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

to

==========
Reland of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for reland:
Fixed the bugs.

Original issues description:
Revert of (Set)IsOverridingUserAgent should be called on VisibleItem (patchset
#9 id:180001 of https://codereview.chromium.org/2711683002/ )

Reason for revert:
Causes ToolsPopupMenuTestCase.testToolsMenuRequestDesktopNetwork to fail with
error:
../../ios/chrome/browser/ui/tools_menu/tools_popup_menu_egtest.mm:109: error:
-[ToolsPopupMenuTestCase testToolsMenuRequestDesktopNetwork] : Exception:
NoMatchingElementException

Original issue's description:
> (Set)IsOverridingUserAgent should be called on VisibleItem
> 
> As explained in the associated bug, it always makes sense to call
> IsOverridingUserAgent() and SetIsOverridingUserAgent() on visible item
> instead of current item. Thus this CL changes CurrentItem to VisibleItem
> wherever necessary.
> 
> Besides, this CL also does the following refactoring to have 
> NavigationManagerImpl encapsulate VisibleItem for calling
> (Set)IsOverridingUserAgent() to provide one more layer of abstraction.
> 
> BUG=693794
> 
> Review-Url: https://codereview.chromium.org/2711683002
> Cr-Commit-Position: refs/heads/master@{#452570}
> Committed:
https://chromium.googlesource.com/chromium/src/+/21359189a7883d38a531e8681cc2...

TBR=eugenebut@chromium.org,kkhorimoto@chromium.org,liaoyuke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=693794
Review-Url: https://codereview.chromium.org/2711733007
Cr-Commit-Position: refs/heads/master@{#452713}
Committed:
https://chromium.googlesource.com/chromium/src/+/55a8e3f7414855960a20135217d3...

BUG=

Review-Url: https://codereview.chromium.org/2710593010
Cr-Commit-Position: refs/heads/master@{#452945}
Committed:
https://chromium.googlesource.com/chromium/src/+/b8453e1f8d0bd721540c5d4741f3...
==========

[commit-bot: I haz the power, 2017-02-24 22:09:29]

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