Fix some unit_tests under PlzNavigate

Fix some unit_tests under PlzNavigate

I started by trying to fix ToolbarModelTest.ShouldDisplayURL but it
turned out that the combination of initializing the test utils, and the
simulation of the RFH unloading fixed the following:

    BookmarkTest.DetachedBookmarkBarOnNTP
    BrowserBookmarkBarTest.StateOnActiveTabChanged
    BrowserCommandsTest.ViewSource
    BrowserInstantControllerTest.DefaultSearchProviderChanged
    BrowserInstantControllerTest.GoogleBaseURLUpdated
    BrowserViewTest.BrowserViewLayout
    InstantNTPURLRewriteTest.UberURLHandler_InstantExtendedNewTabPage
    SearchIPCRouterPolicyTest.ProcessChromeIdentityCheck
    SearchIPCRouterPolicyTest.ProcessDeleteMostVisitedItem
    SearchIPCRouterPolicyTest.ProcessFocusOmnibox
    SearchIPCRouterPolicyTest.ProcessHistorySyncCheck
    SearchIPCRouterPolicyTest.ProcessLogEvent
    SearchIPCRouterPolicyTest.ProcessPasteIntoOmniboxMsg
    SearchIPCRouterPolicyTest.ProcessUndoAllMostVisitedDeletions
    SearchIPCRouterPolicyTest.ProcessUndoMostVisitedDeletion
    SearchIPCRouterPolicyTest.SendMostVisitedItems
    SearchIPCRouterPolicyTest.SendThemeBackgroundInfo
    SearchIPCRouterTest.IgnoreChromeIdentityCheckMsg
    SearchIPCRouterTest.IgnoreDeleteMostVisitedItemMsg
    SearchIPCRouterTest.IgnoreFocusOmniboxMsg
    SearchIPCRouterTest.IgnoreHistorySyncCheckMsg
    SearchIPCRouterTest.IgnoreLogEventMsg
    SearchIPCRouterTest.IgnorePasteAndOpenDropdownMsg
    SearchIPCRouterTest.IgnoreUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.IgnoreUndoMostVisitedDeletionMsg
    SearchIPCRouterTest.ProcessChromeIdentityCheckMsg
    SearchIPCRouterTest.ProcessDeleteMostVisitedItemMsg
    SearchIPCRouterTest.ProcessFocusOmniboxMsg
    SearchIPCRouterTest.ProcessHistorySyncCheckMsg
    SearchIPCRouterTest.ProcessLogEventMsg
    SearchIPCRouterTest.ProcessLogMostVisitedImpressionMsg
    SearchIPCRouterTest.ProcessLogMostVisitedNavigationMsg
    SearchIPCRouterTest.ProcessPasteAndOpenDropdownMsg
    SearchIPCRouterTest.ProcessUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.ProcessUndoMostVisitedDeletionMsg
    SearchTest.InstantCacheableNTPNavigationEntry
    SearchTest.InstantCacheableNTPNavigationEntryNewProfile
    SearchTest.InstantNTPCustomNavigationEntry
    SearchTest.InstantNTPExtendedEnabled
    SearchTest.ProcessIsolation
    SearchTest.ProcessIsolation_RendererInitiated
    SessionsSyncManagerTest.AssociateWindowsDontReloadTabs
    ToolbarModelTest.ShouldDisplayURL

R=clamy@chromium.org, nasko@chromium.org
BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d2b6c5d77bb3643a84b829d51614d126b10d5d42
Cr-Commit-Position: refs/heads/master@{#416003}

[scottmg, 2016-08-29 22:33:54]

Description was changed from

==========
Fix ToolbarModelTest.ShouldDisplayURL in unit_tests when running
--enable-browser-side-navigation

BUG=510836
==========

to

==========
Fix ToolbarModelTest.ShouldDisplayURL in unit_tests when running
--enable-browser-side-navigation

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2016-08-30 03:56:01]

Description was changed from

==========
Fix ToolbarModelTest.ShouldDisplayURL in unit_tests when running
--enable-browser-side-navigation

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix ToolbarModelTest.ShouldDisplayURL in unit_tests under PlzNavigate

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2016-08-30 17:19:51]

Description was changed from

==========
Fix ToolbarModelTest.ShouldDisplayURL in unit_tests under PlzNavigate

BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix some unit_tests under PlzNavigate

I started by trying to fix ToolbarModelTest.ShouldDisplayURL but it
turned out that the combination of initializing the test utils, and the
simulation of the RFH unloading fixed the following:

    BookmarkTest.DetachedBookmarkBarOnNTP
    BrowserBookmarkBarTest.StateOnActiveTabChanged
    BrowserCommandsTest.ViewSource
    BrowserInstantControllerTest.DefaultSearchProviderChanged
    BrowserInstantControllerTest.GoogleBaseURLUpdated
    BrowserViewTest.BrowserViewLayout
    InstantNTPURLRewriteTest.UberURLHandler_InstantExtendedNewTabPage
    SearchIPCRouterPolicyTest.ProcessChromeIdentityCheck
    SearchIPCRouterPolicyTest.ProcessDeleteMostVisitedItem
    SearchIPCRouterPolicyTest.ProcessFocusOmnibox
    SearchIPCRouterPolicyTest.ProcessHistorySyncCheck
    SearchIPCRouterPolicyTest.ProcessLogEvent
    SearchIPCRouterPolicyTest.ProcessPasteIntoOmniboxMsg
    SearchIPCRouterPolicyTest.ProcessUndoAllMostVisitedDeletions
    SearchIPCRouterPolicyTest.ProcessUndoMostVisitedDeletion
    SearchIPCRouterPolicyTest.SendMostVisitedItems
    SearchIPCRouterPolicyTest.SendThemeBackgroundInfo
    SearchIPCRouterTest.IgnoreChromeIdentityCheckMsg
    SearchIPCRouterTest.IgnoreDeleteMostVisitedItemMsg
    SearchIPCRouterTest.IgnoreFocusOmniboxMsg
    SearchIPCRouterTest.IgnoreHistorySyncCheckMsg
    SearchIPCRouterTest.IgnoreLogEventMsg
    SearchIPCRouterTest.IgnorePasteAndOpenDropdownMsg
    SearchIPCRouterTest.IgnoreUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.IgnoreUndoMostVisitedDeletionMsg
    SearchIPCRouterTest.ProcessChromeIdentityCheckMsg
    SearchIPCRouterTest.ProcessDeleteMostVisitedItemMsg
    SearchIPCRouterTest.ProcessFocusOmniboxMsg
    SearchIPCRouterTest.ProcessHistorySyncCheckMsg
    SearchIPCRouterTest.ProcessLogEventMsg
    SearchIPCRouterTest.ProcessLogMostVisitedImpressionMsg
    SearchIPCRouterTest.ProcessLogMostVisitedNavigationMsg
    SearchIPCRouterTest.ProcessPasteAndOpenDropdownMsg
    SearchIPCRouterTest.ProcessUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.ProcessUndoMostVisitedDeletionMsg
    SearchTest.InstantCacheableNTPNavigationEntry
    SearchTest.InstantCacheableNTPNavigationEntryNewProfile
    SearchTest.InstantNTPCustomNavigationEntry
    SearchTest.InstantNTPExtendedEnabled
    SearchTest.ProcessIsolation
    SearchTest.ProcessIsolation_RendererInitiated
    SessionsSyncManagerTest.AssociateWindowsDontReloadTabs
    ToolbarModelTest.ShouldDisplayURL

