Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
441623002:
Eliminate sending NOTIFICATION_TOP_SITES_* from TopSites (Closed)
DescriptionEliminate sending NOTIFICATION_TOP_SITES_* from TopSites
Add new interface TopSitesObserver.
- add TopSitesLoaded method to TopSitesObserver
- add TopSitesChanged method to TopSitesObserver
- Make ChromeHistoryClient a TopSitesObserver.
- Invoke the observer methods and ChromeHistoryClient will send the notification
BUG=373329
TEST=unit_tests --gest_filter=TopSites*,ExpireHistoryTest*
Committed: https://crrev.com/e091cc30ea46508e6f5edd3bf2169f32e4af31d5
Cr-Commit-Position: refs/heads/master@{#295398}
Patch Set 1 #
Total comments: 47
Patch Set 2 : Address review comments #
Total comments: 6
Patch Set 3 : Moving top_sites_observer to components #Patch Set 4 : Add new file to gyp and gn #
Total comments: 2
Patch Set 5 : Fixing the commit message #
Total comments: 2
Patch Set 6 : Address review comments #Patch Set 7 : Rebase to ToT #Patch Set 8 : Fixing ExpireHistoryTest #Patch Set 9 : Moving the WaitTopSitesLoadedObserver to testing_profile #
Total comments: 4
Patch Set 10 : Fix incorrect scoped_refptr handling #
Total comments: 1
Patch Set 11 : Add comment #Patch Set 12 : Fix compilation error #Patch Set 13 : Fix incorrect parmeter being passed in notification #Messages
Total messages: 53 (11 generated)
@sky for: chrome/browser/history/chrome_history_client.cc chrome/browser/history/chrome_history_client.h chrome/browser/history/chrome_history_client_factory.cc chrome/browser/history/top_sites.cc chrome/browser/history/top_sites.h chrome/browser/history/top_sites_impl.cc chrome/browser/history/top_sites_impl.h chrome/browser/history/top_sites_impl_unittest.cc chrome/browser/history/top_sites_observer.h chrome/test/base/testing_profile.cc
I think you're on a good track. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... File chrome/browser/history/chrome_history_client.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.cc:18: class Profile; Remove, this is un-necessary. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.cc:20: ChromeHistoryClient::ChromeHistoryClient(BookmarkModel* bookmark_model, Receive Profile* as an additional parameter and store it. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.cc:86: content::Source<Profile>(top_sites->GetProfile()), Use saved pointer to the Profile*. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.cc:93: content::Source<Profile>(top_sites->GetProfile()), Same. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... File chrome/browser/history/chrome_history_client.h (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.h:23: explicit ChromeHistoryClient(BookmarkModel* bookmark_model, Add the Profile as an additional parameter. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... File chrome/browser/history/chrome_history_client_factory.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client_factory.cc:14: namespace history { Remove, this is un-necessary. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.cc:46: Profile* TopSites::GetProfile() { Remove. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites.h (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:172: // TopSitesObserver Remove this comment block and the Observer type defined here (it is not used, instead you are using TopSitesObserver that is defined in top_sites_observer.h). https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:182: void AddObserver(TopSitesObserver* observer) { style: do not inline functions, see http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:191: void NotifyTopSitesLoaded() { Move to the protected: section since this is only called by TopSitesImpl. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:195: void NotifyTopSitesChanged() { Move to the protected: section since this is only called by TopSitesImpl. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:199: // Get the current Profile. Remove, instead pass the Profile to ChromeHistoryClient constructor. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl.cc:640: Profile* TopSitesImpl::GetProfile() { Remove. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites_impl.h (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl.h:91: virtual Profile* GetProfile() OVERRIDE; Remove. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites_impl_unittest.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:32: class MockTopSitesObserver : public TopSitesObserver { nit: Rename MockTopSitesObserver to TestTopSitesObserver (you're not mocking anything using the gmock framework, so the name MockTopSitesObserver is misleading). https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:45: if (top_sites_) Remove the condition, since the observer must be created with a non-NULL pointer. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:49: MockTopSitesObserver::MockTopSitesObserver(history::TopSites* top_sites) Add the Profile as additional parameter to this constructor. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:51: if (top_sites_) Change to DCHECK(top_sites_) as there is no point creating this observer without a TopSites instance. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:372: if (top_sites_observer_) Change to DCHECK(!top_sites_observer_) as the object should have been destroyed by now (otherwise it may refer to a TopSites* that is dangling). https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites_observer.h (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_observer.h:12: // Interface for TopSitesLoaded and TopSitesChanged Please re-word this description. Maybe something along the lines of: // Interface to receive notifications relating to TopSites. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_observer.h:13: class TopSitesObserver { Please put the TopSitesObserver type into the history namespace. namespace history { class TopSites; // Interface for ... class TopSitesObserver { ... https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_observer.h:15: virtual void TopSitesLoaded(history::TopSites* top_sites) = 0; Please document (you can take inspiration from the documentation of NOTIFICATION_TOP_SITES_LOADED). https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_observer.h:16: virtual void TopSitesChanged(history::TopSites* top_sites) = 0; Please document (you can take inspiration from the documentation of NOTIFICATION_TOP_SITES_CHANGED). https://codereview.chromium.org/441623002/diff/1/chrome/test/base/testing_pro... File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/test/base/testing_pro... chrome/test/base/testing_profile.cc:533: return new ChromeHistoryClient(BookmarkModelFactory::GetForProfile(profile), Pass the Profile* as an additional parameter.
@sdefresne, Thanks for your comments. Please review. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... File chrome/browser/history/chrome_history_client.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.cc:18: class Profile; On 2014/08/04 08:47:17, sdefresne wrote: > Remove, this is un-necessary. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.cc:20: ChromeHistoryClient::ChromeHistoryClient(BookmarkModel* bookmark_model, On 2014/08/04 08:47:18, sdefresne wrote: > Receive Profile* as an additional parameter and store it. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.cc:86: content::Source<Profile>(top_sites->GetProfile()), On 2014/08/04 08:47:18, sdefresne wrote: > Use saved pointer to the Profile*. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.cc:93: content::Source<Profile>(top_sites->GetProfile()), On 2014/08/04 08:47:17, sdefresne wrote: > Same. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... File chrome/browser/history/chrome_history_client.h (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client.h:23: explicit ChromeHistoryClient(BookmarkModel* bookmark_model, On 2014/08/04 08:47:18, sdefresne wrote: > Add the Profile as an additional parameter. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... File chrome/browser/history/chrome_history_client_factory.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/chrom... chrome/browser/history/chrome_history_client_factory.cc:14: namespace history { On 2014/08/04 08:47:18, sdefresne wrote: > Remove, this is un-necessary. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.cc:46: Profile* TopSites::GetProfile() { On 2014/08/04 08:47:18, sdefresne wrote: > Remove. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites.h (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:172: // TopSitesObserver My bad.had the old code left there... deleted On 2014/08/04 08:47:18, sdefresne wrote: > Remove this comment block and the Observer type defined here (it is not used, > instead you are using TopSitesObserver that is defined in top_sites_observer.h). https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:182: void AddObserver(TopSitesObserver* observer) { On 2014/08/04 08:47:18, sdefresne wrote: > style: do not inline functions, see > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:191: void NotifyTopSitesLoaded() { On 2014/08/04 08:47:18, sdefresne wrote: > Move to the protected: section since this is only called by TopSitesImpl. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:195: void NotifyTopSitesChanged() { On 2014/08/04 08:47:18, sdefresne wrote: > Move to the protected: section since this is only called by TopSitesImpl. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites.h:199: // Get the current Profile. On 2014/08/04 08:47:18, sdefresne wrote: > Remove, instead pass the Profile to ChromeHistoryClient constructor. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl.cc:640: Profile* TopSitesImpl::GetProfile() { On 2014/08/04 08:47:18, sdefresne wrote: > Remove. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites_impl.h (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl.h:91: virtual Profile* GetProfile() OVERRIDE; On 2014/08/04 08:47:18, sdefresne wrote: > Remove. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites_impl_unittest.cc (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:32: class MockTopSitesObserver : public TopSitesObserver { On 2014/08/04 08:47:18, sdefresne wrote: > nit: Rename MockTopSitesObserver to TestTopSitesObserver (you're not mocking > anything using the gmock framework, so the name MockTopSitesObserver is > misleading). Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:45: if (top_sites_) On 2014/08/04 08:47:19, sdefresne wrote: > Remove the condition, since the observer must be created with a non-NULL > pointer. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:49: MockTopSitesObserver::MockTopSitesObserver(history::TopSites* top_sites) On 2014/08/04 08:47:18, sdefresne wrote: > Add the Profile as additional parameter to this constructor. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:51: if (top_sites_) On 2014/08/04 08:47:19, sdefresne wrote: > Change to DCHECK(top_sites_) as there is no point creating this observer without > a TopSites instance. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_impl_unittest.cc:372: if (top_sites_observer_) Actually the object will not be destroyed. profile()->CreateTopSites() destroys TopSites and not TopSitesObserver; making top_sites_observer_ refer to dangling TopSites. So before CreateTopSites is called I am destroying the top_sites_observer_ in case it is added as an observer, it will be removed. Is there a better way to do this without changing profile()->CreateTopSites()? On 2014/08/04 08:47:19, sdefresne wrote: > Change to DCHECK(!top_sites_observer_) as the object should have been destroyed > by now (otherwise it may refer to a TopSites* that is dangling). https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... File chrome/browser/history/top_sites_observer.h (right): https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_observer.h:12: // Interface for TopSitesLoaded and TopSitesChanged On 2014/08/04 08:47:19, sdefresne wrote: > Please re-word this description. Maybe something along the lines of: > > // Interface to receive notifications relating to TopSites. Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_observer.h:13: class TopSitesObserver { On 2014/08/04 08:47:19, sdefresne wrote: > Please put the TopSitesObserver type into the history namespace. > > namespace history { > class TopSites; > > // Interface for ... > class TopSitesObserver { > ... Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_observer.h:15: virtual void TopSitesLoaded(history::TopSites* top_sites) = 0; On 2014/08/04 08:47:19, sdefresne wrote: > Please document (you can take inspiration from the documentation of > NOTIFICATION_TOP_SITES_LOADED). Done. https://codereview.chromium.org/441623002/diff/1/chrome/browser/history/top_s... chrome/browser/history/top_sites_observer.h:16: virtual void TopSitesChanged(history::TopSites* top_sites) = 0; On 2014/08/04 08:47:19, sdefresne wrote: > Please document (you can take inspiration from the documentation of > NOTIFICATION_TOP_SITES_CHANGED). Done.
- sky, + brettw who is the proper OWNERS for //chrome/browser/history LGTM, please wait for approval from brettw before submitting. https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/c... File chrome/browser/history/chrome_history_client.cc (right): https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/c... chrome/browser/history/chrome_history_client.cc:85: content::NotificationService::current()->Notify( nit: DCHECK(top_sites == top_sites_); https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/c... chrome/browser/history/chrome_history_client.cc:92: content::NotificationService::current()->Notify( nit: DCHECK(top_sites == top_sites_); https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/t... File chrome/browser/history/top_sites_observer.h (right): https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/t... chrome/browser/history/top_sites_observer.h:17: // Is called when TopSites when the either one of the most visited urls nit: Please remove "when TopSites" and the extra "the" :-) // Is called when either one of the most visited urls change, or one of the images changes.
I forgot to mention that you should move chrome/browser/history/top_sites_observer.h to component/history/core/browser/top_sites_observer.h (you can use tools/git/move_source_file.py for that).
https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/t... File chrome/browser/history/top_sites_observer.h (right): https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/t... chrome/browser/history/top_sites_observer.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. s/2013/2014/ and remove the (c). In general, you should use tools/boilerplate.py to create new files as the script take care of creating correct headers, which are (till the end of the year). // Copyright 2014 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.
@brettw, Please review. Thanks. https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/t... File chrome/browser/history/top_sites_observer.h (right): https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/t... chrome/browser/history/top_sites_observer.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/08/11 12:01:31, sdefresne wrote: > s/2013/2014/ and remove the (c). In general, you should use tools/boilerplate.py > to create new files as the script take care of creating correct headers, which > are (till the end of the year). > > // Copyright 2014 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. Done. https://codereview.chromium.org/441623002/diff/20001/chrome/browser/history/t... chrome/browser/history/top_sites_observer.h:17: // Is called when TopSites when the either one of the most visited urls On 2014/08/11 09:43:23, sdefresne wrote: > nit: Please remove "when TopSites" and the extra "the" :-) > > // Is called when either one of the most visited urls change, or one of the > images changes. Sorry :) Silly mistake
Still LGTM. nit: you should add reference to top_sites_observer.h to //components/history.gypi and //components/history/core/browser/BUILD.gn
On 2014/08/13 09:33:54, sdefresne wrote: > Still LGTM. > > nit: you should add reference to top_sites_observer.h to > //components/history.gypi and //components/history/core/browser/BUILD.gn Done. Please review. @brettw for chrome/browser/history components/history/core @phajdan.jr for chrome/test/base/testing_profile.cc
chrome/test/base LGTM
It looks like you're planning on porting the listeners one-at-a-time as described in the bug. But the CL description doesn't mention this, and the patch does not remove the notification as the description says. Can you make it more precise? https://codereview.chromium.org/441623002/diff/60001/chrome/browser/history/c... File chrome/browser/history/chrome_history_client.h (right): https://codereview.chromium.org/441623002/diff/60001/chrome/browser/history/c... chrome/browser/history/chrome_history_client.h:48: history::TopSites* top_sites_; How does the memory management work for this? I guess TopSites is owned by the profile, but this is some kind of keyed service. What is the lifetime of that in relation to Profile? It seems like we need to make a less fragile way to handle this dependency.
@brettw, fixed the commit message. Please review https://codereview.chromium.org/441623002/diff/60001/chrome/browser/history/c... File chrome/browser/history/chrome_history_client.h (right): https://codereview.chromium.org/441623002/diff/60001/chrome/browser/history/c... chrome/browser/history/chrome_history_client.h:48: history::TopSites* top_sites_; On 2014/08/14 22:45:41, brettw wrote: > How does the memory management work for this? I guess TopSites is owned by the > profile, but this is some kind of keyed service. What is the lifetime of that in > relation to Profile? It seems like we need to make a less fragile way to handle > this dependency. Sylvain, can you please answer this question.
On 2014/08/15 06:16:12, naiem.shaik wrote: > @brettw, > fixed the commit message. > Please review > > https://codereview.chromium.org/441623002/diff/60001/chrome/browser/history/c... > File chrome/browser/history/chrome_history_client.h (right): > > https://codereview.chromium.org/441623002/diff/60001/chrome/browser/history/c... > chrome/browser/history/chrome_history_client.h:48: history::TopSites* > top_sites_; > On 2014/08/14 22:45:41, brettw wrote: > > How does the memory management work for this? I guess TopSites is owned by the > > profile, but this is some kind of keyed service. What is the lifetime of that > in > > relation to Profile? It seems like we need to make a less fragile way to > handle > > this dependency. > > Sylvain, can you please answer this question. Sure. The TopSites object is owned by the Profile (see //chrome/browser/profiles/profile_impl.h) and lazily constructed by the getter. ChromeHistoryClient is a KeyedService linked to the Profile lifetime by the ChromeHistoryClientFactory (which is a BrowserContextKeyedServiceFactory). Before the Profile is destroyed, all the KeyedService Shutdown methods are called, and the Profile is fully constructed before any of the KeyedService can be constructed. The TopSites does not use the HistoryService nor the HistoryClient during construction (it use it later, but supports getting an NULL pointer). I think this is safe (though I would recommend porting the clients of the notifications as soon as possible in followup CLs).
The commit message still is old. https://codereview.chromium.org/441623002/diff/80001/chrome/browser/history/c... File chrome/browser/history/chrome_history_client.h (right): https://codereview.chromium.org/441623002/diff/80001/chrome/browser/history/c... chrome/browser/history/chrome_history_client.h:48: history::TopSites* top_sites_; Can you put a comment here like you did in the code review clarifying that this is a non-owning pointer and talk why the lifetime of this is OK?
PTAL https://codereview.chromium.org/441623002/diff/80001/chrome/browser/history/c... File chrome/browser/history/chrome_history_client.h (right): https://codereview.chromium.org/441623002/diff/80001/chrome/browser/history/c... chrome/browser/history/chrome_history_client.h:48: history::TopSites* top_sites_; On 2014/08/19 22:51:58, brettw wrote: > Can you put a comment here like you did in the code review clarifying that this > is a non-owning pointer and talk why the lifetime of this is OK? Done.
On 2014/08/20 18:31:35, naiem.shaik wrote: > PTAL > > https://codereview.chromium.org/441623002/diff/80001/chrome/browser/history/c... > File chrome/browser/history/chrome_history_client.h (right): > > https://codereview.chromium.org/441623002/diff/80001/chrome/browser/history/c... > chrome/browser/history/chrome_history_client.h:48: history::TopSites* > top_sites_; > On 2014/08/19 22:51:58, brettw wrote: > > Can you put a comment here like you did in the code review clarifying that > this > > is a non-owning pointer and talk why the lifetime of this is OK? > > Done. @brettw, PTAL
@brettw, PTAL
Sorry, LGTM
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/441623002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48213) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/53542) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by naiem.shaik@gmail.com
PTAL, re-based the changes.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/441623002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/441623002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
@brettw for chrome/browser/history/expire_history_backend_unittest.cc PTAL
On 2014/09/02 23:22:03, naiem.shaik wrote: > @brettw for > chrome/browser/history/expire_history_backend_unittest.cc > > PTAL I am thinking of moving the class WaitTopSitesLoadedObserver to test_utils, as that would be the right thing to do. I will work on it and upload the change.
@phajdan.jr PTAL chrome/test/base/testing_profile.cc
Please fix the scoped_ptr issue in the unittest. https://codereview.chromium.org/441623002/diff/160001/chrome/browser/history/... File chrome/browser/history/chrome_history_client_factory.cc (right): https://codereview.chromium.org/441623002/diff/160001/chrome/browser/history/... chrome/browser/history/chrome_history_client_factory.cc:38: return new ChromeHistoryClient( nit: you can define a profile variable here to avoid the three static_cast: Profile* profile = static_cast<Profile*>(context); return new ChromeHistoryClient( BookmarkModelFactory::GetForProfile(profile), profile, profile->GetTopSites()); https://codereview.chromium.org/441623002/diff/160001/chrome/test/base/testin... File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/441623002/diff/160001/chrome/test/base/testin... chrome/test/base/testing_profile.cc:120: scoped_refptr<content::MessageLoopRunner> runner_; Change to a raw pointer or change the constructor to take a scoped_ptr<>. Otherwise there is risk (as is the case later) to have two scoped_ptr pointing to the same object, thus causing double-free issues.
PTAL https://codereview.chromium.org/441623002/diff/160001/chrome/browser/history/... File chrome/browser/history/chrome_history_client_factory.cc (right): https://codereview.chromium.org/441623002/diff/160001/chrome/browser/history/... chrome/browser/history/chrome_history_client_factory.cc:38: return new ChromeHistoryClient( On 2014/09/04 15:50:01, sdefresne wrote: > nit: you can define a profile variable here to avoid the three static_cast: > > Profile* profile = static_cast<Profile*>(context); > return new ChromeHistoryClient( > BookmarkModelFactory::GetForProfile(profile), profile, > profile->GetTopSites()); Done. https://codereview.chromium.org/441623002/diff/160001/chrome/test/base/testin... File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/441623002/diff/160001/chrome/test/base/testin... chrome/test/base/testing_profile.cc:120: scoped_refptr<content::MessageLoopRunner> runner_; On 2014/09/04 15:50:01, sdefresne wrote: > Change to a raw pointer or change the constructor to take a scoped_ptr<>. > Otherwise there is risk (as is the case later) to have two scoped_ptr pointing > to the same object, thus causing double-free issues. Done.
lgtm https://codereview.chromium.org/441623002/diff/180001/chrome/test/base/testin... File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/441623002/diff/180001/chrome/test/base/testin... chrome/test/base/testing_profile.cc:120: content::MessageLoopRunner* runner_; nit: add // weak
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/441623002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/441623002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/441623002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as 26a9f73c0a56d2eb538c97d0804e8c6693dadfe3
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as 26a9f73c0a56d2eb538c97d0804e8c6693dadfe3
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/e091cc30ea46508e6f5edd3bf2169f32e4af31d5 Cr-Commit-Position: refs/heads/master@{#295398} |
