Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2086423005:
Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle() (Closed)
|
Created:
4 years, 6 months ago by afakhry Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUsing WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle()
NavigationEntry::SetTitle() doesn't result in the emission of
WebContentsObserver::TitleWasSet() events. This used to cause listeners like
the task manager to show wrong titles of tabs (for example in the event of an
interstitial page).
BUG=607074
TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/6f0c1ec23bc1eabe221fccdb4b28b1d96273a705
Cr-Commit-Position: refs/heads/master@{#405485}
Patch Set 1 #Patch Set 2 : Extend content_browsertests and fix failures #Patch Set 3 : Make sure the renderer process is not shutting down. #Patch Set 4 : Remove the unnecessary NOTREACHED() and update the Fav icon in the task manager. #Patch Set 5 : DidFinishNavigation is received when RenderFrameHostImpl is destroyed!! #
Total comments: 25
Patch Set 6 : Charlie's comments #Patch Set 7 : Stop listening to attach/detach #Patch Set 8 : Use UpdateTitleForEntry() instead of SetTitle() whenever the entry belongs to a WebContents #Patch Set 9 : Self review #Patch Set 10 : Fix compile error #
Total comments: 11
Patch Set 11 : charlie's and nick's comments #Patch Set 12 : Splitting the task manager changes into its own CL. #
Total comments: 4
Patch Set 13 : Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle() #
Total comments: 14
Patch Set 14 : thestig's comments #Patch Set 15 : Improving comment and removing the NOTREACHED. #
Total comments: 1
Messages
Total messages: 46 (19 generated)
Description was changed from ========== Fix interstitial pages not updating the task manager renderers' titles This ensures that when an interstitial page updates the title of its web contents, a WebContentsObserver::TitleWasSet() event is emitted. Screen recording: https://drive.google.com/a/google.com/file/d/0B6G_-uQnf1_LODFLaTFESlpqWUU/vie... BUG=607074 ========== to ========== Fix interstitial pages not updating the task manager renderers' titles This ensures that when an interstitial page updates the title of its web contents, a WebContentsObserver::TitleWasSet() event is emitted. Screen recording: https://drive.google.com/a/google.com/file/d/0B6G_-uQnf1_LODFLaTFESlpqWUU/vie... BUG=607074 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix interstitial pages not updating the task manager renderers' titles This ensures that when an interstitial page updates the title of its web contents, a WebContentsObserver::TitleWasSet() event is emitted. Screen recording: https://drive.google.com/a/google.com/file/d/0B6G_-uQnf1_LODFLaTFESlpqWUU/vie... BUG=607074 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix interstitial pages not updating the task manager renderers' titles This ensures that when an interstitial page updates the title of its web contents, a WebContentsObserver::TitleWasSet() event is emitted. Screen recording: https://drive.google.com/a/google.com/file/d/0B6G_-uQnf1_LODFLaTFESlpqWUU/vie... BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
afakhry@chromium.org changed reviewers: + creis@chromium.org
Charlie, kindly review this CL. Thank you!
Description was changed from ========== Fix interstitial pages not updating the task manager renderers' titles This ensures that when an interstitial page updates the title of its web contents, a WebContentsObserver::TitleWasSet() event is emitted. Screen recording: https://drive.google.com/a/google.com/file/d/0B6G_-uQnf1_LODFLaTFESlpqWUU/vie... BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix interstitial pages not updating the task manager renderers' titles This ensures that when an interstitial page updates the title of its web contents, a WebContentsObserver::TitleWasSet() event is emitted. Screen recording: https://drive.google.com/a/google.com/file/d/0B6G_-uQnf1_LX0VtUnNWaEd6cUU/vie... BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
afakhry@chromium.org changed reviewers: + nick@chromium.org
+nick@chromium.org for web_contents_task_provider.cc
On 2016/06/23 15:28:19, afakhry wrote: > Charlie, kindly review this CL. Thank you! Sorry for the delay! I'll try to take a look later today, as things have been a bit hectic this week.
Thanks for the fix, and sorry for my late review! I like where this is headed, though I have a few questions and suggestions below. https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/guest_task.cc (right): https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/guest_task.cc:44: return base::string16(); Just curious, in which case did this fail? Might be nice to have a comment about it, unless it's covered by one of your new tests. https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:59: content::NavigationHandle* navigation_handle) override; Hooray! Thanks for moving to the new NavHandle-based API. https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:172: web_contents()->GetMainFrame()->GetProcess()); This line caught my eye, since it looks buggy for OOPIFs. And indeed, I get a crash here in debug builds with the following repro steps: 1) Start Chrome with --site-per-process and open Task Manager. 2) Visit http://csreis.github.io/tests/cross-site-iframe.html. 3) Click "Go cross-site (simple page)" 4) Right click inside the iframe and open DevTools. (Important, so that we're inspecting the OOPIF's page.) 5) Type "while (1);" in the console. 6) Try to select text in the iframe and wait 20 seconds. I'll file this as a separate bug so that we can keep this CL focused on titles. https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:205: void WebContentsEntry::DidAttachInterstitialPage() { Not sure I understand-- why do we need DidAttachInterstitialPage and DidDetachInterstitialPage in addition to the changes to how TitleWasSet is called? https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:304: INVALIDATE_TYPE_TITLE); One way to make this cleaner would be to move NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE) inside UpdateTitleForEntry. All the call sites do it immediately afterward, and it looks like they should only do it if it returns true. If we put that call inside UpdateTitleForEntry, it probably doesn't need a return value at all. Also, it seems consistent with the intent of UpdateTitleForEntry, which is updating both the entry and the view. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:416: controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE); Same here. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:677: NavigationEntryImpl* visible_entry = controller_->GetVisibleEntry(); I think this might be wrong. In general, we shouldn't mix values from GetVisibleEntry and GetLastCommittedEntry, which could be two unrelated pages. Here, I'm not 100% sure this will always return the transient entry and not the pending entry, in which case it might put the old title on the pending entry. Using GetTransientEntry would be safer if that's your intent. That said, I'm a bit skeptical about updating the transient entry as well. Why would we need to put the last committed page's title on the transient entry? https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl_browsertest.cc:185: // changes made by the interstitial page is emitted to the WebContentsObservers nit: s/is/are/ https://codereview.chromium.org/2086423005/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:802: friend class InterstitialPageImpl; I think it's probably safer to make UpdateTitleForEntry public and remove the whole friend declaration, especially if we do the other cleanup I suggested.
https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/guest_task.cc (right): https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/guest_task.cc:44: return base::string16(); On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > Just curious, in which case did this fail? Might be nice to have a comment > about it, unless it's covered by one of your new tests. This caused crashes in some browsertests when I changed the task manager from listening to DidNavigateMainFrame() to listening to DidFinishNavigation() as Nick told me the former will be deprecated. Not related to interstitial titles. Here are the failing tests: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... With the following stack trace: [13384:13384:0623/120511:FATAL:guest_task.cc(43)] Check failed: guest. #0 0x000004c7a1ee base::debug::StackTrace::StackTrace() #1 0x000004c9024a logging::LogMessage::~LogMessage() #2 0x000004e9fac7 task_management::GuestTask::GetCurrentTitle() #3 0x000004e9fb6b task_management::GuestTask::UpdateTitle() #4 0x000004ea2998 task_management::WebContentsEntry::DidFinishNavigation() #5 0x0000025fa347 content::WebContentsImpl::DidFinishNavigation() #6 0x000002364e18 content::NavigationHandleImpl::~NavigationHandleImpl() #7 0x000002365199 content::NavigationHandleImpl::~NavigationHandleImpl() #8 0x00000236ca4a content::RenderFrameHostImpl::~RenderFrameHostImpl() #9 0x00000236ce49 content::RenderFrameHostImpl::~RenderFrameHostImpl() #10 0x00000237d128 content::RenderFrameHostManager::~RenderFrameHostManager() #11 0x000002356556 content::FrameTreeNode::~FrameTreeNode() #12 0x000002356897 content::FrameTreeNode::ResetForNewProcess() #13 0x0000025e895a content::WebContentsImpl::~WebContentsImpl() #14 0x0000025e9739 content::WebContentsImpl::~WebContentsImpl() #15 0x0000036472eb guest_view::GuestViewBase::Destroy() #16 0x0000025e8fa4 content::WebContentsImpl::~WebContentsImpl() #17 0x0000025e9739 content::WebContentsImpl::~WebContentsImpl() #18 0x0000035b03e3 extensions::AppWindowContentsImpl::~AppWindowContentsImpl() #19 0x0000035ad91e extensions::AppWindow::~AppWindow() #20 0x0000035adad9 extensions::AppWindow::~AppWindow() #21 0x0000035ae497 extensions::AppWindow::OnNativeClose() #22 0x0000058aaa66 views::Widget::OnNativeWidgetDestroyed() #23 0x0000058b66cc views::NativeWidgetAura::OnWindowDestroyed() #24 0x0000061f629e aura::Window::~Window() #25 0x0000061f69a9 aura::Window::~Window() #26 0x000004d074a9 base::debug::TaskAnnotator::RunTask() #27 0x000004c97a45 base::MessageLoop::RunTask() #28 0x000004c97d38 base::MessageLoop::DeferOrRunPendingTask() #29 0x000004c9805b base::MessageLoop::DoWork() #30 0x000004c99b79 base::MessagePumpGlib::Run() #31 0x000004c9757d base::MessageLoop::RunHandler() #32 0x000004cba500 base::RunLoop::Run() #33 0x0000075fe99c content::RunThisRunLoop() #34 0x0000075fe93a content::RunMessageLoop() #35 0x0000015a2741 InProcessBrowserTest::QuitBrowsers() #36 0x0000015a24e5 InProcessBrowserTest::RunTestOnMainThreadLoop() #37 0x0000016842f4 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() #38 0x000004d38da9 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() #39 0x000004d37ea5 ChromeBrowserMainParts::PreMainMessageLoopRun() #40 0x000001759e31 chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() #41 0x00000229a3d5 content::BrowserMainLoop::PreMainMessageLoopRun() #42 0x0000025c3517 content::StartupTaskRunner::RunAllTasksNow() #43 0x000002298b94 content::BrowserMainLoop::CreateStartupTasks() #44 0x00000229d182 content::BrowserMainRunnerImpl::Initialize() #45 0x000002296336 content::BrowserMain() #46 0x000004b8149e content::ContentMainRunnerImpl::Run() #47 0x000004b7fe80 content::ContentMain() #48 0x000001683ca8 content::BrowserTestBase::SetUp() #49 0x0000015a09eb InProcessBrowserTest::SetUp() #50 0x0000057c2cc8 testing::Test::Run() #51 0x0000057c3a3a testing::TestInfo::Run() #52 0x0000057c3ec3 testing::TestCase::Run() #53 0x0000057cb0b9 testing::internal::UnitTestImpl::RunAllTests() #54 0x0000057cad5e testing::UnitTest::Run() #55 0x0000015acb66 base::TestSuite::Run() #56 0x000004c6f59b ChromeTestSuiteRunner::RunTestSuite() #57 0x0000075fb7ce content::LaunchTests() #58 0x0000005fedc1 main #59 0x7f780ddf67ed __libc_start_main #60 0x0000005fec81 <unknown> I'm not sure of the meaning of that when it happens, so I don't know what's the right comment to write here. I also changed it from returning an empty string16() to returning the original cached title(). https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:59: content::NavigationHandle* navigation_handle) override; On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > Hooray! Thanks for moving to the new NavHandle-based API. Yes, Nick told me DidNavigateMainFrame() will be deprecated. https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:172: web_contents()->GetMainFrame()->GetProcess()); On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > This line caught my eye, since it looks buggy for OOPIFs. And indeed, I get a > crash here in debug builds with the following repro steps: > > 1) Start Chrome with --site-per-process and open Task Manager. > 2) Visit http://csreis.github.io/tests/cross-site-iframe.html. > 3) Click "Go cross-site (simple page)" > 4) Right click inside the iframe and open DevTools. (Important, so that we're > inspecting the OOPIF's page.) > 5) Type "while (1);" in the console. > 6) Try to select text in the iframe and wait 20 seconds. > > I'll file this as a separate bug so that we can keep this CL focused on titles. I saw the bug you filed, and I will handle it soon. Thanks for letting me know about this issue. https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:205: void WebContentsEntry::DidAttachInterstitialPage() { On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > Not sure I understand-- why do we need DidAttachInterstitialPage and > DidDetachInterstitialPage in addition to the changes to how TitleWasSet is > called? Updating titles here alone is not enough. It will update the title to the link "malware.testing.google.test/testing/malware" but not the actual title "Security Error". We can probably skip the title update here, since now we have it in TitleWasSet(). However, we need this for updating the Favicon. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:304: INVALIDATE_TYPE_TITLE); On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > One way to make this cleaner would be to move > NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE) inside UpdateTitleForEntry. > All the call sites do it immediately afterward, and it looks like they should > only do it if it returns true. > > If we put that call inside UpdateTitleForEntry, it probably doesn't need a > return value at all. Also, it seems consistent with the intent of > UpdateTitleForEntry, which is updating both the entry and the view. I made it public, moved the call to NotifyNavigationStateChanged() from WebContentsImpl::UpdateTitle() to UpdateTitleForEntry(), I didn't change the return value however, just case it's needed in the future as UpdateTitleForEntry() maybe a no-op sometimes. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:416: controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE); On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > Same here. Done. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:677: NavigationEntryImpl* visible_entry = controller_->GetVisibleEntry(); On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > I think this might be wrong. In general, we shouldn't mix values from > GetVisibleEntry and GetLastCommittedEntry, which could be two unrelated pages. > > Here, I'm not 100% sure this will always return the transient entry and not the > pending entry, in which case it might put the old title on the pending entry. > Using GetTransientEntry would be safer if that's your intent. > > That said, I'm a bit skeptical about updating the transient entry as well. Why > would we need to put the last committed page's title on the transient entry? We need this if we want TitleWasSet() to be emitted when "Back to saftey" is pressed. I used the visible entery as I found it to be corresponding to the interstitial page itself when it's being dismissed due to "Back to safety" pressed. I also found that the last committed entry corresponds to the page we were at before we navigated to the malware page that resulted in the interstitial showing up. I used the transient_entry, as suggested, instead of the visible entry and it worked too. The idea is that I need different entries with different titles, otherwise calling UpdateTitleForEntry() will be a no-op here: https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... and here: https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl_browsertest.cc:185: // changes made by the interstitial page is emitted to the WebContentsObservers On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > nit: s/is/are/ Done. https://codereview.chromium.org/2086423005/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:802: friend class InterstitialPageImpl; On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > I think it's probably safer to make UpdateTitleForEntry public and remove the > whole friend declaration, especially if we do the other cleanup I suggested. Done.
Thanks-- a few more thoughts below. https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/guest_task.cc (right): https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/guest_task.cc:44: return base::string16(); On 2016/06/27 14:29:18, afakhry wrote: > On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > > Just curious, in which case did this fail? Might be nice to have a comment > > about it, unless it's covered by one of your new tests. > > This caused crashes in some browsertests when I changed the task manager from > listening to DidNavigateMainFrame() to listening to DidFinishNavigation() as > Nick told me the former will be deprecated. Not related to interstitial titles. > > Here are the failing tests: > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > With the following stack trace: > > [13384:13384:0623/120511:FATAL:guest_task.cc(43)] Check failed: guest. > #0 0x000004c7a1ee base::debug::StackTrace::StackTrace() > #1 0x000004c9024a logging::LogMessage::~LogMessage() > #2 0x000004e9fac7 task_management::GuestTask::GetCurrentTitle() > #3 0x000004e9fb6b task_management::GuestTask::UpdateTitle() > #4 0x000004ea2998 task_management::WebContentsEntry::DidFinishNavigation() > #5 0x0000025fa347 content::WebContentsImpl::DidFinishNavigation() > #6 0x000002364e18 content::NavigationHandleImpl::~NavigationHandleImpl() > #7 0x000002365199 content::NavigationHandleImpl::~NavigationHandleImpl() > #8 0x00000236ca4a content::RenderFrameHostImpl::~RenderFrameHostImpl() > #9 0x00000236ce49 content::RenderFrameHostImpl::~RenderFrameHostImpl() > #10 0x00000237d128 content::RenderFrameHostManager::~RenderFrameHostManager() > #11 0x000002356556 content::FrameTreeNode::~FrameTreeNode() > #12 0x000002356897 content::FrameTreeNode::ResetForNewProcess() > #13 0x0000025e895a content::WebContentsImpl::~WebContentsImpl() > #14 0x0000025e9739 content::WebContentsImpl::~WebContentsImpl() > #15 0x0000036472eb guest_view::GuestViewBase::Destroy() > #16 0x0000025e8fa4 content::WebContentsImpl::~WebContentsImpl() > #17 0x0000025e9739 content::WebContentsImpl::~WebContentsImpl() > #18 0x0000035b03e3 extensions::AppWindowContentsImpl::~AppWindowContentsImpl() > #19 0x0000035ad91e extensions::AppWindow::~AppWindow() > #20 0x0000035adad9 extensions::AppWindow::~AppWindow() > #21 0x0000035ae497 extensions::AppWindow::OnNativeClose() > #22 0x0000058aaa66 views::Widget::OnNativeWidgetDestroyed() > #23 0x0000058b66cc views::NativeWidgetAura::OnWindowDestroyed() > #24 0x0000061f629e aura::Window::~Window() > #25 0x0000061f69a9 aura::Window::~Window() > #26 0x000004d074a9 base::debug::TaskAnnotator::RunTask() > #27 0x000004c97a45 base::MessageLoop::RunTask() > #28 0x000004c97d38 base::MessageLoop::DeferOrRunPendingTask() > #29 0x000004c9805b base::MessageLoop::DoWork() > #30 0x000004c99b79 base::MessagePumpGlib::Run() > #31 0x000004c9757d base::MessageLoop::RunHandler() > #32 0x000004cba500 base::RunLoop::Run() > #33 0x0000075fe99c content::RunThisRunLoop() > #34 0x0000075fe93a content::RunMessageLoop() > #35 0x0000015a2741 InProcessBrowserTest::QuitBrowsers() > #36 0x0000015a24e5 InProcessBrowserTest::RunTestOnMainThreadLoop() > #37 0x0000016842f4 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() > #38 0x000004d38da9 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() > #39 0x000004d37ea5 ChromeBrowserMainParts::PreMainMessageLoopRun() > #40 0x000001759e31 > chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() > #41 0x00000229a3d5 content::BrowserMainLoop::PreMainMessageLoopRun() > #42 0x0000025c3517 content::StartupTaskRunner::RunAllTasksNow() > #43 0x000002298b94 content::BrowserMainLoop::CreateStartupTasks() > #44 0x00000229d182 content::BrowserMainRunnerImpl::Initialize() > #45 0x000002296336 content::BrowserMain() > #46 0x000004b8149e content::ContentMainRunnerImpl::Run() > #47 0x000004b7fe80 content::ContentMain() > #48 0x000001683ca8 content::BrowserTestBase::SetUp() > #49 0x0000015a09eb InProcessBrowserTest::SetUp() > #50 0x0000057c2cc8 testing::Test::Run() > #51 0x0000057c3a3a testing::TestInfo::Run() > #52 0x0000057c3ec3 testing::TestCase::Run() > #53 0x0000057cb0b9 testing::internal::UnitTestImpl::RunAllTests() > #54 0x0000057cad5e testing::UnitTest::Run() > #55 0x0000015acb66 base::TestSuite::Run() > #56 0x000004c6f59b ChromeTestSuiteRunner::RunTestSuite() > #57 0x0000075fb7ce content::LaunchTests() > #58 0x0000005fedc1 main > #59 0x7f780ddf67ed __libc_start_main > #60 0x0000005fec81 <unknown> > > I'm not sure of the meaning of that when it happens, so I don't know what's the > right comment to write here. I also changed it from returning an empty > string16() to returning the original cached title(). Ack. For posterity (when the link above expires), the failing tests were: WebViewTests_WebViewTest.Shim_TestNestedCrossOriginSubframes_0 WebViewTests_WebViewTest.Shim_TestNestedCrossOriginSubframes_1 WebViewTests_WebViewTest.Shim_TestNestedSubframes_0 WebViewTests_WebViewTest.Shim_TestNestedSubframes_1 https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:205: void WebContentsEntry::DidAttachInterstitialPage() { On 2016/06/27 14:29:18, afakhry wrote: > On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > > Not sure I understand-- why do we need DidAttachInterstitialPage and > > DidDetachInterstitialPage in addition to the changes to how TitleWasSet is > > called? > > Updating titles here alone is not enough. It will update the title to the link > "malware.testing.google.test/testing/malware" but not the actual title "Security > Error". We can probably skip the title update here, since now we have it in > TitleWasSet(). > > However, we need this for updating the Favicon. I think I'm ok with that. Just curious, would we still need it if we put a UpdateFavicon in TitleWasSet? That might be a bit cleaner and less brittle if other places trigger TitleWasSet, assuming that titles and favicons often (not always) change at the same time. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:677: NavigationEntryImpl* visible_entry = controller_->GetVisibleEntry(); On 2016/06/27 14:29:18, afakhry wrote: > On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > > I think this might be wrong. In general, we shouldn't mix values from > > GetVisibleEntry and GetLastCommittedEntry, which could be two unrelated pages. > > > > Here, I'm not 100% sure this will always return the transient entry and not > the > > pending entry, in which case it might put the old title on the pending entry. > > Using GetTransientEntry would be safer if that's your intent. > > > > That said, I'm a bit skeptical about updating the transient entry as well. > Why > > would we need to put the last committed page's title on the transient entry? > > We need this if we want TitleWasSet() to be emitted when "Back to saftey" is > pressed. > I used the visible entery as I found it to be corresponding to the interstitial > page itself when it's being dismissed due to "Back to safety" pressed. I also > found that the last committed entry corresponds to the page we were at before we > navigated to the malware page that resulted in the interstitial showing up. > I used the transient_entry, as suggested, instead of the visible entry and it > worked too. > > The idea is that I need different entries with different titles, otherwise > calling UpdateTitleForEntry() will be a no-op here: > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > and here: > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... I feel like we're going about this the wrong way. We shouldn't have to change the title of the transient entry just before it's deleted to notify observers. Deleting the transient entry and going back to showing the last committed entry (on the DiscardNonCommittedEntries call below) seems like it should have a way to trigger a title update, since the visible NavigationEntry changes at that point. Can you look to see if there's a reasonable way to do that? I can try to take a look later for suggestions, but I'm out of time for the moment.
https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2086423005/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:205: void WebContentsEntry::DidAttachInterstitialPage() { On 2016/06/28 00:47:09, Charlie Reis wrote: > On 2016/06/27 14:29:18, afakhry wrote: > > On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > > > Not sure I understand-- why do we need DidAttachInterstitialPage and > > > DidDetachInterstitialPage in addition to the changes to how TitleWasSet is > > > called? > > > > Updating titles here alone is not enough. It will update the title to the link > > "malware.testing.google.test/testing/malware" but not the actual title > "Security > > Error". We can probably skip the title update here, since now we have it in > > TitleWasSet(). > > > > However, we need this for updating the Favicon. > > I think I'm ok with that. Just curious, would we still need it if we put a > UpdateFavicon in TitleWasSet? That might be a bit cleaner and less brittle if > other places trigger TitleWasSet, assuming that titles and favicons often (not > always) change at the same time. No we won't need listening to the attach/detach events if we UpdateFavicon() inside TitleWasSet(). Done. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:677: NavigationEntryImpl* visible_entry = controller_->GetVisibleEntry(); On 2016/06/28 00:47:09, Charlie Reis wrote: > On 2016/06/27 14:29:18, afakhry wrote: > > On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > > > I think this might be wrong. In general, we shouldn't mix values from > > > GetVisibleEntry and GetLastCommittedEntry, which could be two unrelated > pages. > > > > > > Here, I'm not 100% sure this will always return the transient entry and not > > the > > > pending entry, in which case it might put the old title on the pending > entry. > > > Using GetTransientEntry would be safer if that's your intent. > > > > > > That said, I'm a bit skeptical about updating the transient entry as well. > > Why > > > would we need to put the last committed page's title on the transient entry? > > > > We need this if we want TitleWasSet() to be emitted when "Back to saftey" is > > pressed. > > I used the visible entery as I found it to be corresponding to the > interstitial > > page itself when it's being dismissed due to "Back to safety" pressed. I also > > found that the last committed entry corresponds to the page we were at before > we > > navigated to the malware page that resulted in the interstitial showing up. > > I used the transient_entry, as suggested, instead of the visible entry and it > > worked too. > > > > The idea is that I need different entries with different titles, otherwise > > calling UpdateTitleForEntry() will be a no-op here: > > > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > > and here: > > > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > > I feel like we're going about this the wrong way. We shouldn't have to change > the title of the transient entry just before it's deleted to notify observers. > Deleting the transient entry and going back to showing the last committed entry > (on the DiscardNonCommittedEntries call below) seems like it should have a way > to trigger a title update, since the visible NavigationEntry changes at that > point. > > Can you look to see if there's a reasonable way to do that? I can try to take a > look later for suggestions, but I'm out of time for the moment. Yes, it happens as a result of calling: delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL); here: https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... Which updates the title of the view, but doesn't emit TitleWasSet() events. That's why my first suggested solution was to do emit TitleWasSet() from inside NotifyNavigationStateChanged() when INVALIDATE_TYPE_TITLE is one of the flags as mentioned here: https://bugs.chromium.org/p/chromium/issues/detail?id=607074#c6 I'm not sure what to do here since by the time delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL); is called, the transient entry has been discarded already.
Looks like we're getting to the heart of why observing title changes is difficult. :) I have some ideas for how we can move toward a better state below-- let me know what you think. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:304: INVALIDATE_TYPE_TITLE); On 2016/06/27 14:29:18, afakhry wrote: > On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > > One way to make this cleaner would be to move > > NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE) inside > UpdateTitleForEntry. > > All the call sites do it immediately afterward, and it looks like they should > > only do it if it returns true. > > > > If we put that call inside UpdateTitleForEntry, it probably doesn't need a > > return value at all. Also, it seems consistent with the intent of > > UpdateTitleForEntry, which is updating both the entry and the view. > > I made it public, moved the call to NotifyNavigationStateChanged() from > WebContentsImpl::UpdateTitle() to UpdateTitleForEntry(), I didn't change the > return value however, just case it's needed in the future as > UpdateTitleForEntry() maybe a no-op sometimes. If no one is listening to the return value, we should remove it. It's easy to add back if it's needed later. Otherwise it's confusing why it's unused. https://codereview.chromium.org/2086423005/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_impl.cc:677: NavigationEntryImpl* visible_entry = controller_->GetVisibleEntry(); On 2016/06/28 14:34:17, afakhry wrote: > On 2016/06/28 00:47:09, Charlie Reis wrote: > > On 2016/06/27 14:29:18, afakhry wrote: > > > On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > > > > I think this might be wrong. In general, we shouldn't mix values from > > > > GetVisibleEntry and GetLastCommittedEntry, which could be two unrelated > > pages. > > > > > > > > Here, I'm not 100% sure this will always return the transient entry and > not > > > the > > > > pending entry, in which case it might put the old title on the pending > > entry. > > > > Using GetTransientEntry would be safer if that's your intent. > > > > > > > > That said, I'm a bit skeptical about updating the transient entry as well. > > > > Why > > > > would we need to put the last committed page's title on the transient > entry? > > > > > > We need this if we want TitleWasSet() to be emitted when "Back to saftey" is > > > pressed. > > > I used the visible entery as I found it to be corresponding to the > > interstitial > > > page itself when it's being dismissed due to "Back to safety" pressed. I > also > > > found that the last committed entry corresponds to the page we were at > before > > we > > > navigated to the malware page that resulted in the interstitial showing up. > > > I used the transient_entry, as suggested, instead of the visible entry and > it > > > worked too. > > > > > > The idea is that I need different entries with different titles, otherwise > > > calling UpdateTitleForEntry() will be a no-op here: > > > > > > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > > > and here: > > > > > > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > > > > I feel like we're going about this the wrong way. We shouldn't have to change > > the title of the transient entry just before it's deleted to notify observers. > > > Deleting the transient entry and going back to showing the last committed > entry > > (on the DiscardNonCommittedEntries call below) seems like it should have a way > > to trigger a title update, since the visible NavigationEntry changes at that > > point. > > > > Can you look to see if there's a reasonable way to do that? I can try to take > a > > look later for suggestions, but I'm out of time for the moment. > > Yes, it happens as a result of calling: > > delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL); > here: > https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... > > Which updates the title of the view, but doesn't emit TitleWasSet() events. > That's why my first suggested solution was to do emit TitleWasSet() from inside > NotifyNavigationStateChanged() when INVALIDATE_TYPE_TITLE is one of the flags as > mentioned here: https://bugs.chromium.org/p/chromium/issues/detail?id=607074#c6 > > I'm not sure what to do here since by the time > delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL); > is called, the transient entry has been discarded already. I see. This seems to boil down to WebContentsObserver doing a poor job of exposing title change events. TitleWasSet was apparently only meant for cases that a page manually changes its title. Observers have to separately watch for navigation commits (which can also change the title), and don't even have a way to watch for this case, where a visible (transient or pending) entry is canceled and the title goes back to showing the last committed entry. Here, we're kind of adding a hack to make TitleWasSet get fired for the last case in addition to times the title is set manually. Worse, we're doing it by changing the title of the transient entry to an incorrect value just before we throw it away. I see two ways forward: 1) Change the WebContentsObserver API to have a TitleChanged event instead, and fire it in all the cases that the title can change. This means we'll have an extra event on every navigation commit, but it simplifies the cases where people just want to know if the title changes. I'm not sure how much existing code would need updating, but it does seem like a nicer end state. 2) Stick with TitleWasSet for now, and make DiscardNonCommittedEntries fire it if the title was changed as a result. This might require exposing a new method on NavigationControllerDelegate (implemented by WebContents) that fires TitleWasSet, without requiring a change to a NavigationEntry like UpdateTitleForEntry. Also update the documentation of TitleWasSet to say it includes this case but not title changes due to navigation. Option 2 seems a little like we're continuing the hacks, but I suppose it's also a necessary step if we want to do option 1 at some point. That might be sufficient for this CL. What do you think?
Charlie, please take a look. - I moved the UpdateTitleForEntry() to the base WebContents. - Then I replaced (for all Nav Entries that belong to WebContents) the calls NavigationEntry::SetTitle() by WebContents::UpdateTitleForEntry().
This still needs charlie to look at it, but this right approach seems right to me. https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/devtool... File chrome/browser/devtools/devtools_ui_bindings.cc (right): https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/devtool... chrome/browser/devtools/devtools_ui_bindings.cc:538: // DevTools UI is not localized. question for charlie: can we/should we need to do anything to discourage clients from calling SetTitle on the NavigationEntry directly? Can we hide that method from content/public? https://codereview.chromium.org/2086423005/diff/180001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/2086423005/diff/180001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:187: class WebContentsTitleWatcher : public WebContentsObserver { There's a public util class TitleWatcher -- I think you can use it instead. https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.h...
This looks great! The changes in the two task manager files could possibly go in a followup CL, but maybe it's fine to leave them here. (We wouldn't want the UpdateTitleForEntry stuff to get reverted and possibly regress the task manager.) A few small comments below. https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/devtool... File chrome/browser/devtools/devtools_ui_bindings.cc (right): https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/devtool... chrome/browser/devtools/devtools_ui_bindings.cc:538: // DevTools UI is not localized. On 2016/07/13 18:56:02, ncarter wrote: > question for charlie: can we/should we need to do anything to discourage clients > from calling SetTitle on the NavigationEntry directly? Can we hide that method > from content/public? I think we need to keep it for cases that use it before giving the entry to a WebContents, like ContentSerializedNavigationBuilder::ToNavigationEntry. That said, can you add something to the NavigationEntry::SetTitle comment telling people to use UpdateTitleForEntry in most cases, and only call SetTitle directly if it's known to be a non-visible entry? https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/media/t... File chrome/browser/media/tab_desktop_media_list_unittest.cc (right): https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/media/t... chrome/browser/media/tab_desktop_media_list_unittest.cc:329: contents->UpdateTitleForEntry(controller->GetTransientEntry(), I'm pretty confused why this test is using GetTransientEntry, but your change looks correct. I'll contact the test author about it. https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:1204: base::string16()); I think this should be safe. Technically we might not need it in this case because we haven't loaded the page yet-- we're just configuring the NavigationEntry before the load happens (e.g., removing the scroll offset on the line above). We might generate an extra notification as a result of this, but I think that's probably ok. https://codereview.chromium.org/2086423005/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2086423005/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1089: void WebContentsImpl::UpdateTitleForEntry(NavigationEntry* entry, nit: I think it's probably better to leave this in the old location. That would preserve the blame history, and it would make reviewing the changes to it easier as well.
Description was changed from ========== Fix interstitial pages not updating the task manager renderers' titles This ensures that when an interstitial page updates the title of its web contents, a WebContentsObserver::TitleWasSet() event is emitted. Screen recording: https://drive.google.com/a/google.com/file/d/0B6G_-uQnf1_LX0VtUnNWaEd6cUU/vie... BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle() NavigationEntry::SetTitle() doesn't result in the emission of WebContentsObserver::TitleWasSet() events. This used to cause listeners like the task manager to show wrong titles of tabs (for example in the event of an interstitial page). BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by afakhry@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...
Done. I split the task manager changes into its own CL: https://codereview.chromium.org/2148083002 as recommended, and updated the commit message and the CL description of this one. PTAL. https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/devtool... File chrome/browser/devtools/devtools_ui_bindings.cc (right): https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/devtool... chrome/browser/devtools/devtools_ui_bindings.cc:538: // DevTools UI is not localized. On 2016/07/13 21:02:23, Charlie Reis wrote: > On 2016/07/13 18:56:02, ncarter wrote: > > question for charlie: can we/should we need to do anything to discourage > clients > > from calling SetTitle on the NavigationEntry directly? Can we hide that method > > from content/public? > > I think we need to keep it for cases that use it before giving the entry to a > WebContents, like ContentSerializedNavigationBuilder::ToNavigationEntry. > > That said, can you add something to the NavigationEntry::SetTitle comment > telling people to use UpdateTitleForEntry in most cases, and only call SetTitle > directly if it's known to be a non-visible entry? Done, added the comment. https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/media/t... File chrome/browser/media/tab_desktop_media_list_unittest.cc (right): https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/media/t... chrome/browser/media/tab_desktop_media_list_unittest.cc:329: contents->UpdateTitleForEntry(controller->GetTransientEntry(), On 2016/07/13 21:02:23, Charlie Reis wrote: > I'm pretty confused why this test is using GetTransientEntry, but your change > looks correct. I'll contact the test author about it. Acknowledged. https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2086423005/diff/180001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:1204: base::string16()); On 2016/07/13 21:02:23, Charlie Reis wrote: > I think this should be safe. Technically we might not need it in this case > because we haven't loaded the page yet-- we're just configuring the > NavigationEntry before the load happens (e.g., removing the scroll offset on the > line above). We might generate an extra notification as a result of this, but I > think that's probably ok. Acknowledged. https://codereview.chromium.org/2086423005/diff/180001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/2086423005/diff/180001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:187: class WebContentsTitleWatcher : public WebContentsObserver { On 2016/07/13 18:56:02, ncarter wrote: > There's a public util class TitleWatcher -- I think you can use it instead. > https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.h... > Cool, thanks! Done. https://codereview.chromium.org/2086423005/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2086423005/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1089: void WebContentsImpl::UpdateTitleForEntry(NavigationEntry* entry, On 2016/07/13 21:02:23, Charlie Reis wrote: > nit: I think it's probably better to leave this in the old location. That would > preserve the blame history, and it would make reviewing the changes to it easier > as well. Done.
Thanks! LGTM. https://codereview.chromium.org/2086423005/diff/220001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/2086423005/diff/220001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:295: title_update_watcher_->InitWait("TEXT_CHANGED"); Is it still useful to keep the old title_update_watcher_, or is that redundant now? (I don't feel strongly.) https://codereview.chromium.org/2086423005/diff/220001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2086423005/diff/220001/content/public/browser... content/public/browser/navigation_entry.h:90: // Use WebContents::UpdateTitleForEntry() in most cases. Only call the below Let's elaborate a bit: Use WebContents::UpdateTitleForEntry() in most cases, since that notifies observers when the visible title changes. Only call SetTitle directly if this entry is known to not be visible.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
https://codereview.chromium.org/2086423005/diff/220001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl_browsertest.cc (right): https://codereview.chromium.org/2086423005/diff/220001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl_browsertest.cc:295: title_update_watcher_->InitWait("TEXT_CHANGED"); On 2016/07/13 22:21:47, Charlie Reis wrote: > Is it still useful to keep the old title_update_watcher_, or is that redundant > now? (I don't feel strongly.) It's probably redundant. I also don't feel strongly. Even the comment above "InterstitialTitleUpdateWatcher" says: "The existing TitleWatcher does not work for interstitial pages. Note that this title watcher waits for the title update IPC message not the actual title update. So, the new title is probably not propagated completely, yet." Since we fixed this bug, I think it makes sense to remove that old watcher now. Done. https://codereview.chromium.org/2086423005/diff/220001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2086423005/diff/220001/content/public/browser... content/public/browser/navigation_entry.h:90: // Use WebContents::UpdateTitleForEntry() in most cases. Only call the below On 2016/07/13 22:21:47, Charlie Reis wrote: > Let's elaborate a bit: > > Use WebContents::UpdateTitleForEntry() in most cases, since that notifies > observers when the visible title changes. Only call SetTitle directly if this > entry is known to not be visible. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
afakhry@chromium.org changed reviewers: + lazyboy@chromium.org, thestig@chromium.org
lazyboy@chromium.org: Please review changes in extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc thestig@chromium.org: Please review changes in chrome/browser/devtools/devtools_ui_bindings.cc chrome/browser/media/tab_desktop_media_list_unittest.cc chrome/browser/ui/browser_commands.cc chrome/browser/ui/search/search_tab_helper.cc chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc chrome/test/base/browser_with_test_window_test.cc
On 2016/07/13 22:58:28, afakhry wrote: > mailto:thestig@chromium.org: Please review changes in > chrome/browser/devtools/devtools_ui_bindings.cc ... so all of chrome/, ok. :)
lgtm https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/devtool... File chrome/browser/devtools/devtools_ui_bindings.cc (right): https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/devtool... chrome/browser/devtools/devtools_ui_bindings.cc:47: #include "content/public/browser/invalidate_type.h" Remove? https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/media/t... File chrome/browser/media/tab_desktop_media_list_unittest.cc (right): https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/media/t... chrome/browser/media/tab_desktop_media_list_unittest.cc:142: contents->UpdateTitleForEntry(entry, base::UTF8ToUTF16("Test tab")); ASCIIToUTF16() while we are here? Ditto on line 330. https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/media/t... chrome/browser/media/tab_desktop_media_list_unittest.cc:328: content::NavigationController* controller = &contents->GetController(); Just use a const ref? https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... content/public/browser/navigation_entry.h:91: // observers when the visible title changes. Only call the below phrasing: Only call NavigationEntry::SetTitle() below directly ... https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... content/public/browser/web_contents.h:321: // will update history and the view for the new title, and also synthesize "with the new title" ? https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... content/public/browser/web_contents.h:322: // titles for file URLs that have none (so we require that the URL of the I don't quite understand the part in the parenthesis. Does "we" refer to WebContents?
mime_handler_view_guest.cc lgtm
https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/devtool... File chrome/browser/devtools/devtools_ui_bindings.cc (right): https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/devtool... chrome/browser/devtools/devtools_ui_bindings.cc:47: #include "content/public/browser/invalidate_type.h" On 2016/07/13 23:17:17, Lei Zhang wrote: > Remove? Done. https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/media/t... File chrome/browser/media/tab_desktop_media_list_unittest.cc (right): https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/media/t... chrome/browser/media/tab_desktop_media_list_unittest.cc:142: contents->UpdateTitleForEntry(entry, base::UTF8ToUTF16("Test tab")); On 2016/07/13 23:17:17, Lei Zhang wrote: > ASCIIToUTF16() while we are here? Ditto on line 330. Done. https://codereview.chromium.org/2086423005/diff/240001/chrome/browser/media/t... chrome/browser/media/tab_desktop_media_list_unittest.cc:328: content::NavigationController* controller = &contents->GetController(); On 2016/07/13 23:17:17, Lei Zhang wrote: > Just use a const ref? Done. https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... content/public/browser/navigation_entry.h:91: // observers when the visible title changes. Only call the below On 2016/07/13 23:17:17, Lei Zhang wrote: > phrasing: Only call NavigationEntry::SetTitle() below directly ... Done. https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... content/public/browser/web_contents.h:321: // will update history and the view for the new title, and also synthesize On 2016/07/13 23:17:17, Lei Zhang wrote: > "with the new title" ? Done. https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... content/public/browser/web_contents.h:322: // titles for file URLs that have none (so we require that the URL of the On 2016/07/13 23:17:17, Lei Zhang wrote: > I don't quite understand the part in the parenthesis. Does "we" refer to > WebContents? Probably! I just copied this comment from: https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... when I moved it to the base WebContents.
https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... content/public/browser/web_contents.h:322: // titles for file URLs that have none (so we require that the URL of the On 2016/07/13 23:36:17, afakhry wrote: > On 2016/07/13 23:17:17, Lei Zhang wrote: > > I don't quite understand the part in the parenthesis. Does "we" refer to > > WebContents? > > Probably! I just copied this comment from: > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > when I moved it to the base WebContents. Sounds like a good time to fix both. I think it should say: Thus |entry| must have a URL set.
The CQ bit was checked by afakhry@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/2086423005/diff/240001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2086423005/diff/240001/content/public/browser... content/public/browser/web_contents.h:322: // titles for file URLs that have none (so we require that the URL of the On 2016/07/13 23:44:24, Lei Zhang wrote: > On 2016/07/13 23:36:17, afakhry wrote: > > On 2016/07/13 23:17:17, Lei Zhang wrote: > > > I don't quite understand the part in the parenthesis. Does "we" refer to > > > WebContents? > > > > Probably! I just copied this comment from: > > > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > > when I moved it to the base WebContents. > > Sounds like a good time to fix both. I think it should say: > > Thus |entry| must have a URL set. Done. https://codereview.chromium.org/2086423005/diff/280001/chrome/browser/task_ma... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (left): https://codereview.chromium.org/2086423005/diff/280001/chrome/browser/task_ma... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:182: NOTREACHED(); Note the removal of these NOTREACHED()s had to be brought back from the CL that I split from this one: https://codereview.chromium.org/2148083002 We found that it's normal that a WebContents may have a title even though it's crashed and we're not tracking it.
On 2016/07/14 00:01:53, afakhry wrote: > On 2016/07/13 23:44:24, Lei Zhang wrote: > > Sounds like a good time to fix both. I think it should say: > > > > Thus |entry| must have a URL set. > > Done. Thanks for all the cleanup.
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 afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, thestig@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2086423005/#ps280001 (title: "Improving comment and removing the NOTREACHED.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Message was sent while issue was closed.
Description was changed from ========== Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle() NavigationEntry::SetTitle() doesn't result in the emission of WebContentsObserver::TitleWasSet() events. This used to cause listeners like the task manager to show wrong titles of tabs (for example in the event of an interstitial page). BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle() NavigationEntry::SetTitle() doesn't result in the emission of WebContentsObserver::TitleWasSet() events. This used to cause listeners like the task manager to show wrong titles of tabs (for example in the event of an interstitial page). BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle() NavigationEntry::SetTitle() doesn't result in the emission of WebContentsObserver::TitleWasSet() events. This used to cause listeners like the task manager to show wrong titles of tabs (for example in the event of an interstitial page). BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle() NavigationEntry::SetTitle() doesn't result in the emission of WebContentsObserver::TitleWasSet() events. This used to cause listeners like the task manager to show wrong titles of tabs (for example in the event of an interstitial page). BUG=607074 TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/6f0c1ec23bc1eabe221fccdb4b28b1d96273a705 Cr-Commit-Position: refs/heads/master@{#405485} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/6f0c1ec23bc1eabe221fccdb4b28b1d96273a705 Cr-Commit-Position: refs/heads/master@{#405485} |