R=clamy@chromium.org, nasko@chromium.org
BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2016-08-30 17:26:55]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[scottmg, 2016-08-30 17:27:09]

Description was changed from

==========
Fix some unit_tests under PlzNavigate

I started by trying to fix ToolbarModelTest.ShouldDisplayURL but it
turned out that the combination of initializing the test utils, and the
simulation of the RFH unloading fixed the following:

    BookmarkTest.DetachedBookmarkBarOnNTP
    BrowserBookmarkBarTest.StateOnActiveTabChanged
    BrowserCommandsTest.ViewSource
    BrowserInstantControllerTest.DefaultSearchProviderChanged
    BrowserInstantControllerTest.GoogleBaseURLUpdated
    BrowserViewTest.BrowserViewLayout
    InstantNTPURLRewriteTest.UberURLHandler_InstantExtendedNewTabPage
    SearchIPCRouterPolicyTest.ProcessChromeIdentityCheck
    SearchIPCRouterPolicyTest.ProcessDeleteMostVisitedItem
    SearchIPCRouterPolicyTest.ProcessFocusOmnibox
    SearchIPCRouterPolicyTest.ProcessHistorySyncCheck
    SearchIPCRouterPolicyTest.ProcessLogEvent
    SearchIPCRouterPolicyTest.ProcessPasteIntoOmniboxMsg
    SearchIPCRouterPolicyTest.ProcessUndoAllMostVisitedDeletions
    SearchIPCRouterPolicyTest.ProcessUndoMostVisitedDeletion
    SearchIPCRouterPolicyTest.SendMostVisitedItems
    SearchIPCRouterPolicyTest.SendThemeBackgroundInfo
    SearchIPCRouterTest.IgnoreChromeIdentityCheckMsg
    SearchIPCRouterTest.IgnoreDeleteMostVisitedItemMsg
    SearchIPCRouterTest.IgnoreFocusOmniboxMsg
    SearchIPCRouterTest.IgnoreHistorySyncCheckMsg
    SearchIPCRouterTest.IgnoreLogEventMsg
    SearchIPCRouterTest.IgnorePasteAndOpenDropdownMsg
    SearchIPCRouterTest.IgnoreUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.IgnoreUndoMostVisitedDeletionMsg
    SearchIPCRouterTest.ProcessChromeIdentityCheckMsg
    SearchIPCRouterTest.ProcessDeleteMostVisitedItemMsg
    SearchIPCRouterTest.ProcessFocusOmniboxMsg
    SearchIPCRouterTest.ProcessHistorySyncCheckMsg
    SearchIPCRouterTest.ProcessLogEventMsg
    SearchIPCRouterTest.ProcessLogMostVisitedImpressionMsg
    SearchIPCRouterTest.ProcessLogMostVisitedNavigationMsg
    SearchIPCRouterTest.ProcessPasteAndOpenDropdownMsg
    SearchIPCRouterTest.ProcessUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.ProcessUndoMostVisitedDeletionMsg
    SearchTest.InstantCacheableNTPNavigationEntry
    SearchTest.InstantCacheableNTPNavigationEntryNewProfile
    SearchTest.InstantNTPCustomNavigationEntry
    SearchTest.InstantNTPExtendedEnabled
    SearchTest.ProcessIsolation
    SearchTest.ProcessIsolation_RendererInitiated
    SessionsSyncManagerTest.AssociateWindowsDontReloadTabs
    ToolbarModelTest.ShouldDisplayURL

R=clamy@chromium.org, nasko@chromium.org
BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix some unit_tests under PlzNavigate

I started by trying to fix ToolbarModelTest.ShouldDisplayURL but it
turned out that the combination of initializing the test utils, and the
simulation of the RFH unloading fixed the following:

    BookmarkTest.DetachedBookmarkBarOnNTP
    BrowserBookmarkBarTest.StateOnActiveTabChanged
    BrowserCommandsTest.ViewSource
    BrowserInstantControllerTest.DefaultSearchProviderChanged
    BrowserInstantControllerTest.GoogleBaseURLUpdated
    BrowserViewTest.BrowserViewLayout
    InstantNTPURLRewriteTest.UberURLHandler_InstantExtendedNewTabPage
    SearchIPCRouterPolicyTest.ProcessChromeIdentityCheck
    SearchIPCRouterPolicyTest.ProcessDeleteMostVisitedItem
    SearchIPCRouterPolicyTest.ProcessFocusOmnibox
    SearchIPCRouterPolicyTest.ProcessHistorySyncCheck
    SearchIPCRouterPolicyTest.ProcessLogEvent
    SearchIPCRouterPolicyTest.ProcessPasteIntoOmniboxMsg
    SearchIPCRouterPolicyTest.ProcessUndoAllMostVisitedDeletions
    SearchIPCRouterPolicyTest.ProcessUndoMostVisitedDeletion
    SearchIPCRouterPolicyTest.SendMostVisitedItems
    SearchIPCRouterPolicyTest.SendThemeBackgroundInfo
    SearchIPCRouterTest.IgnoreChromeIdentityCheckMsg
    SearchIPCRouterTest.IgnoreDeleteMostVisitedItemMsg
    SearchIPCRouterTest.IgnoreFocusOmniboxMsg
    SearchIPCRouterTest.IgnoreHistorySyncCheckMsg
    SearchIPCRouterTest.IgnoreLogEventMsg
    SearchIPCRouterTest.IgnorePasteAndOpenDropdownMsg
    SearchIPCRouterTest.IgnoreUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.IgnoreUndoMostVisitedDeletionMsg
    SearchIPCRouterTest.ProcessChromeIdentityCheckMsg
    SearchIPCRouterTest.ProcessDeleteMostVisitedItemMsg
    SearchIPCRouterTest.ProcessFocusOmniboxMsg
    SearchIPCRouterTest.ProcessHistorySyncCheckMsg
    SearchIPCRouterTest.ProcessLogEventMsg
    SearchIPCRouterTest.ProcessLogMostVisitedImpressionMsg
    SearchIPCRouterTest.ProcessLogMostVisitedNavigationMsg
    SearchIPCRouterTest.ProcessPasteAndOpenDropdownMsg
    SearchIPCRouterTest.ProcessUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.ProcessUndoMostVisitedDeletionMsg
    SearchTest.InstantCacheableNTPNavigationEntry
    SearchTest.InstantCacheableNTPNavigationEntryNewProfile
    SearchTest.InstantNTPCustomNavigationEntry
    SearchTest.InstantNTPExtendedEnabled
    SearchTest.ProcessIsolation
    SearchTest.ProcessIsolation_RendererInitiated
    SessionsSyncManagerTest.AssociateWindowsDontReloadTabs
    ToolbarModelTest.ShouldDisplayURL

