NTP: simplify mojo connection setup

chrome/browser/ui/search/search_ipc_router_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/search/search_ipc_router.h"
 
 #include <stdint.h>
 
 #include <memory>
 #include <tuple>
@@ skipping 165 matching lines @@
   }
 
   MockSearchBox* mock_search_box() { return &mock_search_box_; }
 
  private:
   MockSearchIPCRouterDelegate delegate_;
   base::FieldTrialList field_trial_list_;
   MockSearchBox mock_search_box_;
 };
 
-TEST_F(SearchIPCRouterTest, IgnoreMessagesFromNonInstantRenderers) {
-  NavigateAndCommitActiveTab(GURL("file://foo/bar"));
-  SetupMockDelegateAndPolicy();
-  EXPECT_CALL(*mock_delegate(), FocusOmnibox(OMNIBOX_FOCUS_VISIBLE)).Times(0);
-  content::WebContents* contents = web_contents();
-  bool is_active_tab = IsActiveTab(contents);
-  EXPECT_TRUE(is_active_tab);
-
-  MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
-  EXPECT_CALL(*policy, ShouldProcessFocusOmnibox(is_active_tab)).Times(0);
-
-  GetSearchIPCRouter().FocusOmnibox(GetSearchIPCRouterSeqNo(),
-                                    OMNIBOX_FOCUS_VISIBLE);
-}
-
 TEST_F(SearchIPCRouterTest, ProcessFocusOmniboxMsg) {
   NavigateAndCommitActiveTab(GURL(chrome::kChromeSearchLocalNtpUrl));
   SetupMockDelegateAndPolicy();
   MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
   EXPECT_CALL(*mock_delegate(), FocusOmnibox(OMNIBOX_FOCUS_VISIBLE)).Times(1);
 
   content::WebContents* contents = web_contents();
   bool is_active_tab = IsActiveTab(contents);
   EXPECT_TRUE(is_active_tab);
   EXPECT_CALL(*policy, ShouldProcessFocusOmnibox(is_active_tab)).Times(1)
@@ skipping 216 matching lines @@
   NavigateAndCommitActiveTab(GURL("chrome-search://foo/bar"));
   SetupMockDelegateAndPolicy();
   MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
   EXPECT_CALL(*mock_delegate(), OnUndoAllMostVisitedDeletions()).Times(0);
   EXPECT_CALL(*policy, ShouldProcessUndoAllMostVisitedDeletions()).Times(1)
       .WillOnce(testing::Return(false));
 
   GetSearchIPCRouter().UndoAllMostVisitedDeletions(GetSearchIPCRouterSeqNo());
 }
 
-TEST_F(SearchIPCRouterTest, IgnoreMessageIfThePageIsNotActive) {
-  NavigateAndCommitActiveTab(GURL(chrome::kChromeSearchLocalNtpUrl));
-  SetupMockDelegateAndPolicy();
-  MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
-  int page_seq_no = GetSearchIPCRouterSeqNo();
-
-  content::WebContents* contents = web_contents();
-  bool is_active_tab = IsActiveTab(contents);
-  GURL item_url("www.foo.com");
-
-  // Navigate away from the NTP. Afterwards, all messages should be ignored.

[Marc Treib, 2017/02/13 09:35:57]

It's not clear to me why it's safe to remove these tests. Unless there are other
tests covering the behavior, we should add some.

[tibell, 2017/02/19 23:18:45]

Mojo being connection oriented helps us here. If we start in the state where the
main frame of the NTP has a connection to the SearchIPCRouter and we navigate
away from the NTP then 1) the old frame gets destroyed and all its Mojo
connections with it and 2) the new frame is not allowed to set up a connection,
because it fails the IsInInstantProcess test.

What we could test is that a non-instant frame isn't allowed to connect to the
SearchIPCRouter. I'm still relatively new to Chrome so I'm not sure how to set
up such a test using our testing framework. Thinking about this it seems like a
browser test wouldn't work, because the way we instantiate the SearchBox ensures
it doesn't get attached to a non-instant frame to begin with. We'd need to write
a test that creates a "custom" frame that is non-instant but still has a
SearchBox attached to it. Any idea how to do that?

