Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2619063003:
Update viewport_meta setting on any navigation. (Closed)
|
Created:
3 years, 11 months ago by aelias_OOO_until_Jul13 Modified:
3 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate viewport_meta setting on any navigation.
In http://crrev.com/1785953002 I tied a setting to the
is_overriding_user_agent property of NavigationEntry, but I forgot to
keep the preference updated during arbitrary navigation entry changes
(e.g. back button).
BUG=671580
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move to RendererDidNavigateToExistingPage #
Total comments: 2
Patch Set 3 : Reupload patch set 1 #
Total comments: 2
Patch Set 4 : Add unit test #Patch Set 5 : Bring back pending_entry_ null check #
Total comments: 4
Patch Set 6 : EXPECT_EQ -> EXPECT_TRUE/FALSE #Patch Set 7 : Change test to test GoBack() #Patch Set 8 : Another EXPECT_EQ -> EXPECT_FALSE #
Messages
Total messages: 53 (32 generated)
Description was changed from ========== Update viewport_meta setting on any navigation. In http://crrev.com/1785953002 I tied a setting to the is_overriding_user_agent property of NavigationEntry, but I forgot to keep the preference updated during arbitrary navigation entry changes (e.g. back button). BUG=671580 ========== to ========== Update viewport_meta setting on any navigation. In http://crrev.com/1785953002 I tied a setting to the is_overriding_user_agent property of NavigationEntry, but I forgot to keep the preference updated during arbitrary navigation entry changes (e.g. back button). BUG=671580 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
aelias@chromium.org changed reviewers: + nasko@chromium.org
Hi nasko@, PTAL.
https://codereview.chromium.org/2619063003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2619063003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:962: delegate_->UpdateOverridingUserAgent(); Technically this should only be called if we actually committed a new page, right? Regardless whether it is in session history or not. It seems more appropriate to put it in the cases we know it will happen than just generically here.
https://codereview.chromium.org/2619063003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2619063003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:962: delegate_->UpdateOverridingUserAgent(); On 2017/01/09 at 23:07:53, nasko wrote: > Technically this should only be called if we actually committed a new page, right? Regardless whether it is in session history or not. It seems more appropriate to put it in the cases we know it will happen than just generically here. Right. Could you give me a more concrete suggestion of where I should put this instead? This was the most specific place I found, but I'm not too familiar with this code.
https://codereview.chromium.org/2619063003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2619063003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:962: delegate_->UpdateOverridingUserAgent(); On 2017/01/09 23:10:52, aelias wrote: > On 2017/01/09 at 23:07:53, nasko wrote: > > Technically this should only be called if we actually committed a new page, > right? Regardless whether it is in session history or not. It seems more > appropriate to put it in the cases we know it will happen than just generically > here. > > Right. Could you give me a more concrete suggestion of where I should put this > instead? This was the most specific place I found, but I'm not too familiar > with this code. Yes. The switch statement on 850 calls a specific method for each type of navigation. History navigations are classified as NAVIGATION_TYPE_EXISTING_PAGE, so you can put the code inside of RendererDidNavigateToExistingPage. It is also pretty helpful that this method enforces through DCHECK that it is only called for main frames, which fits the behavior of our case too.
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
https://codereview.chromium.org/2619063003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2619063003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:962: delegate_->UpdateOverridingUserAgent(); On 2017/01/09 at 23:44:03, nasko wrote: > History navigations are classified as NAVIGATION_TYPE_EXISTING_PAGE, so you can put the code inside of RendererDidNavigateToExistingPage. It is also pretty helpful that this method enforces through DCHECK that it is only called for main frames, which fits the behavior of our case too. OK, done. Note that I needed to change the underlying entry lookup to GetActiveEntry() because GetVisibleEntry() only changed in NotifyNavigationEntryCommitted().
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2619063003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2619063003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2386: return GetController().GetActiveEntry() && This is not an ok change, since GetActiveEntry is marked deprecated and we'd like to remove it. I've traced through calls and I think your previous patch is the more correct one, even though it will potentially notify on navigations other than back/forward.
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2619063003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2619063003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2386: return GetController().GetActiveEntry() && On 2017/01/10 at 17:33:35, nasko wrote: > This is not an ok change, since GetActiveEntry is marked deprecated and we'd like to remove it. I've traced through calls and I think your previous patch is the more correct one, even though it will potentially notify on navigations other than back/forward. OK then, I reuploaded patch set 1.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
nasko@chromium.org changed reviewers: + creis@chromium.org
Adding creis@, who was interested in this CL and whether the code belongs in NavigationController or not.
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
For what it's worth, the reason this feature is tied to navigation is because the following UX was decided upon back when it was originally added: - After checking the "Request desktop site" checkbox, all links tapped on should remain in desktop site mode. - However, when going back, if the user goes back to a point where the checkbox was not set yet, it should be as originally presented rather than forced into desktop site mode. - The checkbox also only affects the current tab.
On 2017/01/11 00:22:36, aelias wrote: > For what it's worth, the reason this feature is tied to navigation is because > the following UX was decided upon back when it was originally added: > - After checking the "Request desktop site" checkbox, all links tapped on should > remain in desktop site mode. > - However, when going back, if the user goes back to a point where the checkbox > was not set yet, it should be as originally presented rather than forced into > desktop site mode. > - The checkbox also only affects the current tab. Thanks for the context. I can understand the motivation to store the state on NavigationEntry to support that behavior. Ideally, I don't think that NavigationController should be managing this state, beyond setting the values on NavigationEntry. Overriding the user agent isn't really within its scope-- that's more a state of the WebContents. WebContents is also the one that knows how to notify renderers when that state changes. (In general, NavigationController should have a limited number of delegate methods to call back out to WebContents. For post-commit state changes like this, WebContentsImpl::DidNavigateAnyFramePostCommit seems like it would be a better fit, although WebContents would need to store whether it was in the override state before to tell if it has changed on a given commit.) That said, there's apparently a fair amount of this logic in NavigationController already, and I'm not going to ask you to refactor the whole feature in this CL. While it feels out of place in RendererDidNavigate, I do think that's a safe place to put it for the time being, and maybe there will be a way to pull this code out to WebContents later. I'll also note that it's important for this UpdateOverridingUserAgent call to happen on subframe history navigations and not just main frame ones, so the RendererDidNavigateToExistingPage version would have been wrong for that reason as well. Here's repro steps: 1) Visit page A with a blank iframe. 2) Navigate the iframe to page A1. (This is AUTO_SUBFRAME, generating no commit notification and no new entry.) 3) Navigate the iframe to page A2. (This is MANUAL_SUBFRAME, generating both a commit notification and a new entry.) 4) Request desktop site, affecting page A with iframe A2. 5) Go back. From your description, we expect to end up on page A with iframe A1, *not* in desktop mode. Your current CL should hopefully get that right, because the back navigation is AUTO_SUBFRAME and returns true from RendererDidNavigateAutoSubframe, such that we generate a commit notification and get to your newly added code. (This is in contrast to the AUTO_SUBFRAME in step 2, which returns false and does not reach your newly added code. That's ok, because it can't change the RDS setting.) Overall, I'll say this LGTM, but please add a test for the behavior. That will help us avoid regressing it if we refactor the code out to WebContents.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK, thanks for the detailed review, I added a unit test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/01/12 04:03:31, aelias wrote: > OK, thanks for the detailed review, I added a unit test. What
https://codereview.chromium.org/2619063003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2619063003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:810: if (pending_entry_ && I don't think you can remove this check. Plenty of navigations commit without a pending entry (e.g., renderer-initiated navigations).
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2619063003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2619063003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:810: if (pending_entry_ && On 2017/01/12 at 17:45:59, Charlie Reis wrote: > I don't think you can remove this check. Plenty of navigations commit without a pending entry (e.g., renderer-initiated navigations). Right, that removal was a text editing mistake, sorry.
Thanks. LGTM with one addition to the test. https://codereview.chromium.org/2619063003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/2619063003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:1694: EXPECT_EQ(2, change_counter); Would this test fail without your fix? I thought the bug was that the preferences didn't update after you go back. Can you add a GoBack call similar to line 1713 below and verify we get the update? https://codereview.chromium.org/2619063003/diff/80001/content/test/test_rende... File content/test/test_render_view_host.h (right): https://codereview.chromium.org/2619063003/diff/80001/content/test/test_rende... content/test/test_render_view_host.h:207: void set_webkit_preferences_changed_counter(int* counter) { nit: Please add comments similar to set_delete_counter. Same below.
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by aelias@chromium.org
https://codereview.chromium.org/2619063003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/2619063003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:1694: EXPECT_EQ(2, change_counter); On 2017/01/12 at 19:37:54, Charlie Reis wrote: > Would this test fail without your fix? I thought the bug was that the preferences didn't update after you go back. Can you add a GoBack call similar to line 1713 below and verify we get the update? The previous version did fail without the fix, but I changed the test to more specifically simulate the back button scenario. https://codereview.chromium.org/2619063003/diff/80001/content/test/test_rende... File content/test/test_render_view_host.h (right): https://codereview.chromium.org/2619063003/diff/80001/content/test/test_rende... content/test/test_render_view_host.h:207: void set_webkit_preferences_changed_counter(int* counter) { On 2017/01/12 at 19:37:54, Charlie Reis wrote: > nit: Please add comments similar to set_delete_counter. Same below. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2619063003/#ps140001 (title: "Another EXPECT_EQ -> EXPECT_FALSE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484255953156970,
"parent_rev": "4809bbc10e25dff14277270bbff6d6f504178a12", "commit_rev":
"100c919f8fc51a06aee219a6898424f006cca92a"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
Committed as http://crrev.com/443399
Message was sent while issue was closed.
jonchitchat@gmail.com changed reviewers: + jonchitchat@gmail.com
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
On 2017/01/06 23:36:15, aelias wrote: > Description was changed from > > ========== > Update viewport_meta setting on any navigation. > > In http://crrev.com/1785953002 I tied a setting to the > is_overriding_user_agent property of NavigationEntry, but I forgot to > keep the preference updated during arbitrary navigation entry changes > (e.g. back button). > > BUG=671580 > ========== > > to > > ========== > Update viewport_meta setting on any navigation. > > In http://crrev.com/1785953002 I tied a setting to the > is_overriding_user_agent property of NavigationEntry, but I forgot to > keep the preference updated during arbitrary navigation entry changes > (e.g. back button). > > BUG=671580 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation > ========== Wtf |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