R=clamy@chromium.org, nasko@chromium.org
BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2016-08-30 17:27:09]

scottmg@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[commit-bot: I haz the power, 2016-08-30 17:27:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-08-30 18:02:40]

scottmg@chromium.org changed reviewers:
+ sky@chromium.org

[scottmg, 2016-08-30 18:03:10]

scottmg@chromium.org changed reviewers:
- sky@chromium.org

[scottmg, 2016-08-30 18:15:31]

https://codereview.chromium.org/2296483002/diff/80001/content/public/test/bro...
File content/public/test/browser_side_navigation_test_utils.cc (right):

https://codereview.chromium.org/2296483002/diff/80001/content/public/test/bro...
content/public/test/browser_side_navigation_test_utils.cc:83:
main_frame->PrepareForCommit();
I had to move this file into content/public/test so that it could use
FrameTreeNode::PrepareForCommit().

[commit-bot: I haz the power, 2016-08-30 18:36:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 18:36:17]

Dry run: This issue passed the CQ dry run.

[scottmg, 2016-08-30 20:00:35]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 20:01:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2016-08-30 21:17:47]

Thanks! Good find!

https://codereview.chromium.org/2296483002/diff/100001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2296483002/diff/100001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:147:
RenderFrameHostTester::For(old_rfh)->SendBeforeUnloadACK(true);
PrepareForCommit actually takes care fo sending the BeforeUnloadACK. So if we
call SimulateCurrentRenderFrameHostUnloading which calls PrepareForCommit, we no
longer need to call SendBeforeUnloadACK here.

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/br...
File content/public/test/browser_side_navigation_test_utils.h (right):

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/br...
content/public/test/browser_side_navigation_test_utils.h:52: void
SimulateCurrentRenderFrameHostUnloading(RenderFrameHost* rfh);
Could we just expose PrepareForCommit in the RenderFrameHostTester? This way
it's clearer it's the same function used in unit_tests & content_unittests.

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/te...
content/public/test/test_renderer_host.cc:64: return
web_contents->GetRenderManagerForTesting()
Which test need to get access to the pending RFH?

@nasko: for a long time we did not want to expose the speculative RFH outside of
content, since it is an implementation detail that may go away. Are we okay to
do so now?

[scottmg, 2016-08-30 21:47:43]

Patchset #7 (id:120001) has been deleted

[scottmg, 2016-08-30 21:55:27]

Thanks!

https://codereview.chromium.org/2296483002/diff/100001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2296483002/diff/100001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:147:
RenderFrameHostTester::For(old_rfh)->SendBeforeUnloadACK(true);
On 2016/08/30 21:17:47, clamy wrote:
> PrepareForCommit actually takes care fo sending the BeforeUnloadACK. So if we
> call SimulateCurrentRenderFrameHostUnloading which calls PrepareForCommit, we
no
> longer need to call SendBeforeUnloadACK here.

Thanks, done.

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/br...
File content/public/test/browser_side_navigation_test_utils.h (right):

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/br...
content/public/test/browser_side_navigation_test_utils.h:52: void
SimulateCurrentRenderFrameHostUnloading(RenderFrameHost* rfh);
On 2016/08/30 21:17:47, clamy wrote:
> Could we just expose PrepareForCommit in the RenderFrameHostTester? This way
> it's clearer it's the same function used in unit_tests & content_unittests.

Aha, much better! Done.

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/te...
content/public/test/test_renderer_host.cc:64: return
web_contents->GetRenderManagerForTesting()
On 2016/08/30 21:17:47, clamy wrote:
> Which test need to get access to the pending RFH?
> 
> @nasko: for a long time we did not want to expose the speculative RFH outside
of
> content, since it is an implementation detail that may go away. Are we okay to
> do so now?

That was for ToolbarModelTest (and maybe others), via
https://cs.chromium.org/chromium/src/chrome/test/base/browser_with_test_windo...
otherwise the wrong url was active here
https://cs.chromium.org/chromium/src/chrome/browser/ui/toolbar/toolbar_model_...
. I don't full understand the details, so let me know if that doesn't seem to
make much sense and I'll dig in more.

[scottmg, 2016-08-30 22:34:44]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 22:35:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2016-08-30 23:08:38]

Thanks!

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/2296483002/diff/100001/content/public/test/te...
content/public/test/test_renderer_host.cc:64: return
web_contents->GetRenderManagerForTesting()
On 2016/08/30 21:55:27, scottmg wrote:
> On 2016/08/30 21:17:47, clamy wrote:
> > Which test need to get access to the pending RFH?
> > 
> > @nasko: for a long time we did not want to expose the speculative RFH
outside
> of
> > content, since it is an implementation detail that may go away. Are we okay
to
> > do so now?
> 
> That was for ToolbarModelTest (and maybe others), via
>
https://cs.chromium.org/chromium/src/chrome/test/base/browser_with_test_windo...
> otherwise the wrong url was active here
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/toolbar/toolbar_model_...
> . I don't full understand the details, so let me know if that doesn't seem to
> make much sense and I'll dig in more.

As explained in the new patchset, I think we can actually use
WebContentsTester::CommitPendingNavigation in BrowserWithTestWindowTest, which
should remove the need for accessing the pending RFH in that function.

https://codereview.chromium.org/2296483002/diff/160001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2296483002/diff/160001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:150: if (pending_rfh)
Actually we should not get a SwapOutACK here since we move unload to happen in
the background (3 years ago). While we're at it, we should probably remove that
call as well.

And while I look at it, I wonder why we try to reimplement the navigations call
here, instead of using WebcontentsTester::CommitPendingNavigation. Unless we
have a very good reason, I think we should do that, this will avoid the
simulation of navigation calls getting out-of-date with what actually happens in
content/.

https://codereview.chromium.org/2296483002/diff/160001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2296483002/diff/160001/content/public/test/te...
content/public/test/test_renderer_host.h:99: // If we are doing a cross-site
navigation, this simulates the current RFH
nit: s/RFH/RenderFrameHost here and below.

