(Set)IsOverridingUserAgent should be called on VisibleItem

ios/web/navigation/navigation_manager_impl.mm

Index: ios/web/navigation/navigation_manager_impl.mm
diff --git a/ios/web/navigation/navigation_manager_impl.mm b/ios/web/navigation/navigation_manager_impl.mm
index 140c760468abf0f45fe5fdaa6d512dbb8b08ed32..3fe9e20b19b3141203d251f97fca1f9b86bb7ef0 100644
--- a/ios/web/navigation/navigation_manager_impl.mm
+++ b/ios/web/navigation/navigation_manager_impl.mm
@@ -420,6 +420,10 @@ bool AreURLsInPageNavigation(const GURL& existing_url, const GURL& new_url) {
   return result;
 }
 
+bool NavigationManagerImpl::IsUsingDesktopUserAgent() const {
+  return GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent();

[Eugene But (OOO till 7-30), 2017/02/22 21:57:42]

Why Visible? Should this be LastCommittedItem? According to the comments this
represents "currently loaded page", which is LastCommittedItem, not visible
item.

[kkhorimoto, 2017/02/22 22:54:15]

When reviewing, I was looking at the places this is used, which is in the BVC to
enable/disable the desktop UA option.  If there's a user-initiated navigation
happening and the user taps on the "use desktop user agent" button, then
presumably, they wanted to load the page they typed out using the desktop UA,
which may be the visible item if it's taking a long time to load.  Maybe to
solve this problem, BVC should be manually inspecting the visible item rather
than writing logic here that's used specifically for the BVC's purposes.

[Eugene But (OOO till 7-30), 2017/02/22 23:13:56]

Makes sense. Yuke, please update header comments to reflect what method does.

[liaoyuke, 2017/02/22 23:48:31]

Done.

[liaoyuke, 2017/02/22 23:48:31]

Acknowledged.

[liaoyuke, 2017/02/22 23:48:31]

Isn't it the case that CRWWebController also uses this method to decide how to
build the web view? 

Given that user agent status should always be queried on visible item, I
actually think it makes sense to wrap this method inside NavigationManagerImpl,
otherwise, every time someone wants to get the user agent status, one has to
think whether the caller should query the last committedItem or currentItem or
visibleItem, which is quite error prone.

[Eugene But (OOO till 7-30), 2017/02/23 00:03:13]

So this statement made me rethink this approach. Querying
|IsOverridingUserAgent| on NavigationItem is actually not error prone, because
the caller can clearly tell if they query the flag on LastCommitted, Pending or
Visible Item. Querying |IsOverridingUserAgent| on NavigationManager is error
prone, because people would assume that this code "just does the right thing",
but I can imagine cases when callers actually need to query UA flag on current
page, not on pending page. How about removing |IsUsingDesktopUserAgent| from
navigation manager, so clients have to explicitly specify which item they need.

+}
+
 void NavigationManagerImpl::OverrideDesktopUserAgentForNextPendingItem() {
   if (GetPendingItem())
     GetPendingItem()->SetIsOverridingUserAgent(true);