New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 734772 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug
Proj-Servicification

Blocking:
issue 368813



Sign in to add a comment

"Request desktop site" state dropped under PlzNavigate navigation

Project Member Reported by aelias@chromium.org, Jun 19 2017

Issue description

Repro versions: 61.0.3135.0, 60.0.3112.36, with --enable-browser-side-navigation (no repro when disabled)

1) Visit http://en.wikipedia.org/ on a phone.
2) Select "Request Desktop Site" in the kebab menu.  Observe page reloads to desktop Wikipedia as expected.
3) Tap on any link on the page.  This page loads as the mobile site.  In the kebab menu, "Request Desktop Site" is no longer checked.  (Expected behavior: RDS flag persists on the navigation subtree after it's set, and is only cleared by pressing back button from the original URL it was set for.)

Note that this flag is set on NavigationEntry as "user_agent_override".

 

Comment 1 by creis@chromium.org, Jun 19 2017

Labels: Proj-PlzNavigate-Blocking
Adding Proj-PlzNavigate-Blocking given the ReleaseBlock-Stable label.

Comment 2 by aelias@chromium.org, Jun 19 2017

Owner: jinsuk...@chromium.org
Status: Assigned (was: Available)
Maybe a good task for Jinsuk since this is an Android-specific PlzNavigate problem?

navigation_request.cc:277:

  // TODO(clamy): See how we should handle override of the user agent when the
  // navigation may start in a renderer and commit in another one.
On the first look, I think the flag (user agent override) can be copied over from the last committed navigation entry (not from the pending entry which is null) to make it persist in the renderer-initiated navigation. 

Comment 5 by nasko@chromium.org, Jun 20 2017

jam@ fixed at least one bug with Request Desktop Site and PlzNavigate in  issue 705218 . Is this a new regression or has it been like that for a while?
 Issue 705218  fixed the bug in the description but this is about another bug - RDS setting doesn't stick. Click a new link, and RDS setting gets lost. 
Status: Started (was: Assigned)

Comment 8 by jam@chromium.org, Jun 22 2017

Cc: anan...@chromium.org bustamante@chromium.org gov...@chromium.org
 Issue 734974  has been merged into this issue.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 26 2017

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

commit c7616c7db94822e05489004042923b0df02882ed
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Mon Jun 26 17:48:37 2017

Set UA override flag for Renderer-initiated NavigationRequest

Use last committed NavigationEntry instead of the pending endtry
(which can be null) to have the user agent override flag stick
across navigations. The entry can affect navigation entry id
but it is safe to replace it since the id used for browser-initiated
navigation only.

BUG= 734772 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Icb34f55af82bafde5a3c0e1457fa25c1172db9d0
Reviewed-on: https://chromium-review.googlesource.com/541277
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482311}
[modify] https://crrev.com/c7616c7db94822e05489004042923b0df02882ed/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/c7616c7db94822e05489004042923b0df02882ed/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/c7616c7db94822e05489004042923b0df02882ed/content/browser/frame_host/navigator_impl.cc

Status: Fixed (was: Started)

Comment 11 by jam@chromium.org, Jun 28 2017

Labels: -M-61 M-60

Comment 12 by jam@chromium.org, Jun 28 2017

Labels: Merge-Request-60
Project Member

Comment 13 by sheriffbot@chromium.org, Jun 28 2017

Labels: -Merge-Request-60 Hotlist-Merge-Review Merge-Review-60
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 14 by bugdroid1@chromium.org, Jun 28 2017

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

commit 8520d829abf95ddfd04171323373422ebe57a4d2
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Wed Jun 28 21:03:54 2017

PlzNavigate: Add a test for user agent overriding

This CL adss a browser test that ensures the user setting
'Request Desktop Site' is preserved when navigated to next
entry via a link click.

BUG= 734772 

Change-Id: Ic015352ee7f97d7ccb3b479010087a69bcf8c006
Reviewed-on: https://chromium-review.googlesource.com/552645
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483137}
[modify] https://crrev.com/8520d829abf95ddfd04171323373422ebe57a4d2/chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java

Labels: -Merge-Review-60 Merge-Approved-60
Approved for M60 branch 3112.
Project Member

Comment 16 by bugdroid1@chromium.org, Jun 29 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/790356ce6c03c372d5fe2310f4e319265441f231

commit 790356ce6c03c372d5fe2310f4e319265441f231
Author: John Abd-El-Malek <jam@chromium.org>
Date: Thu Jun 29 23:17:55 2017

Set UA override flag for Renderer-initiated NavigationRequest

Use last committed NavigationEntry instead of the pending endtry
(which can be null) to have the user agent override flag stick
across navigations. The entry can affect navigation entry id
but it is safe to replace it since the id used for browser-initiated
navigation only.

BUG= 734772 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Icb34f55af82bafde5a3c0e1457fa25c1172db9d0
Reviewed-on: https://chromium-review.googlesource.com/541277
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#482311}
Review-Url: https://codereview.chromium.org/2966573003 .
Cr-Commit-Position: refs/branch-heads/3112@{#496}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/790356ce6c03c372d5fe2310f4e319265441f231/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/790356ce6c03c372d5fe2310f4e319265441f231/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/790356ce6c03c372d5fe2310f4e319265441f231/content/browser/frame_host/navigator_impl.cc

Components: Internals>Network>Service
Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.

Sign in to add a comment