https://codereview.chromium.org/2296483002/diff/160001/content/public/test/te...
content/public/test/test_renderer_host.h:102: // PlzNavigate: the pending RFH is
not created before the navigation commit,
This comment from "the pending RFH" to "deleted in the meantime" actually
belongs to where it was before. It describes why in this specific function we
need to call PrepareForCommit before trying to access the main or pending RFH.

For the comment, I would suggest getting the one from
TestRenderFrameHost::PrepareForCommit, and keeping most of the last paragraph.

https://codereview.chromium.org/2296483002/diff/160001/content/public/test/te...
content/public/test/test_renderer_host.h:112: // urls, etc.) there will be no
NavigationRequest, and no simulation of the
Please remove the mention of NavigationRequest, since this is not a class we
expose outside of content/

https://codereview.chromium.org/2296483002/diff/160001/content/test/test_rend...
File content/test/test_render_frame_host.h (right):

https://codereview.chromium.org/2296483002/diff/160001/content/test/test_rend...
content/test/test_render_frame_host.h:66: void SimulateUnloading() override;
I'm not a big fan of the name SimulateUnloading, since
a) we simulate BeforeUnload and not Unload
b) we simulate a lot more than that in the PlzNavigate case.

I don't have a good suggestion though. I've been thinking of
PrepareForCommitIfNecessary or SimulateCommitableState (which is what the
function is attempting to do) but that doesn't sound super good.

[scottmg, 2016-08-30 23:55:30]

https://codereview.chromium.org/2296483002/diff/160001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2296483002/diff/160001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:150: if (pending_rfh)
On 2016/08/30 23:08:38, clamy wrote:
> Actually we should not get a SwapOutACK here since we move unload to happen in
> the background (3 years ago). While we're at it, we should probably remove
that
> call as well.

Removed.

> 
> And while I look at it, I wonder why we try to reimplement the navigations
call
> here, instead of using WebcontentsTester::CommitPendingNavigation. Unless we
> have a very good reason, I think we should do that, this will avoid the
> simulation of navigation calls getting out-of-date with what actually happens
in
> content/.

I'm... not sure! :)

BrowserWithTestWindowTest doesn't currently seem to use TestWebContents, instead
creating just a plain WebContentsImpl.

>	content.dll!content::WebContentsImpl::WebContentsImpl(content::BrowserContext
* browser_context) Line 441	C++
 	content.dll!content::WebContentsImpl::CreateWithOpener(const
content::WebContents::CreateParams & params, content::FrameTreeNode * opener)
Line 559	C++
 	content.dll!content::WebContents::Create(const
content::WebContents::CreateParams & params) Line 255	C++
 	unit_tests.exe!`anonymous namespace'::CreateTargetContents(const
chrome::NavigateParams & params, const GURL & url) Line 359	C++
 	unit_tests.exe!chrome::Navigate(chrome::NavigateParams * params) Line 510	C++
 	unit_tests.exe!BrowserWithTestWindowTest::AddTab(Browser * browser, const GURL
& url) Line 129	C++
 	unit_tests.exe!ToolbarModelTest_ShouldDisplayURL_Test::TestBody() Line 141	C++

Your suggestion seems totally plausible, but it would affect what looks like
~200+ tests, so I'm a little hesitant given my current and on-going ignorance.
:)

I'll start looking into switching BrowserWithTestWindowTest to use
TestWebContents if that seems like the right thing to do, but please let me know
if that's not what you were thinking.

https://codereview.chromium.org/2296483002/diff/160001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2296483002/diff/160001/content/public/test/te...
content/public/test/test_renderer_host.h:99: // If we are doing a cross-site
navigation, this simulates the current RFH
On 2016/08/30 23:08:38, clamy wrote:
> nit: s/RFH/RenderFrameHost here and below.

Done.

https://codereview.chromium.org/2296483002/diff/160001/content/public/test/te...
content/public/test/test_renderer_host.h:102: // PlzNavigate: the pending RFH is
not created before the navigation commit,
On 2016/08/30 23:08:38, clamy wrote:
> This comment from "the pending RFH" to "deleted in the meantime" actually
> belongs to where it was before. It describes why in this specific function we
> need to call PrepareForCommit before trying to access the main or pending RFH.
> 
> For the comment, I would suggest getting the one from
> TestRenderFrameHost::PrepareForCommit, and keeping most of the last paragraph.

Done.

https://codereview.chromium.org/2296483002/diff/160001/content/public/test/te...
content/public/test/test_renderer_host.h:112: // urls, etc.) there will be no
NavigationRequest, and no simulation of the
On 2016/08/30 23:08:38, clamy wrote:
> Please remove the mention of NavigationRequest, since this is not a class we
> expose outside of content/

Done.

https://codereview.chromium.org/2296483002/diff/160001/content/test/test_rend...
File content/test/test_render_frame_host.h (right):

https://codereview.chromium.org/2296483002/diff/160001/content/test/test_rend...
content/test/test_render_frame_host.h:66: void SimulateUnloading() override;
On 2016/08/30 23:08:38, clamy wrote:
> I'm not a big fan of the name SimulateUnloading, since
> a) we simulate BeforeUnload and not Unload
> b) we simulate a lot more than that in the PlzNavigate case.
> 
> I don't have a good suggestion though. I've been thinking of
> PrepareForCommitIfNecessary or SimulateCommitableState (which is what the
> function is attempting to do) but that doesn't sound super good.

I went with PrepareForCommitIfNecessary(). It's not pretty, but it's at least
explanatory I guess.

[scottmg, 2016-08-31 01:55:18]

https://codereview.chromium.org/2296483002/diff/160001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2296483002/diff/160001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:150: if (pending_rfh)
On 2016/08/30 23:55:30, scottmg wrote:
> On 2016/08/30 23:08:38, clamy wrote:
> > Actually we should not get a SwapOutACK here since we move unload to happen
in
> > the background (3 years ago). While we're at it, we should probably remove
> that
> > call as well.
> 
> Removed.
> 
> > 
> > And while I look at it, I wonder why we try to reimplement the navigations
> call
> > here, instead of using WebcontentsTester::CommitPendingNavigation. Unless we
> > have a very good reason, I think we should do that, this will avoid the
> > simulation of navigation calls getting out-of-date with what actually
happens
> in
> > content/.
> 
> I'm... not sure! :)
> 
> BrowserWithTestWindowTest doesn't currently seem to use TestWebContents,
instead
> creating just a plain WebContentsImpl.
> 
>
>	content.dll!content::WebContentsImpl::WebContentsImpl(content::BrowserContext
> * browser_context) Line 441	C++
>  	content.dll!content::WebContentsImpl::CreateWithOpener(const
> content::WebContents::CreateParams & params, content::FrameTreeNode * opener)
> Line 559	C++
>  	content.dll!content::WebContents::Create(const
> content::WebContents::CreateParams & params) Line 255	C++
>  	unit_tests.exe!`anonymous namespace'::CreateTargetContents(const
> chrome::NavigateParams & params, const GURL & url) Line 359	C++
>  	unit_tests.exe!chrome::Navigate(chrome::NavigateParams * params) Line
510	C++
>  	unit_tests.exe!BrowserWithTestWindowTest::AddTab(Browser * browser, const
GURL
> & url) Line 129	C++
>  	unit_tests.exe!ToolbarModelTest_ShouldDisplayURL_Test::TestBody() Line
141	C++
> 
> Your suggestion seems totally plausible, but it would affect what looks like
> ~200+ tests, so I'm a little hesitant given my current and on-going ignorance.
> :)
> 
> I'll start looking into switching BrowserWithTestWindowTest to use
> TestWebContents if that seems like the right thing to do, but please let me
know
> if that's not what you were thinking.

