• Lukasz Anforowicz's avatar
    NTP-initiated navigations should show-up as "visible" NavigationEntry. · 641234d5
    Lukasz Anforowicz authored
    r705290 moved when exactly a renderer/NTP-initiated navigation starts to
    be treated as if it was a browser/bookmark-initiated navigation.  This
    regressed omnibox behavior which stopped showing the URL of a navigation
    initiated by an NTP click.  The main cause of the regression is that
    after r705290 the NavigationEntryImpl::is_renderer_initiated() would
    return true (inconsistent with NavigationRequest).
    
    This CL makes sure that NavigationEntry associated with a
    renderer/NTP-initiated navigation also indicates that this navigation
    should be treated as browser-initiated.  This is achieved by adding an
    extra call to ContentBrowserClient::OverrideNavigationParams *before*
    creating the NavigationEntry and *before* the new Omnibox display text
    is calculated by the following callstack:
        LocationBarModelImpl::GetFormattedURL()
        LocationBarModelImpl::GetURLForDisplay()
        OmniboxEditModel::ResetDisplayTexts()
        OmniboxViewViews::Update()
        LocationBarView::Update()
        ToolbarView::Update()
        Browser::UpdateToolbar()
        Browser::ScheduleUIUpdate()
        Browser::NavigationStateChanged()
        content::WebContentsImpl::NotifyNavigationStateChanged()
        content::NavigatorImpl::GetNavigationEntryForRendererInitiatedNavigation()
        content::NavigatorImpl::OnBeginNavigation()
        content::RenderFrameHostImpl::BeginNavigation()
    
    The decision whether a navigation is NTP-initiated needs to be based on
    the |source_site_instance|.  The decision cannot be based on the
    |initiator_origin|, because we want https://example.com to be treated
    differently from https://example.com/chrome/ntp (the latter might have a
    different effective Site URL when it is designated as a "remote NTP").
    This requirement necessitates adding a |source_site_instance| parameter
    to NavigationController::CreateNavigationEntry().  In the NTP scenario,
    the NavigationEntry is created by the following callstack:
        NavigationEntryImpl::NavigationEntryImpl()
        NavigationController::CreateNavigationEntry()
        NavigatorImpl::GetNavigationEntryForRendererInitiatedNavigation()
        NavigatorImpl::OnBeginNavigation()
        RenderFrameHostImpl::BeginNavigation()
    
    To account for DCHECKs that ensure that NavigationRequest and
    NavigationEntryImpl are compatible, the CL also had to move the older
    OverrideNavigationParams call from NavigationRequest::StartNavigation to
    the NavigationRequest's constructor.
    
    This CL adds a regression test: LocalNTPTest.PendingNavigations
    Additionally, I've manually tested that repro steps from
    https://crbug.com/1020610#c1 have expected behavior after this CL.
    
    Bug: 1020610
    Change-Id: I7a16b28eb9426c348f37f432e7b4813bee55ddd6
    Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1896295Reviewed-by: default avatarNasko Oskov <nasko@chromium.org>
    Reviewed-by: default avatarDan Beam <dbeam@chromium.org>
    Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
    Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#713592}
    641234d5
local_ntp_browsertest.cc 53.9 KB