Unfortunately, even launching the local NTP won't let us get rid of the title hackery in SearchTabHelper, as third-party NTPs might still depend on it. (Though I'm not sure if they actually do depend on it in practice - if none of them do, then maybe we can get rid of it after all.)

If we do need to keep the title logic, then I'm not sure how to fix it. Is there any "title was changed" notification that we could intercept and rewrite the title again?

The solution hack that I put in that CL is to set the title after the load completes in DidFinishLoad(). Take a look (https://crrev.com/c/590273).

Your suggestion to intercept the "title was changed" notification won't work and seems to show some misunderstanding of the issue. Right now, the NTP is title-less, and therefore there will not be a "title was changed" notification. I'm trying to change that, but the NTP issue needs to be changed first.

BTW, I'm not saying you can't set the title at all in SearchTabHelper. I'm saying you must do it after the load is complete.

My understanding was that with your change, the NTP will go from being title-less to having an empty title. I was wondering whether there's any way to intercept that empty title and replace it in-flight. I guess the answer is "no".

I think your CL as it is right now might cause some flickering in the title, since it'll get set a bit later.

Ah, so if we land your change and mine together we might be able to do it. (Landing them separately would break NTP titles and I wouldn't do that.)

There is WebContentsObserver::TitleWasSet(). That's called after the NavigationEntry's title was updated, so I suppose we could set the title again in that callback.

BTW, I find the code that determines if a page is the NTP rather confusing. The DidStartNavigation code uses "search::IsNTPURL()", while the DidFinishNavigation code uses "entry->GetVirtualURL() == chrome::kChromeUINewTabURL || search::NavEntryIsInstantNTP()". How does one tell if a page is the NTP?

BTW, I'm not even sure how setting the title directly on a NavigationEntry works, and how it updates the UI. I've run into cases where code mangled the title on a NavigationEntry and the UI didn't properly update. I highly recommend setting the title either with a <title> tag, or with injected JavaScript that sets document.title, as that will flow through the proper code paths.

Alright, sounds like TitleWasSet should work. Do you want to include that in your CL? I'll be happy to help testing it.

Agreed that the whole title mangling is horrible, and it has lead to bugs in the past. Unfortunately, it's basically a public API (third-party NTPs might depend on it), so it's hard to change.


You're right that the number of ways to check "is this an NTP" is completely ridiculous. They all have subtle differences (e.g. whether they include the local NTP, the incognito/guest mode NTPs, the native Android/iOS NTPs, and probably more that I'm forgetting), and it's very hard to check at any given call site which of those is actually intended. </rant>

"Unfortunately, it's basically a public API (third-party NTPs might depend on it), so it's hard to change."

Can you clarify what you mean by the "title mangling"? What API do we need to support?

Sorry for being unclear. By "the API", I meant the fact that an NTP without a title will get its title set by Chrome.
Note that the NTP can be provided by the user's default search engine. Besides Google, Bing and Yandex have their own NTPs.

I am aware of third-party NTPs. To be explicit, the contract we need to fulfil is that, if the NTP has no title by the end of loading, we need to set IDS_NEW_TAB_TITLE as the title. Correct?

--

There are two times in SearchTabHelper that the title is set, and they are both problematic.

1. In DidStartNavigation(), if |search::IsNTPURL()| is true, then IDS_NEW_TAB_TITLE is set as the pending navigation entry's title.

2. In DidFinishNavigation(), if there is no set title, and the pending entry is NTP-like (the virtual URL is kChromeUINewTabURL and |search::NavEntryIsInstantNTP()| is true) then IDS_NEW_TAB_TITLE is set as the pending navigation entry's title.

In both cases, the loading of a page with no title tag will reset the title in the navigation entry during the process of loading, which happens after navigation is complete. Therefore, any title set within any navigation callback will be overwritten.

(As a side note, there's a comment on 2 about how the title set can be reset in "several places" and calls out clicking the reload button. That title resetting was due to a bad fix for the same bug I'm trying to correctly fix now 🙂.)

The plan, then, is to remove both navigation title functions, and instead intercept TitleWasSet. In that function, if the page is an NTP and the title being set is blank, we instead set IDS_NEW_TAB_TITLE. How do I determine if the page is "an NTP" for this purpose?

re contract: Mostly; I think we'll have to set the title before the end of loading to avoid flicker. Basically, I think 1. will have to stay. Otherwise, the plan SGTM.
I think IsInstantNTP or NavEntryIsInstantNTP or IsInstantNTPURL should all work for this.

"Basically, I think 1. will have to stay."

The point of this bug is that to fix a bug in core web functionality, 1. will stop working. There is no point in leaving in non-functional code.

Ah. Well, we'll have to make sure there's no flickering in the title in some way. Maybe just rewriting the empty title to IDS_NEW_TAB_TITLE will be enough, I don't know.

In experimenting, I'm realizing that if I remove the initial set of a title on the pending NavigationEntry, then the page setting a null title will early-return and we won't catch it. I might need to leave #1 happening. Not because it will work, but because then we can catch the title change and intercept it.

Still investigating; will let you know once I have something working.

This seems to work, and seems to prevent your flickering as well.

Officially over to you, since your pending CL crrev.com/c/590273 fixes this :)

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

commit 5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e
Author: Avi Drissman <avi@chromium.org>
Date: Tue Nov 14 01:19:51 2017

Clear the page title on reload.

This change makes it so that Blink, as it loads a page,
always reports a title, removing guesswork from the higher
layers.

BUG= 96041 ,  783529 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel
Change-Id: Ide09c91342cdb3d36f8a02fccf2f4755d551a64f
Reviewed-on: https://chromium-review.googlesource.com/590273
Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516133}
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/chrome/browser/ui/search/search_tab_helper.h
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/chrome/browser/ui/search/search_tab_helper_unittest.cc
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/chrome/test/data/extensions/api_test/tabs/on_updated/test.js
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/fast/loader/subframe-removes-itself-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/307-after-303-after-post-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/bad-scheme-subframe-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/bad-server-subframe-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/basic-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/dont-preload-non-img-srcset-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/empty-subframe-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/fire-error-event-empty-404-script-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/fire-error-event-script-no-content-type-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/load-javascript-after-many-xhrs-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/location-hash-reload-cycle-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/module-script-wrong-mime-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/nested_bad_objects-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/onload-vs-immediate-refresh-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/pdf-commit-load-callbacks-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/pending-script-leak-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-append-scan-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-css-test-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-ignore-invalid-base-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-sizes-2x-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-sizes-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-src-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-srcset-2x-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-srcset-duplicate-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-srcset-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-srcset-reverse-order-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-srcset-src-preloaded-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-image-srcset-src-preloaded-reverse-order-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-picture-invalid-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-picture-nested-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-picture-sizes-2x-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-picture-sizes-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/preload-video-poster-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/redirect-methods-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/redirect-with-no-location-crash-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/remove-child-triggers-parser-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/slow-parsing-subframe-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/state-object-security-exception-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/stop-load-at-commit-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/loading/text-content-type-with-binary-extension-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/navigation/cross-origin-fragment-navigation-is-async-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/navigation/same-origin-fragment-navigation-is-sync-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-iframe-with-hsts.https-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/platform/linux/http/tests/loading/simple-subframe-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/platform/mac/http/tests/loading/simple-subframe-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/LayoutTests/platform/win/http/tests/loading/simple-subframe-expected.txt
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/5b4a0cb6c9f73c4c27bddd6909706452cf2cd47e/third_party/WebKit/Source/core/exported/WebFrameTest.cpp