[Marc Treib, 2017/02/20 10:14:52]

That sounds reasonable (to the engineer who has no idea about IPC/Mojo :D).
I'm not really familiar enough with Mojo and the whole IPC setup to advise here,
but it seems like it should be possible to just manually create a SearchBox
instance for a given RenderFrame? Though after your explanations, I'm not sure
how useful such a test would be.

In related news, I've been working on adding some end-to-end integration test
for the NTP, including the embeddedSearch APIs, see e.g.
https://codereview.chromium.org/2695813012/ (pending). It should be fairly easy
to add a test that navigates to an NTP and away again, and makes sure the APIs
are only available on the NTP.
(Doesn't have to happen in this CL since it's somewhat orthogonal. But it would
be great to have this sort of coverage before making more/bigger changes to the
IPC setup. WDYT?)

-  NavigateAndCommitActiveTab(item_url);
-
-  EXPECT_CALL(*mock_delegate(), OnDeleteMostVisitedItem(item_url)).Times(0);
-  EXPECT_CALL(*policy, ShouldProcessDeleteMostVisitedItem()).Times(0);
-  GetSearchIPCRouter().DeleteMostVisitedItem(page_seq_no, item_url);
-
-  EXPECT_CALL(*mock_delegate(), OnUndoMostVisitedDeletion(item_url)).Times(0);
-  EXPECT_CALL(*policy, ShouldProcessUndoMostVisitedDeletion()).Times(0);
-  GetSearchIPCRouter().UndoMostVisitedDeletion(page_seq_no, item_url);
-
-  EXPECT_CALL(*mock_delegate(), OnUndoAllMostVisitedDeletions()).Times(0);
-  EXPECT_CALL(*policy, ShouldProcessUndoAllMostVisitedDeletions()).Times(0);
-  GetSearchIPCRouter().UndoAllMostVisitedDeletions(page_seq_no);
-
-  EXPECT_CALL(*mock_delegate(), FocusOmnibox(OMNIBOX_FOCUS_VISIBLE)).Times(0);
-  EXPECT_CALL(*policy, ShouldProcessFocusOmnibox(is_active_tab)).Times(0);
-  GetSearchIPCRouter().FocusOmnibox(page_seq_no, OMNIBOX_FOCUS_VISIBLE);
-
-  base::TimeDelta delta = base::TimeDelta::FromMilliseconds(123);
-  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_ALL_TILES_LOADED, delta))
-      .Times(0);
-  EXPECT_CALL(*policy, ShouldProcessLogEvent()).Times(0);
-  GetSearchIPCRouter().LogEvent(page_seq_no, NTP_ALL_TILES_LOADED, delta);
-
-  base::string16 text;
-  EXPECT_CALL(*mock_delegate(), PasteIntoOmnibox(text)).Times(0);
-  EXPECT_CALL(*policy, ShouldProcessPasteIntoOmnibox(is_active_tab)).Times(0);
-  GetSearchIPCRouter().PasteAndOpenDropdown(page_seq_no, text);
-}
-
 TEST_F(SearchIPCRouterTest, ProcessPasteAndOpenDropdownMsg) {
   NavigateAndCommitActiveTab(GURL(chrome::kChromeSearchLocalNtpUrl));
   SetupMockDelegateAndPolicy();
   MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
 
   content::WebContents* contents = web_contents();
   bool is_active_tab = IsActiveTab(contents);
   EXPECT_TRUE(is_active_tab);
 
   base::string16 text;
@@ skipping 150 matching lines @@
 TEST_F(SearchIPCRouterTest, DoNotSendSubmitMsg) {
   NavigateAndCommitActiveTab(GURL(chrome::kChromeSearchLocalNtpUrl));
   SetupMockDelegateAndPolicy();
   MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
   EXPECT_CALL(*policy, ShouldSubmitQuery()).Times(1)
       .WillOnce(testing::Return(false));
 
   EXPECT_CALL(*mock_search_box(), Submit(_, _)).Times(0);
   GetSearchIPCRouter().Submit(base::string16(), EmbeddedSearchRequestParams());
 }