Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121203002:
Add BrowsingDataCounterFactory (Closed)
|
Created:
4 years, 5 months ago by ioanap Modified:
4 years, 5 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, msramek+watch_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, markusheintz_ Base URL:
https://chromium.googlesource.com/chromium/src.git@move_bd_counters_to_components Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd BrowsingDataCounterFactory
Add a factory to facilitate the creation of browsing data counters for when they will be fully componentized.
BUG=620317
Committed: https://crrev.com/c6d9534492f46738b63b7d8500cd411bd723aa80
Cr-Commit-Position: refs/heads/master@{#404648}
Patch Set 1 #Patch Set 2 : Added MediaLicensesCounter to the factory #Patch Set 3 : Fixed typo in Android code #Patch Set 4 : Fixed Android code and rebase #
Total comments: 14
Patch Set 5 : Addressed comments #Patch Set 6 : Fixed includes #Patch Set 7 : Fixed Android code #
Total comments: 2
Patch Set 8 : Moved TODO #
Total comments: 4
Patch Set 9 : Addressed comments #Messages
Total messages: 45 (22 generated)
Description was changed from ========== Add a factory to facilitate the creation of browsing data counters for when they will be fully componentized. BUG=620317 ========== to ========== Add BrowsingDataCounterFactory Add a factory to facilitate the creation of browsing data counters for when they will be fully componentized. BUG=620317 ==========
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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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: + melandory@chromium.org, msramek@chromium.org
Please have a look! Thanks, Ioana
https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/android/... File chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc (left): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:29: counter_.reset(CreateCounterForPreference(pref, profile)); Why don't we remove this method? https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_factory.cc:23: browsing_data::BrowsingDataCounter* We should return a unique_ptr<>, not a naked pointer. That will also simplify the code on the callsites. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_factory.h (right): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_factory.h:21: static browsing_data::BrowsingDataCounter* GetForPreference( Consider calling this GetForProfileAndPreference or maybe shorter GetForProfileAndPref. In that spirit, I would also list the |profile| first. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_factory.h:22: std::string pref_name, nit: use a const ref to avoid copying the string https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/clear_browser_data_handler.cc:30: #include "chrome/browser/browsing_data/cache_counter.h" Ditto here - please remove the unnecessary includes. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:18: #include "chrome/browser/browsing_data/cache_counter.h" None of the .*counter\.h includes is needed anymore. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:219: // TODO(msramek): Simplify this using a factory. We're already using a factory, but (apart from removing includes) this didn't simplify things much. Consider using the following pattern: std::string prefs = { prefs::kDeleteBrowsingHistory, ... } for (const std::string& pref : prefs) AddCounter(...) This already improves things by explicitly listing the preferences instead of the counters. The last thing that would remain (out of the scope of this CL) is to auto-generate the list of preferences from the HTML/JS instead of hardcoding it. So please update the TODO for me: // TODO(msramek): Get the list of deletion preferences from the JS side.
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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #7 (id:120001) 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...
Addressed comments. Thank you, Ioana https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/android/... File chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc (left): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/android/... chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:29: counter_.reset(CreateCounterForPreference(pref, profile)); On 2016/07/07 11:55:32, msramek wrote: > Why don't we remove this method? Oups! Removed it now. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_factory.cc:23: browsing_data::BrowsingDataCounter* On 2016/07/07 11:55:32, msramek wrote: > We should return a unique_ptr<>, not a naked pointer. That will also simplify > the code on the callsites. Done. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_factory.h (right): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_factory.h:21: static browsing_data::BrowsingDataCounter* GetForPreference( On 2016/07/07 11:55:33, msramek wrote: > Consider calling this GetForProfileAndPreference or maybe shorter > GetForProfileAndPref. In that spirit, I would also list the |profile| first. GetForProfileAndPref sounds good! Done. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_factory.h:22: std::string pref_name, On 2016/07/07 11:55:33, msramek wrote: > nit: use a const ref to avoid copying the string Done. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/clear_browser_data_handler.cc:30: #include "chrome/browser/browsing_data/cache_counter.h" On 2016/07/07 11:55:33, msramek wrote: > Ditto here - please remove the unnecessary includes. Done. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:18: #include "chrome/browser/browsing_data/cache_counter.h" On 2016/07/07 11:55:33, msramek wrote: > None of the .*counter\.h includes is needed anymore. Done. https://codereview.chromium.org/2121203002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:219: // TODO(msramek): Simplify this using a factory. On 2016/07/07 11:55:33, msramek wrote: > We're already using a factory, but (apart from removing includes) this didn't > simplify things much. > > Consider using the following pattern: > > std::string prefs = { > prefs::kDeleteBrowsingHistory, > ... > } > > for (const std::string& pref : prefs) > AddCounter(...) > > This already improves things by explicitly listing the preferences instead of > the counters. The last thing that would remain (out of the scope of this CL) is > to auto-generate the list of preferences from the HTML/JS instead of hardcoding > it. So please update the TODO for me: > > // TODO(msramek): Get the list of deletion preferences from the JS side. Thank you! Done.
LGTM! https://codereview.chromium.org/2121203002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2121203002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:222: // TODO(msramek): Get the list of deletion preferences from the JS side. nit: This TODO now belongs to the list of prefs :)
ioanap@google.com changed reviewers: + dbeam@chromium.org
dbeam@chromium.org: Please review changes in chrome/browser/ui/webui/ Thanks, Ioana
@dbeam: friendly ping! :) Please review the changes in: chrome/browser/ui/webui/ Thank you, Ioana https://codereview.chromium.org/2121203002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2121203002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:222: // TODO(msramek): Get the list of deletion preferences from the JS side. On 2016/07/07 16:49:05, msramek wrote: > nit: This TODO now belongs to the list of prefs :) Done.
lgtm
ioanap@google.com changed reviewers: + bauerb@chromium.org
Thank you, Dan! Bernhard, please take a look at browsing_data_counter_bridge.cc. Thank you, Ioana
lgtm https://codereview.chromium.org/2121203002/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right): https://codereview.chromium.org/2121203002/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:29: if (pref_name == prefs::kDeleteBrowsingHistory) These need braces, as they are more than one line. Also, some empty lines might be nice for readability. https://codereview.chromium.org/2121203002/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:31: new HistoryCounter(profile)); Would base::WrapUnique or even base::MakeUnique work here?
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...
Thank you for reviewing this! Ioana https://codereview.chromium.org/2121203002/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right): https://codereview.chromium.org/2121203002/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:29: if (pref_name == prefs::kDeleteBrowsingHistory) On 2016/07/11 10:57:02, Bernhard Bauer wrote: > These need braces, as they are more than one line. Also, some empty lines might > be nice for readability. Done. https://codereview.chromium.org/2121203002/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:31: new HistoryCounter(profile)); On 2016/07/11 10:57:02, Bernhard Bauer wrote: > Would base::WrapUnique or even base::MakeUnique work here? base::MakeUnique looks good! Thank you! Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ioanap@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, bauerb@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2121203002/#ps180001 (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 BrowsingDataCounterFactory Add a factory to facilitate the creation of browsing data counters for when they will be fully componentized. BUG=620317 ========== to ========== Add BrowsingDataCounterFactory Add a factory to facilitate the creation of browsing data counters for when they will be fully componentized. BUG=620317 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add BrowsingDataCounterFactory Add a factory to facilitate the creation of browsing data counters for when they will be fully componentized. BUG=620317 ========== to ========== Add BrowsingDataCounterFactory Add a factory to facilitate the creation of browsing data counters for when they will be fully componentized. BUG=620317 Committed: https://crrev.com/c6d9534492f46738b63b7d8500cd411bd723aa80 Cr-Commit-Position: refs/heads/master@{#404648} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c6d9534492f46738b63b7d8500cd411bd723aa80 Cr-Commit-Position: refs/heads/master@{#404648} |
