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

Issue 683402 link

Starred by 1 user
Status: Fixed
Owner:
jam@chromium.org
Closed: Jan 2018
Cc:
█ kbr@chromium.org
clamy@chromium.org
█ nasko@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 689549
Blocking:
issue 832180
issue 575305



Sign in to add a comment

Make some NavigationHandle getters available earlier once PlzNavigate is turned on

Project Member Reported by jam@chromium.org, Jan 20 2017 
When converting some WebContentsObservers to use the new API, one thing that came up was that some getters are now not possible to call in DidStartNavigation. It looks like for non-PlzNavigate, it makes sense that we have to wait for the network request to start before this information is available. However with PlzNavigate, these members are already known when the NavigationRequest is created.

So when PlzNavigate is the only code path, I suggest we set these early so that their getters can have the dcheck's removed.

virtual bool IsPost() = 0;
virtual const Referrer& GetReferrer() = 0;
virtual bool HasUserGesture() = 0;
virtual ui::PageTransition GetPageTransition() = 0;

Comment 1 by jam@chromium.org, Jan 21 2017 

btw two CLs that could change to use DidStartNavigation once this is fixed:
https://codereview.chromium.org/2648803002
https://codereview.chromium.org/2645153003/

Comment 2 by kbr@chromium.org, Jun 3 2017

Cc: kbr@chromium.org
Another CL (still very much a work in progress) that could also benefit from the earlier access to these flags:
https://chromium-review.googlesource.com/522938/

Comment 3 by kbr@chromium.org, Jun 5 2017

Blocking: 575305

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

Labels: Proj-PlzNavigate

Comment 5 by kbr@chromium.org, Aug 15 2017

Blockedon: 689549
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 4 2018 

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

commit 028a4fd073be923c50b37265461eac63189715b5
Author: John Abd-El-Malek <jam@chromium.org>
Date: Thu Jan 04 16:49:22 2018

Make some NavigationHandle getters available earlier now that PlzNavigate is the only path.

Bug:  683402 
Change-Id: I8e7ed0ddf13f43e81006c2bffefee0a577113207
Reviewed-on: https://chromium-review.googlesource.com/849664
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527012}
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/chrome/browser/extensions/extension_navigation_throttle_unittest.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/chrome/browser/page_load_metrics/experiments/delay_navigation_throttle_unittest.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/chrome/browser/plugins/flash_download_interception_unittest.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/chrome/browser/ssl/typed_navigation_timing_throttle_unittest.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/components/navigation_interception/intercept_navigation_throttle_unittest.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/browser/frame_host/interstitial_page_navigator_impl.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/browser/frame_host/navigation_handle_impl_unittest.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/public/browser/navigation_handle.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/public/browser/navigation_handle.h
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/test/test_render_frame_host.cc
[modify] https://crrev.com/028a4fd073be923c50b37265461eac63189715b5/content/test/test_render_frame_host.h

Comment 7 by jam@chromium.org, Jan 4 2018

Owner: jam@chromium.org
Status: Fixed (was: Available)

Comment 8 by kbr@chromium.org, Apr 12 2018

Blocking: 832180
►

Sign in to add a comment