New issue
Advanced search Search tips

Issue 693794 link

Starred by 1 user
Status: Fixed
Owner:
liaoyuke@chromium.org
Closed: Feb 2017
Cc:
eugene...@chromium.org
kkhorimoto@chromium.org
ios-bugs-priority@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug

Blocking:
issue 678047



Sign in to add a comment

IsOverridingUserAgent() and SetIsOverridingUserAgent() should be called on visibleItem instead of currentItem

Project Member Reported by liaoyuke@chromium.org, Feb 18 2017 
First of all, currentItem is too ambiguous, we should avoid using it.

Next, calling those two functions on currentItem is actually a bug. imagine when a user clicks on "Request Desktop Site" while the page is still loading.

1. If the pending page is user initiated, then the user's intention is to request the desktop version of the pending page.

2. otherwise the pending page is renderer initiated, then the user's intention would be requesting desktop version of the last committed page instead of the pending page.

If we look at the definition of currentItem and visibleItem attached bellow, it's easy to see that visibleItem satisfies the above mentioned two rules perfectly, while currentItem doesn't.

- (CRWSessionEntry*)currentEntry {
  if (_transientEntry)
    return _transientEntry.get();
  if (_pendingEntry)
    return _pendingEntry.get();
  return [self lastCommittedEntry];
}

- (CRWSessionEntry*)visibleEntry {
  if (_transientEntry)
    return _transientEntry.get();
  // Only return the pending_entry for new (non-history), browser-initiated
  // navigations in order to prevent URL spoof attacks.
  web::NavigationItemImpl* pendingItem = [_pendingEntry navigationItemImpl];

  if (pendingItem) {
    bool isUserInitiated = pendingItem->NavigationInitiationType() ==
                           web::NavigationInitiationType::USER_INITIATED;
    bool safeToShowPending = isUserInitiated && _pendingItemIndex == -1;

    if (safeToShowPending)
      return _pendingEntry.get();
  }

  return [self lastCommittedEntry];
}

Comment 1 by liaoyuke@chromium.org, Feb 18 2017

Labels: OS-iOS

Comment 2 by khorimoto@chromium.org, Feb 18 2017

Cc: -khorimoto@chromium.org kkhorimoto@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21359189a7883d38a531e8681cc250bdf301fa85

commit 21359189a7883d38a531e8681cc250bdf301fa85
Author: liaoyuke <liaoyuke@chromium.org>
Date: Thu Feb 23 19:00:06 2017

(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}

[modify] https://crrev.com/21359189a7883d38a531e8681cc250bdf301fa85/ios/chrome/browser/tabs/tab.h
[modify] https://crrev.com/21359189a7883d38a531e8681cc250bdf301fa85/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/21359189a7883d38a531e8681cc250bdf301fa85/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/21359189a7883d38a531e8681cc250bdf301fa85/ios/web/navigation/crw_session_controller.mm
[modify] https://crrev.com/21359189a7883d38a531e8681cc250bdf301fa85/ios/web/web_state/ui/crw_web_controller.mm

Comment 4 by liaoyuke@chromium.org, Feb 23 2017

Status: Fixed (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 24 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55a8e3f7414855960a20135217d32de930466f3d

commit 55a8e3f7414855960a20135217d32de930466f3d
Author: gchatz <gchatz@chromium.org>
Date: Fri Feb 24 01:54:01 2017

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}

[modify] https://crrev.com/55a8e3f7414855960a20135217d32de930466f3d/ios/chrome/browser/tabs/tab.h
[modify] https://crrev.com/55a8e3f7414855960a20135217d32de930466f3d/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/55a8e3f7414855960a20135217d32de930466f3d/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/55a8e3f7414855960a20135217d32de930466f3d/ios/web/navigation/crw_session_controller.mm
[modify] https://crrev.com/55a8e3f7414855960a20135217d32de930466f3d/ios/web/web_state/ui/crw_web_controller.mm

Comment 6 by kkhorimoto@chromium.org, Feb 24 2017

Status: Assigned (was: Fixed)
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 24 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8453e1f8d0bd721540c5d4741f3b75e782eaf1d

commit b8453e1f8d0bd721540c5d4741f3b75e782eaf1d
Author: liaoyuke <liaoyuke@chromium.org>
Date: Fri Feb 24 22:08:44 2017

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}

[modify] https://crrev.com/b8453e1f8d0bd721540c5d4741f3b75e782eaf1d/ios/chrome/browser/tabs/tab.h
[modify] https://crrev.com/b8453e1f8d0bd721540c5d4741f3b75e782eaf1d/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/b8453e1f8d0bd721540c5d4741f3b75e782eaf1d/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/b8453e1f8d0bd721540c5d4741f3b75e782eaf1d/ios/web/navigation/crw_session_controller.mm
[modify] https://crrev.com/b8453e1f8d0bd721540c5d4741f3b75e782eaf1d/ios/web/web_state/ui/crw_web_controller.mm

Comment 8 by liaoyuke@chromium.org, Feb 24 2017

Status: Fixed (was: Assigned)
It turns out that in some cases, such as GoBack, we may need to query user agent flag on pending item instead of visible item.
►

Sign in to add a comment