Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2350773002:
Add a browsing data counter factory on iOS (Closed)
DescriptionAdd a browsing data counter factory on iOS
- ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.* is a counterpart of chrome/browser/browsing_data/browsing_data_counter_factory.*
The utils added in this CL will be used downstream.
BUG=620318
Committed: https://crrev.com/35c03a44f02d421bce1dc588f56e8721b0b14034
Cr-Commit-Position: refs/heads/master@{#419730}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Addressed comments #
Messages
Total messages: 30 (22 generated)
Description was changed from ========== Add a browsing data counter factory and a counter wrapper on iOS` BUG= ========== to ========== Add a browsing data counter factory and a counter wrapper on iOS BUG= ==========
Description was changed from ========== Add a browsing data counter factory and a counter wrapper on iOS BUG= ========== to ========== Add a browsing data counter factory and a counter wrapper on iOS BUG=620318 ==========
Description was changed from ========== Add a browsing data counter factory and a counter wrapper on iOS BUG=620318 ========== to ========== Add a browsing data counter factory and a counter wrapper on iOS - ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.* is a counterpart of chrome/browser/browsing_data/browsing_data_counter_factory.* BUG=620318 ==========
Description was changed from ========== Add a browsing data counter factory and a counter wrapper on iOS - ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.* is a counterpart of chrome/browser/browsing_data/browsing_data_counter_factory.* BUG=620318 ========== to ========== Add a browsing data counter factory on iOS - ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.* is a counterpart of chrome/browser/browsing_data/browsing_data_counter_factory.* The utils added in this CL will be used downstream. BUG=620318 ==========
The CQ bit was checked by ioanap@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
ioanap@google.com changed reviewers: + sdefresne@chromium.org
Please take a look! Thanks, Ioana
https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... File ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.cc (right): https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.cc:45: BrowsingDataCounterWrapper::~BrowsingDataCounterWrapper() {} nit: Can you put method implementation in the same order as declaration? https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... File ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h (right): https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:11: #include "base/callback.h" I think you can include base/callback_forward.h here and include base/callback.h in the implementation file instead (may have to include in multiple implementation files). https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:12: #include "components/browsing_data/core/counters/browsing_data_counter.h" You don't call any method of BrowsingDataCounter in that file, so you can probably forward-declare it and move the include in the implementation file (files?). https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:24: typedef base::Callback<void( nit: I think using is recommended instead of typedef now (easier to read) using UpdateUICallback = base::Callback<void(const browsing_data::BrowsingDataCounter::Result&)>; https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:30: static BrowsingDataCounterWrapper* CreateCounterWrapper( Why is this method not returning a std::unique_ptr<>? https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:34: const UpdateUICallback update_ui_callback); const UpdateUICallback& update_ui_callback https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:57: }; DISALLOW_COPY_AND_ASSIGN()? https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... File ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h (right): https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h:23: const std::string& pref_name); In browsing_data_counter_wrapper.cc you are calling this method with a "const char*" parameter, thus allocating memory for the string. I see that in the implementation you use std::string::operator == to compare to other "const char*". Can you change this parameter to instead be a base::StringPiece to avoid memory allocation?
The CQ bit was checked by ioanap@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Addressed comments. Thanks, Ioana https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... File ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.cc (right): https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.cc:45: BrowsingDataCounterWrapper::~BrowsingDataCounterWrapper() {} On 2016/09/19 17:07:52, sdefresne wrote: > nit: Can you put method implementation in the same order as declaration? Done. https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... File ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h (right): https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:11: #include "base/callback.h" On 2016/09/19 17:07:52, sdefresne wrote: > I think you can include base/callback_forward.h here and include base/callback.h > in the implementation file instead (may have to include in multiple > implementation files). Done. https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:12: #include "components/browsing_data/core/counters/browsing_data_counter.h" On 2016/09/19 17:07:52, sdefresne wrote: > You don't call any method of BrowsingDataCounter in that file, so you can > probably forward-declare it and move the include in the implementation file > (files?). I don't call any method, but I need a nested class from it: BrowsingDataCounter::Result. https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:24: typedef base::Callback<void( On 2016/09/19 17:07:52, sdefresne wrote: > nit: I think using is recommended instead of typedef now (easier to read) > > using UpdateUICallback = base::Callback<void(const > browsing_data::BrowsingDataCounter::Result&)>; Didn't know that. Thank you! https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:30: static BrowsingDataCounterWrapper* CreateCounterWrapper( On 2016/09/19 17:07:52, sdefresne wrote: > Why is this method not returning a std::unique_ptr<>? Done. https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:34: const UpdateUICallback update_ui_callback); On 2016/09/19 17:07:52, sdefresne wrote: > const UpdateUICallback& update_ui_callback Done. https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:57: }; On 2016/09/19 17:07:52, sdefresne wrote: > DISALLOW_COPY_AND_ASSIGN()? Done. https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... File ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h (right): https://codereview.chromium.org/2350773002/diff/1/ios/chrome/browser/browsing... ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h:23: const std::string& pref_name); On 2016/09/19 17:07:53, sdefresne wrote: > In browsing_data_counter_wrapper.cc you are calling this method with a "const > char*" parameter, thus allocating memory for the string. I see that in the > implementation you use std::string::operator == to compare to other "const > char*". Can you change this parameter to instead be a base::StringPiece to avoid > memory allocation? Done.
lgtm with nits https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... File ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h (right): https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:27: // This method returns a nullptr if there is no corresponding counter for the nit: We usually do not use "nullptr" in comments, so I would just write something like // This method returns the counter corresponding to the data type specified by // |pref_name| or null if there is no such counter. https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... File ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.cc (right): https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.cc:45: base::Bind(&GetUpdatedWebHistoryService, nit: Can't you just bind ios::WebHistoryServiceFactory::GetForBrowserState? base::Bind( &ios::WebHistoryServiceFactory::GetForBrowserState, base::Unretained(browser_state)), Code is fine as is, so if the answer is no, I'm okay with current code. https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... File ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h (right): https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h:11: #include "ios/chrome/browser/browser_state/chrome_browser_state.h" nit: I think you can forward-declare ios::ChromeBrowserState. https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h:22: GetForBrowserStateAndPref(ios::ChromeBrowserState* browserState, style: this is C++, so the variable should be called "browser_state" (in the comment too).
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by ioanap@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Addressed comments. Landing now! Thanks, Ioana https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... File ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h (right): https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/browsing_data_counter_wrapper.h:27: // This method returns a nullptr if there is no corresponding counter for the On 2016/09/20 11:50:46, sdefresne wrote: > nit: We usually do not use "nullptr" in comments, so I would just write > something like > > // This method returns the counter corresponding to the data type specified by > // |pref_name| or null if there is no such counter. Done. https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... File ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.cc (right): https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.cc:45: base::Bind(&GetUpdatedWebHistoryService, On 2016/09/20 11:50:46, sdefresne wrote: > nit: Can't you just bind ios::WebHistoryServiceFactory::GetForBrowserState? > > base::Bind( > &ios::WebHistoryServiceFactory::GetForBrowserState, > base::Unretained(browser_state)), > > Code is fine as is, so if the answer is no, I'm okay with current code. Indeed I can. Thank you! https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... File ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h (right): https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h:11: #include "ios/chrome/browser/browser_state/chrome_browser_state.h" On 2016/09/20 11:50:46, sdefresne wrote: > nit: I think you can forward-declare ios::ChromeBrowserState. Done. https://codereview.chromium.org/2350773002/diff/20001/ios/chrome/browser/brow... ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.h:22: GetForBrowserStateAndPref(ios::ChromeBrowserState* browserState, On 2016/09/20 11:50:46, sdefresne wrote: > style: this is C++, so the variable should be called "browser_state" (in the > comment too). Done.
The CQ bit was checked by ioanap@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2350773002/#ps60001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a browsing data counter factory on iOS - ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.* is a counterpart of chrome/browser/browsing_data/browsing_data_counter_factory.* The utils added in this CL will be used downstream. BUG=620318 ========== to ========== Add a browsing data counter factory on iOS - ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.* is a counterpart of chrome/browser/browsing_data/browsing_data_counter_factory.* The utils added in this CL will be used downstream. BUG=620318 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a browsing data counter factory on iOS - ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.* is a counterpart of chrome/browser/browsing_data/browsing_data_counter_factory.* The utils added in this CL will be used downstream. BUG=620318 ========== to ========== Add a browsing data counter factory on iOS - ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.* is a counterpart of chrome/browser/browsing_data/browsing_data_counter_factory.* The utils added in this CL will be used downstream. BUG=620318 Committed: https://crrev.com/35c03a44f02d421bce1dc588f56e8721b0b14034 Cr-Commit-Position: refs/heads/master@{#419730} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/35c03a44f02d421bce1dc588f56e8721b0b14034 Cr-Commit-Position: refs/heads/master@{#419730} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