I spent some trying trying to get this to work by making
BrowserWithTestWindowTest::AddTab() manually create and install a
TestWebContents rather than using chrome::Navigate() (which makes a
WebContentsImpl), and having BrowserWithTestWindowTest::CommitPendingLoad() just
call

  content::WebContentsTester::For(controller->GetWebContents())
      ->CommitPendingNavigation();

But that makes quite a few tests fail (even in non-PlzNavigate)
https://gist.github.com/sgraham/cce3dc8e1aceffe4dffbb2d130df89db. Looking at
some of them, prerender and print preview both make a new (non-Test)
WebContents, so can't use TestWebContents::CommitPendingNavigation().

I can start trying to make some of those tests create TestWebContents instead,
but I'm not sure this the best use of time in the context of this change. WDYT?

[clamy, 2016-08-31 18:23:21]

Thanks! A few more comments below, but it's mostly good apart from that.

https://codereview.chromium.org/2296483002/diff/160001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2296483002/diff/160001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:150: if (pending_rfh)
On 2016/08/31 01:55:18, scottmg wrote:
> On 2016/08/30 23:55:30, scottmg wrote:
> > On 2016/08/30 23:08:38, clamy wrote:
> > > Actually we should not get a SwapOutACK here since we move unload to
happen
> in
> > > the background (3 years ago). While we're at it, we should probably remove
> > that
> > > call as well.
> > 
> > Removed.
> > 
> > > 
> > > And while I look at it, I wonder why we try to reimplement the navigations
> > call
> > > here, instead of using WebcontentsTester::CommitPendingNavigation. Unless
we
> > > have a very good reason, I think we should do that, this will avoid the
> > > simulation of navigation calls getting out-of-date with what actually
> happens
> > in
> > > content/.
> > 
> > I'm... not sure! :)
> > 
> > BrowserWithTestWindowTest doesn't currently seem to use TestWebContents,
> instead
> > creating just a plain WebContentsImpl.
> > 
> >
>
>	content.dll!content::WebContentsImpl::WebContentsImpl(content::BrowserContext
> > * browser_context) Line 441	C++
> >  	content.dll!content::WebContentsImpl::CreateWithOpener(const
> > content::WebContents::CreateParams & params, content::FrameTreeNode *
opener)
> > Line 559	C++
> >  	content.dll!content::WebContents::Create(const
> > content::WebContents::CreateParams & params) Line 255	C++
> >  	unit_tests.exe!`anonymous namespace'::CreateTargetContents(const
> > chrome::NavigateParams & params, const GURL & url) Line 359	C++
> >  	unit_tests.exe!chrome::Navigate(chrome::NavigateParams * params) Line
> 510	C++
> >  	unit_tests.exe!BrowserWithTestWindowTest::AddTab(Browser * browser, const
> GURL
> > & url) Line 129	C++
> >  	unit_tests.exe!ToolbarModelTest_ShouldDisplayURL_Test::TestBody() Line
> 141	C++
> > 
> > Your suggestion seems totally plausible, but it would affect what looks like
> > ~200+ tests, so I'm a little hesitant given my current and on-going
ignorance.
> > :)
> > 
> > I'll start looking into switching BrowserWithTestWindowTest to use
> > TestWebContents if that seems like the right thing to do, but please let me
> know
> > if that's not what you were thinking.
> 
> I spent some trying trying to get this to work by making
> BrowserWithTestWindowTest::AddTab() manually create and install a
> TestWebContents rather than using chrome::Navigate() (which makes a
> WebContentsImpl), and having BrowserWithTestWindowTest::CommitPendingLoad()
just
> call
> 
>   content::WebContentsTester::For(controller->GetWebContents())
>       ->CommitPendingNavigation();
> 
> But that makes quite a few tests fail (even in non-PlzNavigate)
> https://gist.github.com/sgraham/cce3dc8e1aceffe4dffbb2d130df89db. Looking at
> some of them, prerender and print preview both make a new (non-Test)
> WebContents, so can't use TestWebContents::CommitPendingNavigation().
> 
> I can start trying to make some of those tests create TestWebContents instead,
> but I'm not sure this the best use of time in the context of this change.
WDYT?
> 

Ok I understand why this here now. It's a bit unfortunate, but I agree that this
is not the best use of time. Let's keep it that way, and add a TODO that it
would be better to use WebContentsTester::CommitPendingNavigation instead.

https://codereview.chromium.org/2296483002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/2296483002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:527: friend class
RenderFrameHostTester;
Why is this needed?

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
content/public/test/test_renderer_host.h:100: // RenderFrameHost notifying that
it has unloaded so the pending
nit: s/it has unloaded/BeforeUnload has executed

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
content/public/test/test_renderer_host.h:101: // RenderFrameHost is resumed and
can navigate. Note that for some synchronous
The Note... sentence seems to belong to the PlzNavigate part.

[nasko, 2016-08-31 18:39:05]

Just one suggestion around the exposure of the speculative RFH.

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
content/public/test/test_renderer_host.cc:59: RenderFrameHost*
RenderFrameHostTester::GetPendingForController(
Considering that only browser_with_test_window_test.cc is the user of this
method, can't we just eliminate it in general or just invoke a content-exposed
version of what it does? I saw the discussion about areas of code not using
TestWebContents, but these are already hacks, so why not expose this through
this object (RenderFrameHostTester)?

[scottmg, 2016-08-31 19:01:47]

https://codereview.chromium.org/2296483002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/2296483002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:527: friend class
RenderFrameHostTester;
On 2016/08/31 18:23:21, clamy wrote:
> Why is this needed?

This is for RenderFrameHostTester::GetPendingForController in
content/public/test/test_renderer_host.cc, for speculative_render_frame_host_.

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
content/public/test/test_renderer_host.cc:59: RenderFrameHost*
RenderFrameHostTester::GetPendingForController(
On 2016/08/31 18:39:05, nasko (slow) wrote:
> Considering that only browser_with_test_window_test.cc is the user of this
> method, can't we just eliminate it in general or just invoke a content-exposed
> version of what it does? I saw the discussion about areas of code not using
> TestWebContents, but these are already hacks, so why not expose this through
> this object (RenderFrameHostTester)?

Ah, I didn't notice it was the only user.

I'm not sure I understood the various "this"es in your comment. :) Do you mean
having a helper method here that does CommitPendingLoad's work for a given
controller so that it's sort of inside content at least?

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
content/public/test/test_renderer_host.h:100: // RenderFrameHost notifying that
it has unloaded so the pending
On 2016/08/31 18:23:21, clamy wrote:
> nit: s/it has unloaded/BeforeUnload has executed

Done.

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
content/public/test/test_renderer_host.h:101: // RenderFrameHost is resumed and
can navigate. Note that for some synchronous
On 2016/08/31 18:23:21, clamy wrote:
> The Note... sentence seems to belong to the PlzNavigate part.

Done.

[scottmg, 2016-08-31 19:40:34]

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/2296483002/diff/180001/content/public/test/te...
content/public/test/test_renderer_host.cc:59: RenderFrameHost*
RenderFrameHostTester::GetPendingForController(
On 2016/08/31 19:01:46, scottmg wrote:
> On 2016/08/31 18:39:05, nasko (slow) wrote:
> > Considering that only browser_with_test_window_test.cc is the user of this
> > method, can't we just eliminate it in general or just invoke a
content-exposed
> > version of what it does? I saw the discussion about areas of code not using
> > TestWebContents, but these are already hacks, so why not expose this through
> > this object (RenderFrameHostTester)?
> 
> Ah, I didn't notice it was the only user.
> 
> I'm not sure I understood the various "this"es in your comment. :) Do you mean
> having a helper method here that does CommitPendingLoad's work for a given
> controller so that it's sort of inside content at least?

Did ... something like that. (maybe?)

[nasko, 2016-08-31 19:45:24]

https://codereview.chromium.org/2296483002/diff/220001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2296483002/diff/220001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:138:
RenderFrameHostTester::CommitPendingLoad(controller);
Awesome! This (pun intended) was exactly what I had in mind. Thanks for being a
great mind-reader for folks like me that can't put coherent english together :).

https://codereview.chromium.org/2296483002/diff/220001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/2296483002/diff/220001/content/public/test/te...
content/public/test/test_renderer_host.cc:66: // trivial to do so, so for now we
have this extra simulation for
nit: maybe drop one of the "so" in the "so, so" : ).

[scottmg, 2016-08-31 20:10:36]

https://codereview.chromium.org/2296483002/diff/220001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/2296483002/diff/220001/content/public/test/te...
content/public/test/test_renderer_host.cc:66: // trivial to do so, so for now we
have this extra simulation for
On 2016/08/31 19:45:24, nasko (slow) wrote:
> nit: maybe drop one of the "so" in the "so, so" : ).

Replaced with "this, this". ;)

(Rewrote it a bit to hopefully make things more clear.)

[nasko, 2016-08-31 20:26:44]

LGTM with a nit.

https://codereview.chromium.org/2296483002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/2296483002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:527: friend class
RenderFrameHostTester;
On 2016/08/31 19:01:46, scottmg wrote:
> On 2016/08/31 18:23:21, clamy wrote:
> > Why is this needed?
> 
> This is for RenderFrameHostTester::GetPendingForController in
> content/public/test/test_renderer_host.cc, for speculative_render_frame_host_.

With your latest patch, GetPendingForController is gone. This can go too, right?

[scottmg, 2016-08-31 21:05:48]

https://codereview.chromium.org/2296483002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/2296483002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:527: friend class
RenderFrameHostTester;
On 2016/08/31 20:26:43, nasko (slow) wrote:
> On 2016/08/31 19:01:46, scottmg wrote:
> > On 2016/08/31 18:23:21, clamy wrote:
> > > Why is this needed?
> > 
> > This is for RenderFrameHostTester::GetPendingForController in
> > content/public/test/test_renderer_host.cc, for
speculative_render_frame_host_.
> 
> With your latest patch, GetPendingForController is gone. This can go too,
right?

No, unfortunately RenderFrameHostTester::CommitPendingLoad() needs to do the
same thing.

[scottmg, 2016-08-31 21:59:21]

scottmg@chromium.org changed reviewers:
+ sky@chromium.org

[scottmg, 2016-08-31 21:59:23]

+sky for chrome/test/base/browser_with_test_window_test.cc OWNERS

[scottmg, 2016-08-31 21:59:39]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 22:00:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 23:18:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 23:18:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-08-31 23:34:57]

LGTM

[clamy, 2016-09-01 00:00:19]

Thanks! It's nice we could get rid of exposing the pending RFH. One comment
below, since the use of the test API has changed.

https://codereview.chromium.org/2296483002/diff/240001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2296483002/diff/240001/content/public/test/te...
content/public/test/test_renderer_host.h:99: virtual void
PrepareForCommitIfNecessary() = 0;
We're no longer using this outside of content/ right? If so, it should be
removed from the public API.

[scottmg, 2016-09-01 02:33:19]

https://codereview.chromium.org/2296483002/diff/240001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2296483002/diff/240001/content/public/test/te...
content/public/test/test_renderer_host.h:99: virtual void
PrepareForCommitIfNecessary() = 0;
On 2016/09/01 00:00:18, clamy wrote:
> We're no longer using this outside of content/ right? If so, it should be
> removed from the public API.

Ah, right. Done.

[scottmg, 2016-09-01 02:34:46]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 02:35:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 04:37:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 04:37:05]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-09-01 18:27:58]

Thanks! Lgtm.

[scottmg, 2016-09-01 18:31:04]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-09-01 18:31:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2296483002/#ps260001
(title: "not in public interface")

[commit-bot: I haz the power, 2016-09-01 18:31:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-09-01 18:33:25]

Thanks for all the help on the back-and-forth here!

[commit-bot: I haz the power, 2016-09-01 18:36:42]

Description was changed from

==========
Fix some unit_tests under PlzNavigate

I started by trying to fix ToolbarModelTest.ShouldDisplayURL but it
turned out that the combination of initializing the test utils, and the
simulation of the RFH unloading fixed the following:

    BookmarkTest.DetachedBookmarkBarOnNTP
    BrowserBookmarkBarTest.StateOnActiveTabChanged
    BrowserCommandsTest.ViewSource
    BrowserInstantControllerTest.DefaultSearchProviderChanged
    BrowserInstantControllerTest.GoogleBaseURLUpdated
    BrowserViewTest.BrowserViewLayout
    InstantNTPURLRewriteTest.UberURLHandler_InstantExtendedNewTabPage
    SearchIPCRouterPolicyTest.ProcessChromeIdentityCheck
    SearchIPCRouterPolicyTest.ProcessDeleteMostVisitedItem
    SearchIPCRouterPolicyTest.ProcessFocusOmnibox
    SearchIPCRouterPolicyTest.ProcessHistorySyncCheck
    SearchIPCRouterPolicyTest.ProcessLogEvent
    SearchIPCRouterPolicyTest.ProcessPasteIntoOmniboxMsg
    SearchIPCRouterPolicyTest.ProcessUndoAllMostVisitedDeletions
    SearchIPCRouterPolicyTest.ProcessUndoMostVisitedDeletion
    SearchIPCRouterPolicyTest.SendMostVisitedItems
    SearchIPCRouterPolicyTest.SendThemeBackgroundInfo
    SearchIPCRouterTest.IgnoreChromeIdentityCheckMsg
    SearchIPCRouterTest.IgnoreDeleteMostVisitedItemMsg
    SearchIPCRouterTest.IgnoreFocusOmniboxMsg
    SearchIPCRouterTest.IgnoreHistorySyncCheckMsg
    SearchIPCRouterTest.IgnoreLogEventMsg
    SearchIPCRouterTest.IgnorePasteAndOpenDropdownMsg
    SearchIPCRouterTest.IgnoreUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.IgnoreUndoMostVisitedDeletionMsg
    SearchIPCRouterTest.ProcessChromeIdentityCheckMsg
    SearchIPCRouterTest.ProcessDeleteMostVisitedItemMsg
    SearchIPCRouterTest.ProcessFocusOmniboxMsg
    SearchIPCRouterTest.ProcessHistorySyncCheckMsg
    SearchIPCRouterTest.ProcessLogEventMsg
    SearchIPCRouterTest.ProcessLogMostVisitedImpressionMsg
    SearchIPCRouterTest.ProcessLogMostVisitedNavigationMsg
    SearchIPCRouterTest.ProcessPasteAndOpenDropdownMsg
    SearchIPCRouterTest.ProcessUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.ProcessUndoMostVisitedDeletionMsg
    SearchTest.InstantCacheableNTPNavigationEntry
    SearchTest.InstantCacheableNTPNavigationEntryNewProfile
    SearchTest.InstantNTPCustomNavigationEntry
    SearchTest.InstantNTPExtendedEnabled
    SearchTest.ProcessIsolation
    SearchTest.ProcessIsolation_RendererInitiated
    SessionsSyncManagerTest.AssociateWindowsDontReloadTabs
    ToolbarModelTest.ShouldDisplayURL

R=clamy@chromium.org, nasko@chromium.org
BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix some unit_tests under PlzNavigate

I started by trying to fix ToolbarModelTest.ShouldDisplayURL but it
turned out that the combination of initializing the test utils, and the
simulation of the RFH unloading fixed the following:

    BookmarkTest.DetachedBookmarkBarOnNTP
    BrowserBookmarkBarTest.StateOnActiveTabChanged
    BrowserCommandsTest.ViewSource
    BrowserInstantControllerTest.DefaultSearchProviderChanged
    BrowserInstantControllerTest.GoogleBaseURLUpdated
    BrowserViewTest.BrowserViewLayout
    InstantNTPURLRewriteTest.UberURLHandler_InstantExtendedNewTabPage
    SearchIPCRouterPolicyTest.ProcessChromeIdentityCheck
    SearchIPCRouterPolicyTest.ProcessDeleteMostVisitedItem
    SearchIPCRouterPolicyTest.ProcessFocusOmnibox
    SearchIPCRouterPolicyTest.ProcessHistorySyncCheck
    SearchIPCRouterPolicyTest.ProcessLogEvent
    SearchIPCRouterPolicyTest.ProcessPasteIntoOmniboxMsg
    SearchIPCRouterPolicyTest.ProcessUndoAllMostVisitedDeletions
    SearchIPCRouterPolicyTest.ProcessUndoMostVisitedDeletion
    SearchIPCRouterPolicyTest.SendMostVisitedItems
    SearchIPCRouterPolicyTest.SendThemeBackgroundInfo
    SearchIPCRouterTest.IgnoreChromeIdentityCheckMsg
    SearchIPCRouterTest.IgnoreDeleteMostVisitedItemMsg
    SearchIPCRouterTest.IgnoreFocusOmniboxMsg
    SearchIPCRouterTest.IgnoreHistorySyncCheckMsg
    SearchIPCRouterTest.IgnoreLogEventMsg
    SearchIPCRouterTest.IgnorePasteAndOpenDropdownMsg
    SearchIPCRouterTest.IgnoreUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.IgnoreUndoMostVisitedDeletionMsg
    SearchIPCRouterTest.ProcessChromeIdentityCheckMsg
    SearchIPCRouterTest.ProcessDeleteMostVisitedItemMsg
    SearchIPCRouterTest.ProcessFocusOmniboxMsg
    SearchIPCRouterTest.ProcessHistorySyncCheckMsg
    SearchIPCRouterTest.ProcessLogEventMsg
    SearchIPCRouterTest.ProcessLogMostVisitedImpressionMsg
    SearchIPCRouterTest.ProcessLogMostVisitedNavigationMsg
    SearchIPCRouterTest.ProcessPasteAndOpenDropdownMsg
    SearchIPCRouterTest.ProcessUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.ProcessUndoMostVisitedDeletionMsg
    SearchTest.InstantCacheableNTPNavigationEntry
    SearchTest.InstantCacheableNTPNavigationEntryNewProfile
    SearchTest.InstantNTPCustomNavigationEntry
    SearchTest.InstantNTPExtendedEnabled
    SearchTest.ProcessIsolation
    SearchTest.ProcessIsolation_RendererInitiated
    SessionsSyncManagerTest.AssociateWindowsDontReloadTabs
    ToolbarModelTest.ShouldDisplayURL

R=clamy@chromium.org, nasko@chromium.org
BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-01 18:36:43]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-09-01 18:38:20]

Description was changed from

==========
Fix some unit_tests under PlzNavigate

I started by trying to fix ToolbarModelTest.ShouldDisplayURL but it
turned out that the combination of initializing the test utils, and the
simulation of the RFH unloading fixed the following:

    BookmarkTest.DetachedBookmarkBarOnNTP
    BrowserBookmarkBarTest.StateOnActiveTabChanged
    BrowserCommandsTest.ViewSource
    BrowserInstantControllerTest.DefaultSearchProviderChanged
    BrowserInstantControllerTest.GoogleBaseURLUpdated
    BrowserViewTest.BrowserViewLayout
    InstantNTPURLRewriteTest.UberURLHandler_InstantExtendedNewTabPage
    SearchIPCRouterPolicyTest.ProcessChromeIdentityCheck
    SearchIPCRouterPolicyTest.ProcessDeleteMostVisitedItem
    SearchIPCRouterPolicyTest.ProcessFocusOmnibox
    SearchIPCRouterPolicyTest.ProcessHistorySyncCheck
    SearchIPCRouterPolicyTest.ProcessLogEvent
    SearchIPCRouterPolicyTest.ProcessPasteIntoOmniboxMsg
    SearchIPCRouterPolicyTest.ProcessUndoAllMostVisitedDeletions
    SearchIPCRouterPolicyTest.ProcessUndoMostVisitedDeletion
    SearchIPCRouterPolicyTest.SendMostVisitedItems
    SearchIPCRouterPolicyTest.SendThemeBackgroundInfo
    SearchIPCRouterTest.IgnoreChromeIdentityCheckMsg
    SearchIPCRouterTest.IgnoreDeleteMostVisitedItemMsg
    SearchIPCRouterTest.IgnoreFocusOmniboxMsg
    SearchIPCRouterTest.IgnoreHistorySyncCheckMsg
    SearchIPCRouterTest.IgnoreLogEventMsg
    SearchIPCRouterTest.IgnorePasteAndOpenDropdownMsg
    SearchIPCRouterTest.IgnoreUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.IgnoreUndoMostVisitedDeletionMsg
    SearchIPCRouterTest.ProcessChromeIdentityCheckMsg
    SearchIPCRouterTest.ProcessDeleteMostVisitedItemMsg
    SearchIPCRouterTest.ProcessFocusOmniboxMsg
    SearchIPCRouterTest.ProcessHistorySyncCheckMsg
    SearchIPCRouterTest.ProcessLogEventMsg
    SearchIPCRouterTest.ProcessLogMostVisitedImpressionMsg
    SearchIPCRouterTest.ProcessLogMostVisitedNavigationMsg
    SearchIPCRouterTest.ProcessPasteAndOpenDropdownMsg
    SearchIPCRouterTest.ProcessUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.ProcessUndoMostVisitedDeletionMsg
    SearchTest.InstantCacheableNTPNavigationEntry
    SearchTest.InstantCacheableNTPNavigationEntryNewProfile
    SearchTest.InstantNTPCustomNavigationEntry
    SearchTest.InstantNTPExtendedEnabled
    SearchTest.ProcessIsolation
    SearchTest.ProcessIsolation_RendererInitiated
    SessionsSyncManagerTest.AssociateWindowsDontReloadTabs
    ToolbarModelTest.ShouldDisplayURL

R=clamy@chromium.org, nasko@chromium.org
BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix some unit_tests under PlzNavigate

I started by trying to fix ToolbarModelTest.ShouldDisplayURL but it
turned out that the combination of initializing the test utils, and the
simulation of the RFH unloading fixed the following:

    BookmarkTest.DetachedBookmarkBarOnNTP
    BrowserBookmarkBarTest.StateOnActiveTabChanged
    BrowserCommandsTest.ViewSource
    BrowserInstantControllerTest.DefaultSearchProviderChanged
    BrowserInstantControllerTest.GoogleBaseURLUpdated
    BrowserViewTest.BrowserViewLayout
    InstantNTPURLRewriteTest.UberURLHandler_InstantExtendedNewTabPage
    SearchIPCRouterPolicyTest.ProcessChromeIdentityCheck
    SearchIPCRouterPolicyTest.ProcessDeleteMostVisitedItem
    SearchIPCRouterPolicyTest.ProcessFocusOmnibox
    SearchIPCRouterPolicyTest.ProcessHistorySyncCheck
    SearchIPCRouterPolicyTest.ProcessLogEvent
    SearchIPCRouterPolicyTest.ProcessPasteIntoOmniboxMsg
    SearchIPCRouterPolicyTest.ProcessUndoAllMostVisitedDeletions
    SearchIPCRouterPolicyTest.ProcessUndoMostVisitedDeletion
    SearchIPCRouterPolicyTest.SendMostVisitedItems
    SearchIPCRouterPolicyTest.SendThemeBackgroundInfo
    SearchIPCRouterTest.IgnoreChromeIdentityCheckMsg
    SearchIPCRouterTest.IgnoreDeleteMostVisitedItemMsg
    SearchIPCRouterTest.IgnoreFocusOmniboxMsg
    SearchIPCRouterTest.IgnoreHistorySyncCheckMsg
    SearchIPCRouterTest.IgnoreLogEventMsg
    SearchIPCRouterTest.IgnorePasteAndOpenDropdownMsg
    SearchIPCRouterTest.IgnoreUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.IgnoreUndoMostVisitedDeletionMsg
    SearchIPCRouterTest.ProcessChromeIdentityCheckMsg
    SearchIPCRouterTest.ProcessDeleteMostVisitedItemMsg
    SearchIPCRouterTest.ProcessFocusOmniboxMsg
    SearchIPCRouterTest.ProcessHistorySyncCheckMsg
    SearchIPCRouterTest.ProcessLogEventMsg
    SearchIPCRouterTest.ProcessLogMostVisitedImpressionMsg
    SearchIPCRouterTest.ProcessLogMostVisitedNavigationMsg
    SearchIPCRouterTest.ProcessPasteAndOpenDropdownMsg
    SearchIPCRouterTest.ProcessUndoAllMostVisitedDeletionsMsg
    SearchIPCRouterTest.ProcessUndoMostVisitedDeletionMsg
    SearchTest.InstantCacheableNTPNavigationEntry
    SearchTest.InstantCacheableNTPNavigationEntryNewProfile
    SearchTest.InstantNTPCustomNavigationEntry
    SearchTest.InstantNTPExtendedEnabled
    SearchTest.ProcessIsolation
    SearchTest.ProcessIsolation_RendererInitiated
    SessionsSyncManagerTest.AssociateWindowsDontReloadTabs
    ToolbarModelTest.ShouldDisplayURL

R=clamy@chromium.org, nasko@chromium.org
BUG=510836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d2b6c5d77bb3643a84b829d51614d126b10d5d42
Cr-Commit-Position: refs/heads/master@{#416003}
==========

[commit-bot: I haz the power, 2016-09-01 18:38:22]

Patchset 13 (id:??) landed as
https://crrev.com/d2b6c5d77bb3643a84b829d51614d126b10d5d42
Cr-Commit-Position: refs/heads/master@{#416003}