Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
591483002:
Only send C2F ping for a search through homepage. (Closed)
DescriptionOnly send C2F ping for a search through homepage.
BUG=8424708
Committed: https://crrev.com/9b9d2e0a172bf3c3592fa6543c59b61277f20e2f
Cr-Commit-Position: refs/heads/master@{#312421}
Patch Set 1 #Patch Set 2 : Fix tests #
Total comments: 15
Patch Set 3 : Address comments #Patch Set 4 : Address comments. #Patch Set 5 : Remove unused #include. #Patch Set 6 : Add the testHelper class as a member instead of inheritating. #Patch Set 7 : Rebase #Patch Set 8 : Move setup out of constructor. #Patch Set 9 : Set enable_rlz's default back to 0. #Patch Set 10 : Fix a bug. #
Total comments: 2
Patch Set 11 : Remove the dependency on content/test. #Patch Set 12 : Rebase #
Messages
Total messages: 42 (21 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
yiyaoliu@chromium.org changed reviewers: + rogerta@chromium.org
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
Thanks Yiyao. A few questions below. https://codereview.chromium.org/591483002/diff/260001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/591483002/diff/260001/build/common.gypi#newco... build/common.gypi:1458: 'enable_rlz%': 1, Make sure to take this out before commit. https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.... chrome/browser/rlz/rlz.cc:433: } No need for { and } https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.... chrome/browser/rlz/rlz.cc:455: ui::PAGE_TRANSITION_HOME_PAGE) != 0)) { Should we also check that the previous entry is a google search url? https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.... chrome/browser/rlz/rlz.cc:459: } Should all the return statements above be breaks? https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz_... File chrome/browser/rlz/rlz_unittest.cc (right): https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz_... chrome/browser/rlz/rlz_unittest.cc:293: // Then simulate a search from homepage. Between the two SendNavigateWithTransition(), would it be good to have a call to: ExpectEventRecorded(kHomepageFirstSeach, false); https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... File rlz/test/rlz_test_helpers.cc (right): https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... rlz/test/rlz_test_helpers.cc:139: } I think it would be better to put all the code above into a helper function that can be called from c/b/rlz_unittest.cc, instead of just exposing InitializeRegistryOverridesForTesting(). Same for TearDown(). You could call them something like: SetUpMachineStateForRlzTesting( registry_util::RegistryOverrideManager* override_manager); TearDownMachineStateForRlzTesting();
Patchset #3 (id:280001) has been deleted
https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.... chrome/browser/rlz/rlz.cc:433: } On 2014/09/30 14:43:35, Roger Tawa wrote: > No need for { and } Done. https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.... chrome/browser/rlz/rlz.cc:455: ui::PAGE_TRANSITION_HOME_PAGE) != 0)) { On 2014/09/30 14:43:35, Roger Tawa wrote: > Should we also check that the previous entry is a google search url? hm.. but the previous entry should not be a google search. in an extreme case, the homepage itself could be set to a google search url, and the user did another search on that page. In this case, we could check to make sure it's a google homepage, i.e. some form of google.xxx but not a search? https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.... chrome/browser/rlz/rlz.cc:459: } On 2014/09/30 14:43:35, Roger Tawa wrote: > Should all the return statements above be breaks? Done. https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz_... File chrome/browser/rlz/rlz_unittest.cc (right): https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz_... chrome/browser/rlz/rlz_unittest.cc:293: // Then simulate a search from homepage. On 2014/09/30 14:43:35, Roger Tawa wrote: > Between the two SendNavigateWithTransition(), would it be good to have a call > to: > > ExpectEventRecorded(kHomepageFirstSeach, false); hm.. but kHomepageFirstSeach is defined after this function. I could separate this function into 2 functions, one SimulateHomePage and one SimulateSearch, and have the check done in the test. WDYT? https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... File rlz/test/rlz_test_helpers.cc (right): https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... rlz/test/rlz_test_helpers.cc:139: } On 2014/09/30 14:43:35, Roger Tawa wrote: > I think it would be better to put all the code above into a helper function that > can be called from c/b/rlz_unittest.cc, instead of just exposing > InitializeRegistryOverridesForTesting(). Same for TearDown(). You could call > them something like: > > SetUpMachineStateForRlzTesting( > registry_util::RegistryOverrideManager* override_manager); > TearDownMachineStateForRlzTesting(); Are you saying we should have an overloading function SetUpMachineStateForRlzTesting that takes in different parameters? A separate question, what is "base::mac::ScopedNSAutoreleasePool pool" there for? since I don't see it being used anywhere.
https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.... chrome/browser/rlz/rlz.cc:455: ui::PAGE_TRANSITION_HOME_PAGE) != 0)) { On 2014/09/30 17:10:44, yao wrote: > On 2014/09/30 14:43:35, Roger Tawa wrote: > > Should we also check that the previous entry is a google search url? > > hm.. but the previous entry should not be a google search. > > in an extreme case, the homepage itself could be set to a google search url, and > the user did another search on that page. In this case, we could check to make > sure it's a google homepage, i.e. some form of google.xxx but not a search? How about the case where foo.com is the home page, and then the user clicks on a google.com link. Will that be counted as a first search? I guess the exact pattern we are looking for is a google *search-result* page following a google search or search-result page, where the previous page is marked as HOME_PAGE. Does that make sense? https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... File rlz/test/rlz_test_helpers.cc (right): https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... rlz/test/rlz_test_helpers.cc:139: } On 2014/09/30 17:10:44, yao wrote: > On 2014/09/30 14:43:35, Roger Tawa wrote: > > I think it would be better to put all the code above into a helper function > that > > can be called from c/b/rlz_unittest.cc, instead of just exposing > > InitializeRegistryOverridesForTesting(). Same for TearDown(). You could call > > them something like: > > > > SetUpMachineStateForRlzTesting( > > registry_util::RegistryOverrideManager* override_manager); > > TearDownMachineStateForRlzTesting(); > > Are you saying we should have an overloading function > SetUpMachineStateForRlzTesting that takes in different parameters? > > A separate question, what is "base::mac::ScopedNSAutoreleasePool pool" there > for? since I don't see it being used anywhere. No overload, because RlzLibTest no longer derives from RlzLibTestNoMachineState. What I mean is move all the code from lines 130 to 138 into SetUpMachineStateForRlzTesting(). Then RlzLibTestNoMachineState::SetUp() can call that function, and RlzLibTest can also call that function from line 202 in rlz_unittest.cc. Similar pattern for TearDown(). Wrt ScopedNSAutoreleasePool, good question. Maybe Alexei can tell you, or Nico (thakis@).
https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... File rlz/test/rlz_test_helpers.cc (right): https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... rlz/test/rlz_test_helpers.cc:139: } On 2014/09/30 18:24:36, Roger Tawa wrote: > On 2014/09/30 17:10:44, yao wrote: > > On 2014/09/30 14:43:35, Roger Tawa wrote: > > > I think it would be better to put all the code above into a helper function > > that > > > can be called from c/b/rlz_unittest.cc, instead of just exposing > > > InitializeRegistryOverridesForTesting(). Same for TearDown(). You could > call > > > them something like: > > > > > > SetUpMachineStateForRlzTesting( > > > registry_util::RegistryOverrideManager* override_manager); > > > TearDownMachineStateForRlzTesting(); > > > > Are you saying we should have an overloading function > > SetUpMachineStateForRlzTesting that takes in different parameters? > > > > A separate question, what is "base::mac::ScopedNSAutoreleasePool pool" there > > for? since I don't see it being used anywhere. > > No overload, because RlzLibTest no longer derives from RlzLibTestNoMachineState. > > What I mean is move all the code from lines 130 to 138 into > SetUpMachineStateForRlzTesting(). Then RlzLibTestNoMachineState::SetUp() can > call that function, and RlzLibTest can also call that function from line 202 in > rlz_unittest.cc. > > Similar pattern for TearDown(). > > Wrt ScopedNSAutoreleasePool, good question. Maybe Alexei can tell you, or Nico > (thakis@). hm... maybe I'm confused, but how are we planning to pass the private members to that function?
On 2014/09/30 18:30:01, yao wrote: > https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... > File rlz/test/rlz_test_helpers.cc (right): > > https://codereview.chromium.org/591483002/diff/260001/rlz/test/rlz_test_helpe... > rlz/test/rlz_test_helpers.cc:139: } > On 2014/09/30 18:24:36, Roger Tawa wrote: > > On 2014/09/30 17:10:44, yao wrote: > > > On 2014/09/30 14:43:35, Roger Tawa wrote: > > > > I think it would be better to put all the code above into a helper > function > > > that > > > > can be called from c/b/rlz_unittest.cc, instead of just exposing > > > > InitializeRegistryOverridesForTesting(). Same for TearDown(). You could > > call > > > > them something like: > > > > > > > > SetUpMachineStateForRlzTesting( > > > > registry_util::RegistryOverrideManager* override_manager); > > > > TearDownMachineStateForRlzTesting(); > > > > > > Are you saying we should have an overloading function > > > SetUpMachineStateForRlzTesting that takes in different parameters? > > > > > > A separate question, what is "base::mac::ScopedNSAutoreleasePool pool" there > > > for? since I don't see it being used anywhere. > > > > No overload, because RlzLibTest no longer derives from > RlzLibTestNoMachineState. > > > > What I mean is move all the code from lines 130 to 138 into > > SetUpMachineStateForRlzTesting(). Then RlzLibTestNoMachineState::SetUp() can > > call that function, and RlzLibTest can also call that function from line 202 > in > > rlz_unittest.cc. > > > > Similar pattern for TearDown(). > > > > Wrt ScopedNSAutoreleasePool, good question. Maybe Alexei can tell you, or > Nico > > (thakis@). > > hm... maybe I'm confused, but how are we planning to pass the private members to > that function? By pointer. See the signature of SetUpMachineStateForRlzTesting(). Just like today the code passes the private member to InitializeRegistryOverridesForTesting().
Hi Roger, Sorry for the delay. I thought I already sent it back to you, and when I check morning, I didn't :/ Updated as you suggested. https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.cc File chrome/browser/rlz/rlz.cc (right): https://codereview.chromium.org/591483002/diff/260001/chrome/browser/rlz/rlz.... chrome/browser/rlz/rlz.cc:455: ui::PAGE_TRANSITION_HOME_PAGE) != 0)) { On 2014/09/30 18:24:36, Roger Tawa wrote: > On 2014/09/30 17:10:44, yao wrote: > > On 2014/09/30 14:43:35, Roger Tawa wrote: > > > Should we also check that the previous entry is a google search url? > > > > hm.. but the previous entry should not be a google search. > > > > in an extreme case, the homepage itself could be set to a google search url, > and > > the user did another search on that page. In this case, we could check to make > > sure it's a google homepage, i.e. some form of google.xxx but not a search? > > How about the case where http://foo.com is the home page, and then the user clicks on a > http://google.com link. Will that be counted as a first search? I guess the exact > pattern we are looking for is a google *search-result* page following a google > search or search-result page, where the previous page is marked as HOME_PAGE. > Does that make sense? Done.
lgtm Thanks Yiyao. Chrome coding style does not allow multiple inheritance, you can't inherit from RlzLibTestNoMachineStateHelper. But you can simply add a member like: RlzLibTestNoMachineStateHelper m_rlz_test_helper_; to the two classes and it will do the same thing.
Patchset #7 (id:510001) has been deleted
Patchset #7 (id:540001) has been deleted
Patchset #7 (id:690001) has been deleted
@sky, Could do an owner's review for file chrome/browser/DEPS? Thanks @Roger, The try bot seems complaining about the helper class constructor returning a value, so I moved them to 2 SetUp and TearDown functions. Thanks, Yiyao
yiyaoliu@chromium.org changed reviewers: + sky@chromium.org
still lgtm Thanks Yiyao.
Hi Sky, could you do an owner's review for file chrome/browser/DEPS? Thanks!
https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS#ne... chrome/browser/DEPS:82: "+content/test", Seems to me we should only be depending on content/test/public.
https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS#ne... chrome/browser/DEPS:82: "+content/test", On 2014/10/10 17:45:49, sky wrote: > Seems to me we should only be depending on content/test/public. you mean content/test/net? We need content::TestRenderFrameHost to simulate a navigation. Are you aware of another way of achieving this with the correct dependency?
On 2014/10/10 17:45:49, sky wrote: > https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS > File chrome/browser/DEPS (right): > > https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS#ne... > chrome/browser/DEPS:82: "+content/test", > Seems to me we should only be depending on content/test/public. Hi Scott, seems like lots of tests include files in content/test that are not in content/test/public: https://code.google.com/p/chromium/codesearch#search/&q=test_render_frame_hos...
On 2014/10/10 19:22:55, Roger Tawa wrote: > On 2014/10/10 17:45:49, sky wrote: > > https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS > > File chrome/browser/DEPS (right): > > > > > https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS#ne... > > chrome/browser/DEPS:82: "+content/test", > > Seems to me we should only be depending on content/test/public. > > Hi Scott, seems like lots of tests include files in content/test that are not in > content/test/public: > > https://code.google.com/p/chromium/codesearch#search/&q=test_render_frame_hos... It's fine for tests in content to depend upon anything in content. But tests outside of content should be using content/test/public. -Scott
On 2014/10/13 16:42:15, sky wrote: > On 2014/10/10 19:22:55, Roger Tawa wrote: > > On 2014/10/10 17:45:49, sky wrote: > > > https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS > > > File chrome/browser/DEPS (right): > > > > > > > > > https://codereview.chromium.org/591483002/diff/1190001/chrome/browser/DEPS#ne... > > > chrome/browser/DEPS:82: "+content/test", > > > Seems to me we should only be depending on content/test/public. > > > > Hi Scott, seems like lots of tests include files in content/test that are not > in > > content/test/public: > > > > > https://code.google.com/p/chromium/codesearch#search/&q=test_render_frame_hos... > > > It's fine for tests in content to depend upon anything in content. But tests > outside of content should be using content/test/public. > > -Scott code outside content should use RenderFrameHostTester (in content/public/test). TestRenderFrameHost is just the internal content implementation of that.
Patchset #11 (id:1390001) has been deleted
Hi Roger, I removed the dependency on content/test, and it's ready for review. Thanks! Yiyao
lgtm Thanks Yao!
Patchset #12 (id:1430001) has been deleted
The CQ bit was checked by yiyaoliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/591483002/1450001
Message was sent while issue was closed.
Committed patchset #12 (id:1450001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9b9d2e0a172bf3c3592fa6543c59b61277f20e2f Cr-Commit-Position: refs/heads/master@{#312421} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
