Remove usage of GetVisibleEntry |
||
Issue descriptionTo help with the ongoing NavigationController refactoring, we need to remove GetVisibleEntry from the NavigationController interface. In order to do this, I have the following plan: 1) Replace usages with WebContents::GetVisibleURL when possible. 2) Replace usages with NavigationController::GetLastCommittedEntry when possible. 3) Audit the remaining callsites to assess what data needs to be added to the NavigationController interface to completely remove GetVisibleEntry.
,
Jun 18 2018
To clarify the "when possible" parts, the "visible" entry/URL is needed for features that must stay in sync with the URL shown in the omnibox, even when it's a pending URL. That's often things like the lock icon, page info dialog, favicon, etc, which are closely tied to the omnibox state. The majority of code that wants to interact with URLs and NavigationEntries (in our experience) actually cares about what page is currently running in the tab, which is the "last committed" one instead. That's particularly important for almost all security related checks, unless they're about URL spoofs. Hope that helps in distinguishing which ones should migrate to last committed and which ones need to stay on some variant of GetVisibleEntry / GetVisibleURL.
,
Jun 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d11fcbfb5f036cdfa6c73cb4d012bdcb7caace7 commit 4d11fcbfb5f036cdfa6c73cb4d012bdcb7caace7 Author: clamy <clamy@chromium.org> Date: Wed Jun 20 12:41:01 2018 Remove usage of GetVisibleEntry [1/n] This CL is part of a serie of CLs to remove usage of Navigation::GetVisibleEntry when possible. Instead, we use WebContents::GetVisibleURL or NavigationController::GetLastCommittedEntry when appropriate. Bug: 853705 Change-Id: I97ddfe4712fe68ff730ebc7ff1a6756094d3554d Reviewed-on: https://chromium-review.googlesource.com/1104349 Reviewed-by: Bernhard Bauer <bauerb@chromium.org> Commit-Queue: Camille Lamy <clamy@chromium.org> Cr-Commit-Position: refs/heads/master@{#568800} [modify] https://crrev.com/4d11fcbfb5f036cdfa6c73cb4d012bdcb7caace7/chrome/browser/content_settings/tab_specific_content_settings.cc |
||
►
Sign in to add a comment |
||
Comment 1 by clamy@chromium.org
, Jun 18 2018Status: Assigned (was: Untriaged)