Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2166023004:
Cleanup in InstantTab and InstantController (Closed)
|
Created:
4 years, 5 months ago by Marc Treib Modified:
4 years, 5 months ago Reviewers:
sfiera CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCleanup in InstantTab and InstantController
Removed some unused code.
BUG=627747
Committed: https://crrev.com/cdd7937831d89852cd02c1d056a2d6f940fe0b24
Cr-Commit-Position: refs/heads/master@{#407460}
Patch Set 1 #
Total comments: 14
Patch Set 2 : review #Patch Set 3 : remove Times(1) #
Total comments: 3
Patch Set 4 : bring back Init() #
Total comments: 1
Messages
Total messages: 26 (9 generated)
Description was changed from ========== Cleanup in InstantTab BUG=627747 ========== to ========== Cleanup in InstantTab and InstantController Removed some unused code. BUG=627747 ==========
treib@chromium.org changed reviewers: + sfiera@chromium.org
Here's your first (?) chance to use your awesome new powers of approval! PTAL! https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_controller.h (left): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_controller.h:62: void LogDebugEvent(const std::string& info) const; Made private https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_controller.h:74: void InstantSupportChanged(InstantSupportState instant_support); Moved up, above the debug stuff https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_controller.h:80: virtual Profile* profile() const; Neither of these are needed https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_tab.h (left): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab.h:52: void Init(content::WebContents* web_contents); Merged into the ctor, no need for two-phase initialization https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab.h:58: bool supports_instant() const; This just forwarded to SearchTabHelper, and the lone caller (InstantController) can just as well use that directly. https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab.h:62: bool IsLocal() const; Not used
https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_controller.cc:75: SearchTabHelper::FromWebContents(instant_tab_->web_contents()); By getting search_tab directly here, you've dropped a check that instant_tab_->web_contents() != nullptr. https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_tab.cc (right): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab.cc:61: void InstantTab::ClearContents() { Now called only from InstantSupportDetermined(). Merge it up there? https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_tab_unittest.cc (right): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab_unittest.cc:67: .Times(1); Not needed; gmock will infer .Times(1) when there aren't any ".Will...()"
https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_controller.cc:75: SearchTabHelper::FromWebContents(instant_tab_->web_contents()); On 2016/07/22 12:51:00, sfiera wrote: > By getting search_tab directly here, you've dropped a check that > instant_tab_->web_contents() != nullptr. Good catch! Added the check back. https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_tab.cc (right): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab.cc:61: void InstantTab::ClearContents() { On 2016/07/22 12:51:00, sfiera wrote: > Now called only from InstantSupportDetermined(). Merge it up there? Done. https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_tab_unittest.cc (right): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab_unittest.cc:67: .Times(1); On 2016/07/22 12:51:00, sfiera wrote: > Not needed; gmock will infer .Times(1) when there aren't any ".Will...()" Does it? I thought that without the explicit ".Times(1)", a second call would be considered "uninteresting" but not an error, while with ".Times(1)" it would be an error?
LGTM https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_tab_unittest.cc (right): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab_unittest.cc:67: .Times(1); On 2016/07/22 13:11:17, Marc Treib wrote: > On 2016/07/22 12:51:00, sfiera wrote: > > Not needed; gmock will infer .Times(1) when there aren't any ".Will...()" > > Does it? I thought that without the explicit ".Times(1)", a second call would be > considered "uninteresting" but not an error, while with ".Times(1)" it would be > an error? Nope, .Times(1) is the same thing. In general, I prefer to use StrictMock<> on pretty much everything anyway, in which case all uninteresting calls are errors. Here, InstantTabAboutToNavigateMainFrame() could be called without failing the test.
https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... File chrome/browser/ui/search/instant_tab_unittest.cc (right): https://codereview.chromium.org/2166023004/diff/1/chrome/browser/ui/search/in... chrome/browser/ui/search/instant_tab_unittest.cc:67: .Times(1); On 2016/07/25 08:20:06, sfiera wrote: > On 2016/07/22 13:11:17, Marc Treib wrote: > > On 2016/07/22 12:51:00, sfiera wrote: > > > Not needed; gmock will infer .Times(1) when there aren't any ".Will...()" > > > > Does it? I thought that without the explicit ".Times(1)", a second call would > be > > considered "uninteresting" but not an error, while with ".Times(1)" it would > be > > an error? > > Nope, .Times(1) is the same thing. Alright, removed the "Times(1)" everywhere. > In general, I prefer to use StrictMock<> on pretty much everything anyway, in > which case all uninteresting calls are errors. Here, > InstantTabAboutToNavigateMainFrame() could be called without failing the test. That seems like a good policy; I should adopt that too :) Changing it here is an exercise for another day though.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2166023004/#ps40001 (title: "remove Times(1)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2166023004/diff/40001/chrome/browser/ui/searc... File chrome/browser/ui/search/instant_controller.cc (left): https://codereview.chromium.org/2166023004/diff/40001/chrome/browser/ui/searc... chrome/browser/ui/search/instant_controller.cc:166: instant_tab_->Init(active_tab); Argh, turns out the two-phase initialization is required after all :( Without that, the ctor might call us back (via InstantSupportDetermined) before instant_tab_ is assigned. Unless you have any bright ideas, I'll have to bring back Init()
https://codereview.chromium.org/2166023004/diff/40001/chrome/browser/ui/searc... File chrome/browser/ui/search/instant_controller.cc (left): https://codereview.chromium.org/2166023004/diff/40001/chrome/browser/ui/searc... chrome/browser/ui/search/instant_controller.cc:166: instant_tab_->Init(active_tab); On 2016/07/25 09:49:01, Marc Treib wrote: > Argh, turns out the two-phase initialization is required after all :( > Without that, the ctor might call us back (via InstantSupportDetermined) before > instant_tab_ is assigned. Unless you have any bright ideas, I'll have to bring > back Init() Well, I'd sooner have "InstantTab(this, active_tab)" and "instant_tab_->Init()" than the old way, since then at least it's clear that the delegate and web contents shouldn't change. I assume it's intended that the delegate methods are called on arbitrary threads, rather than just the one that the InstantTab was created on.
https://codereview.chromium.org/2166023004/diff/40001/chrome/browser/ui/searc... File chrome/browser/ui/search/instant_controller.cc (left): https://codereview.chromium.org/2166023004/diff/40001/chrome/browser/ui/searc... chrome/browser/ui/search/instant_controller.cc:166: instant_tab_->Init(active_tab); On 2016/07/25 09:57:14, sfiera wrote: > On 2016/07/25 09:49:01, Marc Treib wrote: > > Argh, turns out the two-phase initialization is required after all :( > > Without that, the ctor might call us back (via InstantSupportDetermined) > before > > instant_tab_ is assigned. Unless you have any bright ideas, I'll have to bring > > back Init() > > Well, I'd sooner have "InstantTab(this, active_tab)" and "instant_tab_->Init()" > than the old way, since then at least it's clear that the delegate and web > contents shouldn't change. Makes sense. It means we have to store the WebContents that's passed in, but oh well. I've called it |pending_web_contents_| to make clear that it's something different than the |web_contents()| inherited from WebContentsObserver. > I assume it's intended that the delegate methods are called on arbitrary > threads, rather than just the one that the InstantTab was created on. I'm pretty sure it's all on the same thread, but I don't really see how that's related...?
On 2016/07/25 11:40:17, Marc Treib wrote: > > I assume it's intended that the delegate methods are called on arbitrary > > threads, rather than just the one that the InstantTab was created on. > > I'm pretty sure it's all on the same thread, but I don't really see how that's > related...? Oh. Yeah, I see now. Now that you point it out, I really dislike what InstantTab is doing there, when it calls back in Init(). I don't think interfaces that call you back should ever call back before they return control to you. Instead, they should schedule the callback with base::ThreadTaskRunnerHandle::Get()->PostTask(). Otherwise you get confusing behavior, just like you just did :)
On 2016/07/25 12:09:02, sfiera wrote: > On 2016/07/25 11:40:17, Marc Treib wrote: > > > I assume it's intended that the delegate methods are called on arbitrary > > > threads, rather than just the one that the InstantTab was created on. > > > > I'm pretty sure it's all on the same thread, but I don't really see how that's > > related...? > > Oh. Yeah, I see now. Now that you point it out, I really dislike what InstantTab > is doing there, when it calls back in Init(). > > I don't think interfaces that call you back should ever call back before they > return control to you. Instead, they should schedule the callback with > base::ThreadTaskRunnerHandle::Get()->PostTask(). Otherwise you get confusing > behavior, just like you just did :) I don't disagree, but changing that here might have other side effects, which I'd like to avoid for now...
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2166023004/#ps60001 (title: "bring back Init()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2166023004/diff/60001/chrome/browser/ui/searc... File chrome/browser/ui/search/instant_tab_unittest.cc (right): https://codereview.chromium.org/2166023004/diff/60001/chrome/browser/ui/searc... chrome/browser/ui/search/instant_tab_unittest.cc:73: TEST_F(InstantTabTest, DetermineIfPageSupportsInstant_NonLocal) { Random other thing: This test is BS, or at least it's in the wrong place. It doesn't test anything in InstantTab. In fact, InstantTab just doesn't do a whole lot...
On 2016/07/25 12:27:56, Marc Treib wrote: > https://codereview.chromium.org/2166023004/diff/60001/chrome/browser/ui/searc... > File chrome/browser/ui/search/instant_tab_unittest.cc (right): > > https://codereview.chromium.org/2166023004/diff/60001/chrome/browser/ui/searc... > chrome/browser/ui/search/instant_tab_unittest.cc:73: TEST_F(InstantTabTest, > DetermineIfPageSupportsInstant_NonLocal) { > Random other thing: This test is BS, or at least it's in the wrong place. It > doesn't test anything in InstantTab. > In fact, InstantTab just doesn't do a whole lot... Well at least we don't have separate InstantTab and InstantPage classes anymore...
Message was sent while issue was closed.
Description was changed from ========== Cleanup in InstantTab and InstantController Removed some unused code. BUG=627747 ========== to ========== Cleanup in InstantTab and InstantController Removed some unused code. BUG=627747 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup in InstantTab and InstantController Removed some unused code. BUG=627747 ========== to ========== Cleanup in InstantTab and InstantController Removed some unused code. BUG=627747 Committed: https://crrev.com/cdd7937831d89852cd02c1d056a2d6f940fe0b24 Cr-Commit-Position: refs/heads/master@{#407460} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cdd7937831d89852cd02c1d056a2d6f940fe0b24 Cr-Commit-Position: refs/heads/master@{#407460} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
