Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716333002:
Implement important sites dialog for desktop. (Closed)
|
Created:
3 years, 9 months ago by dullweber Modified:
3 years, 7 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, dbeam+watch-settings_chromium.org, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement important sites dialog for desktop.
- Implement ui for important sites
- Add BrowserProxy method to fetch important sites
- Pass important sites back to the CBDHandler on deletion
https://screenshot.googleplex.com/RKzDTZz8ufE.png
The dialog will only show up if the "important-sites-in-cbd" flag is enabled.
The size of the site is not yet implemented, so all entries show up as "0 B".
The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works.
If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component.
BUG=698692
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2716333002
Cr-Commit-Position: refs/heads/master@{#472789}
Committed: https://chromium.googlesource.com/chromium/src/+/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e
Patch Set 1 #Patch Set 2 : fix params, add notifications and size #Patch Set 3 : fix closure issue #Patch Set 4 : move checkbox to component #Patch Set 5 : fix build #Patch Set 6 : fix html formatting #
Total comments: 2
Patch Set 7 : extract deletion logic to util class #
Total comments: 2
Patch Set 8 : rename util file and remove pure static class #
Total comments: 11
Patch Set 9 : move filter creation back, add notification status #
Total comments: 2
Patch Set 10 : add browsertest for important sites #Patch Set 11 : log histogram when important sites is shown #
Total comments: 5
Patch Set 12 : fix close button position #Patch Set 13 : rebase #Patch Set 14 : fix rebase #Patch Set 15 : revert touch target size #Patch Set 16 : remove disabled logic from js #Patch Set 17 : rebase #Patch Set 18 : add using declaration #
Total comments: 28
Patch Set 19 : cl format --js #
Total comments: 5
Patch Set 20 : fix comments #Patch Set 21 : rebase #Patch Set 22 : fix object initializer #Patch Set 23 : rebase #
Total comments: 10
Patch Set 24 : extract importantsites dialog to separate dialog #
Total comments: 1
Patch Set 25 : fix test #Patch Set 26 : rebase #Patch Set 27 : remove favicon #
Total comments: 23
Patch Set 28 : fix close event propagation #Patch Set 29 : fix comment #Patch Set 30 : change ImportantSite type declaration #
Total comments: 20
Patch Set 31 : rebase #Patch Set 32 : fix comments #
Total comments: 2
Patch Set 33 : replace task observer in -Bridge and -Handler with a callback #
Total comments: 23
Patch Set 34 : fix comments #
Total comments: 10
Patch Set 35 : add dom-if, fix compilation #Patch Set 36 : rebase #
Total comments: 4
Patch Set 37 : expose flag via loadTimeData #Patch Set 38 : rebase #Patch Set 39 : fix rebase #
Total comments: 18
Patch Set 40 : return list directly #
Total comments: 4
Patch Set 41 : fixes #Patch Set 42 : change element.$.someId to element.$$(#someId) #Dependent Patchsets: Messages
Total messages: 188 (120 generated)
Description was changed from ========== Implement important sites dialog for desktop. Add browserproxy and ui. The dialog will only show up if the "important-sites-in-cbd" flag is enabled. BUG= ========== to ========== Implement important sites dialog for desktop. Add browserproxy and ui. The dialog will only show up if the "important-sites-in-cbd" flag is enabled. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dullweber@chromium.org 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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by dullweber@chromium.org 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...
Description was changed from ========== Implement important sites dialog for desktop. Add browserproxy and ui. The dialog will only show up if the "important-sites-in-cbd" flag is enabled. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dullweber@chromium.org 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...
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. The CBD dialog will switch to Important Sites without an animation. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. The CBD dialog will switch to Important Sites without an animation. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dullweber@chromium.org changed reviewers: + msramek@chromium.org
Hi Martin, please take a look at the changes for the important storage ui.
Looks good at the first glance, but I only took a quick look for now. One important comment to unblock you :) https://codereview.chromium.org/2716333002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:279: // Delete the types protected by Important Sites with a filter, I'd prefer to avoid copy-pasting this from PrefServiceBridge, otherwise every change to the behavior will require changes in both places. Could we extract this to a common function? You can add a new file to browsing_data/ if needed.
I extracted the deletion logic from the java and js proxies to a new browsing_data_remover_util file.
dullweber@chromium.org changed reviewers: + dmurph@chromium.org
Hi Daniel, here is a first working version of the Important Storage dialog for desktop.
On 2017/03/06 15:14:28, dullweber wrote: > I extracted the deletion logic from the java and js proxies to a new > browsing_data_remover_util file. I hate to keep asking you to rename things, but the name has several problems: a) it's very similar to existing browsing_data_utils, which is confusing b) it implies that this is some generic set of utilities for BrowsingDataRemover, but it's not, and never can be, since BrowsingDataRemover will move to content/ and this file will have to stay in chrome/. c) if it's not a generic set of utility methods, but has a specific purpose, then it should be named accordingly -> in this case e.g. "browsing_data_important_sites_utils" or such
https://codereview.chromium.org/2716333002/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_util.h (right): https://codereview.chromium.org/2716333002/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_util.h:13: class BrowsingDataRemoverUtil { The style guide discourages pure static classes and suggests custom namespaces instead. https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... We probably shouldn't use the browsing_data namespace here, as that implies components/browsing_data; it seems that a common practice in these situations is to use a namespace equal to the filename.
https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_important_sites_util.cc (right): https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_important_sites_util.cc:22: std::unique_ptr<content::BrowsingDataFilterBuilder> filter_builder( Unless I'm contradicting something, can you bring the filter builder back out of this method extraction? You can test if anything is in there by calling "IsEmptyBlacklist()" if you need. Let's us be more generic in the exclusion args. But again, if I've been contradicted, don't worry about it. https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:168: return this.isClearBrowsingDataDialog_(this.dialogState_) && Please also reference https://cs.chromium.org/chromium/src/chrome/browser/engagement/important_site... And check (maybe add test) to make sure that if you ignore the dialog (don't uncheck things) 3 times (https://cs.chromium.org/chromium/src/chrome/browser/engagement/important_site..., which you can also add a method to set a testing value if you want), then it'll not show the dialog. You can optimize this as well to not bother fetching the important sites, but that's not necessarily needed. https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:248: ignoring_domains.push_back(domain); Sorry, language here is bad. Excluding means excluding from the clearing - ignoring means we aren't excluding, but we record stats on the items we don't choose - (but in the UI they are 'chosen'). So it should be opposite. Feel free to change the argument names / documentation to avoid this confusion in the future. https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:327: entry->SetInteger(REASON_BITFIELD_FIELD, info.reason_bitfield); You can extract reasons from the reason bitfield to know exactly why it was chosen. If you do, please move that enum (in the important_sites_util.cc file) to the .h file so we know to maintain those reasons.
The CQ bit was checked by dullweber@chromium.org 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...
Thanks, I fixed your comments https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_important_sites_util.cc (right): https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_important_sites_util.cc:22: std::unique_ptr<content::BrowsingDataFilterBuilder> filter_builder( On 2017/03/06 21:16:47, dmurph wrote: > Unless I'm contradicting something, can you bring the filter builder back out of > this method extraction? You can test if anything is in there by calling > "IsEmptyBlacklist()" if you need. > > Let's us be more generic in the exclusion args. But again, if I've been > contradicted, don't worry about it. I moved it back. I think the relevant part that should be extracted is the logic about the separate calls to the BrowsingDataRemover. https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:168: return this.isClearBrowsingDataDialog_(this.dialogState_) && On 2017/03/06 21:16:48, dmurph wrote: > Please also reference > https://cs.chromium.org/chromium/src/chrome/browser/engagement/important_site... > > And check (maybe add test) to make sure that if you ignore the dialog (don't > uncheck things) 3 times > (https://cs.chromium.org/chromium/src/chrome/browser/engagement/important_site..., > which you can also add a method to set a testing value if you want), then it'll > not show the dialog. You can optimize this as well to not bother fetching the > important sites, but that's not necessarily needed. I'm already passing the result of this method to javascript and I handled it in receiveImportantSites() but I will store and access it explicitly to make it more clear https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:248: ignoring_domains.push_back(domain); On 2017/03/06 21:16:48, dmurph wrote: > Sorry, language here is bad. Excluding means excluding from the clearing - > ignoring means we aren't excluding, but we record stats on the items we don't > choose - (but in the UI they are 'chosen'). So it should be opposite. > > Feel free to change the argument names / documentation to avoid this confusion > in the future. Yes the naming is a bit confusing but I think the code is already doing the right thing? When a checkbox is checked we want to delete the data for this site? All domains where the checkbox is not checked are put into excluding_domains and this vector is then passed to the filter as a blacklist. https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:327: entry->SetInteger(REASON_BITFIELD_FIELD, info.reason_bitfield); On 2017/03/06 21:16:48, dmurph wrote: > You can extract reasons from the reason bitfield to know exactly why it was > chosen. If you do, please move that enum (in the important_sites_util.cc file) > to the .h file so we know to maintain those reasons. Moving the enum would change it from ImportantReason to ImportantSitesUtil::ImportantReason and the functions in the anonymous namespace are not happy about this. I added a static function to check if a ImportantDomainInfo has notifications.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
What is the testing story here? Are you able to do end-to-end testing? https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:248: ignoring_domains.push_back(domain); On 2017/03/07 12:24:35, dullweber wrote: > On 2017/03/06 21:16:48, dmurph wrote: > > Sorry, language here is bad. Excluding means excluding from the clearing - > > ignoring means we aren't excluding, but we record stats on the items we don't > > choose - (but in the UI they are 'chosen'). So it should be opposite. > > > > Feel free to change the argument names / documentation to avoid this confusion > > in the future. > > Yes the naming is a bit confusing but I think the code is already doing the > right thing? > When a checkbox is checked we want to delete the data for this site? > All domains where the checkbox is not checked are put into excluding_domains and > this vector is then passed to the filter as a blacklist. Ah, yeah, you're right, I messed myself up. Checked means we are deleting. https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:327: entry->SetInteger(REASON_BITFIELD_FIELD, info.reason_bitfield); On 2017/03/07 12:24:35, dullweber wrote: > On 2017/03/06 21:16:48, dmurph wrote: > > You can extract reasons from the reason bitfield to know exactly why it was > > chosen. If you do, please move that enum (in the important_sites_util.cc file) > > to the .h file so we know to maintain those reasons. > > Moving the enum would change it from ImportantReason to > ImportantSitesUtil::ImportantReason and the functions in the anonymous namespace > are not happy about this. I added a static function to check if a > ImportantDomainInfo has notifications. Really? Even if you change the declarations or do a 'using ImportantReason = ImportantSitesUtil::ImportantReason;' at the top of the cc file? You can declare all of the enum values in the .h file and stuff (probably enum class it). My brain thinks this should work but I may be wrong. https://codereview.chromium.org/2716333002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:175: // TODO(dullweber): Log Histogram? You can log the same histograms as the Java code I'm guessing, as they're the same data, just different platform.
The CQ bit was checked by dullweber@chromium.org 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...
On 2017/03/07 21:20:14, dmurph wrote: > What is the testing story here? Are you able to do end-to-end testing? I added a browser test that goes through the steps of selecting cookies for deletion, clicking the clear button, deselecting a site from deletion and clicking confirm. Then I check that the right arguments are passed to the BrowserProxyMock. I haven't found a real end-to-end test for CBD yet. Maybe it is easier if I add another unittest that tests the BrowserProxy on the C++ side?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site and notification status are not yet implemented. The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented. The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
I fixed the issues you mentioned. I still need to implement fetching the size but I could do this in a separate cl because it will require a bit more code. (Fetching multiple data types from quota manager, fetching local storage, handling several callbacks...) https://codereview.chromium.org/2716333002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:279: // Delete the types protected by Important Sites with a filter, On 2017/03/06 13:00:02, msramek wrote: > I'd prefer to avoid copy-pasting this from PrefServiceBridge, otherwise every > change to the behavior will require changes in both places. Could we extract > this to a common function? You can add a new file to browsing_data/ if needed. Done. https://codereview.chromium.org/2716333002/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_util.h (right): https://codereview.chromium.org/2716333002/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_util.h:13: class BrowsingDataRemoverUtil { On 2017/03/06 15:37:34, msramek wrote: > The style guide discourages pure static classes and suggests custom namespaces > instead. > > https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... > > We probably shouldn't use the browsing_data namespace here, as that implies > components/browsing_data; it seems that a common practice in these situations is > to use a namespace equal to the filename. Done. https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:327: entry->SetInteger(REASON_BITFIELD_FIELD, info.reason_bitfield); On 2017/03/07 21:20:13, dmurph wrote: > On 2017/03/07 12:24:35, dullweber wrote: > > On 2017/03/06 21:16:48, dmurph wrote: > > > You can extract reasons from the reason bitfield to know exactly why it was > > > chosen. If you do, please move that enum (in the important_sites_util.cc > file) > > > to the .h file so we know to maintain those reasons. > > > > Moving the enum would change it from ImportantReason to > > ImportantSitesUtil::ImportantReason and the functions in the anonymous > namespace > > are not happy about this. I added a static function to check if a > > ImportantDomainInfo has notifications. > > Really? Even if you change the declarations or do a 'using ImportantReason = > ImportantSitesUtil::ImportantReason;' at the top of the cc file? You can declare > all of the enum values in the .h file and stuff (probably enum class it). My > brain thinks this should work but I may be wrong. That works, thanks! I don't think enum class is a good idea because it is used for bitwise operations all the time. https://codereview.chromium.org/2716333002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:175: // TODO(dullweber): Log Histogram? On 2017/03/07 21:20:14, dmurph wrote: > You can log the same histograms as the Java code I'm guessing, as they're the > same data, just different platform. Done.
The CQ bit was checked by dullweber@chromium.org 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.
lgtm with nit that confused me. https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:174: var haveImportantSites = this.importantSites_.length > 0; Can you capture the dialog being disabled here? In the Java code we should be reporting a 'false' here if the dialog is disabled, but in this code it looks like we would report that as 'true'. (we do this by just keeping the length at 0: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...) Actually - it looks like you handle this in the cpp side anyways? I guess I was confused because the disabled setting was checked again on line 192
dullweber@chromium.org changed reviewers: + dschuyler@chromium.org
dschuyler@chromium.org: Please review changes in chrome/browser/resources/settings/ and chrome/browser/ui/webui/settings/ https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:174: var haveImportantSites = this.importantSites_.length > 0; On 2017/03/13 22:26:38, dmurph wrote: > Can you capture the dialog being disabled here? In the Java code we should be > reporting a 'false' here if the dialog is disabled, but in this code it looks > like we would report that as 'true'. > > (we do this by just keeping the length at 0: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...) > > Actually - it looks like you handle this in the cpp side anyways? I guess I was > confused because the disabled setting was checked again on line 192 This function will return false in line 170 when the dialog is disabled. I also don't return important sites from cpp if it is disabled. I agree that the check in line 192 is redundant. Should it be removed?
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
who's the product manager and designer working on this? the UI does not look very consistent with the rest of settings
On 2017/03/15 18:14:13, Dan Beam wrote: > who's the product manager and designer working on this? the UI does not look > very consistent with the rest of settings Hi, dknow@ is the pm and chowse@ the designer working on this. There are slides with mockups that I used for the implementation at go/isd-desktop.
https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.html (right): https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:54: </paper-checkbox> Much of important_site_checkbox.* appears to be a paste of settings_checkbox. Would it make sense to add a <content select...> to settings_checkbox? I'm concerned about adding a one-off specialization.
https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.html (right): https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:54: </paper-checkbox> On 2017/03/20 19:22:38, dschuyler wrote: > Much of important_site_checkbox.* appears to be a paste of settings_checkbox. > Would it make sense to add a <content select...> to settings_checkbox? I'm > concerned about adding a one-off specialization. I initially looked at settings_checkbox but then created this control because settings_checkbox is backed by a preference while important sites are not. You are right that there is some duplication because both checkboxes should look about the same but the data behind them is different. Do you have a good idea how this could be improved? Extracting the html to a common base class seems also not like a good solution because both paper-checkbox and the cr-policy-pref-indicator access the pref. The biggest common part is the stylesheet that could be reused for both.
https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.html (right): https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:54: </paper-checkbox> On 2017/03/22 15:34:23, dullweber wrote: > On 2017/03/20 19:22:38, dschuyler wrote: > > Much of important_site_checkbox.* appears to be a paste of settings_checkbox. > > Would it make sense to add a <content select...> to settings_checkbox? I'm > > concerned about adding a one-off specialization. > > I initially looked at settings_checkbox but then created this control because > settings_checkbox is backed by a preference while important sites are not. You > are right that there is some duplication because both checkboxes should look > about the same but the data behind them is different. Do you have a good idea > how this could be improved? Extracting the html to a common base class seems > also not like a good solution because both paper-checkbox and the > cr-policy-pref-indicator access the pref. The biggest common part is the > stylesheet that could be reused for both. Hmm, does that mean that the user will need to adjust these checkboxes each time they reach this dialog (rather than saving the state like the CBD UI does)? Is it reasonable to save the values? Is there any plans to enforce (like by policy) the choices here? If we want to save the selections or allow for policy enforcement then it will be getting closer to a settings checkbox. (For the record, I'm not trying to go in that direction for the sake of using settings checkbox; instead I'm making a prediction that those will be desired and we'll end up there in the near future).
On 2017/03/23 01:35:53, dschuyler wrote: > https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... > File chrome/browser/resources/settings/controls/important_site_checkbox.html > (right): > > https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/controls/important_site_checkbox.html:54: > </paper-checkbox> > On 2017/03/22 15:34:23, dullweber wrote: > > On 2017/03/20 19:22:38, dschuyler wrote: > > > Much of important_site_checkbox.* appears to be a paste of > settings_checkbox. > > > Would it make sense to add a <content select...> to settings_checkbox? I'm > > > concerned about adding a one-off specialization. > > > > I initially looked at settings_checkbox but then created this control because > > settings_checkbox is backed by a preference while important sites are not. You > > are right that there is some duplication because both checkboxes should look > > about the same but the data behind them is different. Do you have a good idea > > how this could be improved? Extracting the html to a common base class seems > > also not like a good solution because both paper-checkbox and the > > cr-policy-pref-indicator access the pref. The biggest common part is the > > stylesheet that could be reused for both. > > Hmm, does that mean that the user will need to adjust these checkboxes each time > they reach this dialog (rather than saving the state like the CBD UI does)? Is > it reasonable to save the values? > > Is there any plans to enforce (like by policy) the choices here? > > If we want to save the selections or allow for policy enforcement then it will > be getting closer to a settings checkbox. (For the record, I'm not trying to go > in that direction for the sake of using settings checkbox; instead I'm making a > prediction that those will be desired and we'll end up there in the near > future). I think it would be a good idea to remember the last choice and preselect the specific pages but I don't think that could be done with boolean preferences because the list of important sites is dynamically created based on a number of engagement metrics. (ImportantSitesUtil::GetImportantRegisterableDomains) If an important site is not selected for a number of times it won't be shown again. (ImportantSitesUtil::RecordBlacklistedAndIgnoredImportantSites). I don't think there is a way to enforce sites by policy but that sounds like a good idea. The current approach is to bring the important sites dialog from Android to Desktop. We should discuss with dknox@ and dmurph@ what they think about adding these features but I think that could be done later.
The CQ bit was checked by dullweber@chromium.org 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...
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/nhyaSYeH0da.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented. The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/b94Okrngdeo.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented. The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dullweber@chromium.org 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...
I fixed the position of the close button and removed the red color from the notification warning.
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/b94Okrngdeo.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented. The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/b94Okrngdeo.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented, so all entries show up as "0 B". The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dullweber@chromium.org 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.
On 2017/03/14 10:32:04, dullweber wrote: > mailto:dschuyler@chromium.org: Please review changes in > chrome/browser/resources/settings/ and > chrome/browser/ui/webui/settings/ > > https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... > File > chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js > (right): > > https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:174: > var haveImportantSites = this.importantSites_.length > 0; > On 2017/03/13 22:26:38, dmurph wrote: > > Can you capture the dialog being disabled here? In the Java code we should be > > reporting a 'false' here if the dialog is disabled, but in this code it looks > > like we would report that as 'true'. > > > > (we do this by just keeping the length at 0: > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...) > > > > Actually - it looks like you handle this in the cpp side anyways? I guess I > was > > confused because the disabled setting was checked again on line 192 > > This function will return false in line 170 when the dialog is disabled. I also > don't return important sites from cpp if it is disabled. I agree that the check > in line 192 is redundant. Should it be removed? Sorry for the late review followup here, I'd probably prefer that we don't have redundant code here, I would suggest: 1. Exposing all information in web, so it has all state. Important sites are populated, but that variable is set to disabled 2. Removing the idea of a disabled dialog in the frontend, and have it just not show when there are no important sites. This way it's controlled by the backend - if it's disabled, just return no important sites. I'd probably prefer #2 - so the frontend doesn't deal with enabled/disabled, it just shows what it's given, if it's given anything.
> Sorry for the late review followup here, I'd probably prefer that we don't have > redundant code here, I would suggest: > 1. Exposing all information in web, so it has all state. Important sites are > populated, but that variable is set to disabled > 2. Removing the idea of a disabled dialog in the frontend, and have it just not > show when there are no important sites. This way it's controlled by the backend > - if it's disabled, just return no important sites. > > I'd probably prefer #2 - so the frontend doesn't deal with enabled/disabled, it > just shows what it's given, if it's given anything. That sounds like a good idea but I have one question regarding the histogram: What should actually be counted as "false"? Android skips users that have the flag disabled. In order to do this I need to know whether the dialog is enabled in the frontend. I noticed that I currently also skip users where IsDialogDisabled() returned false. That needs to be fixed. I will try to implement #2 but I still need to pass the value of the flag to the frontend to do the histogram correctly.
dbeam@, dschuyler@: I changed the notification text color and fixed the position of the close button, could you take another look at this cl? https://screenshot.googleplex.com/b94Okrngdeo.png
On 2017/03/28 08:01:53, dullweber wrote: > > Sorry for the late review followup here, I'd probably prefer that we don't > have > > redundant code here, I would suggest: > > 1. Exposing all information in web, so it has all state. Important sites are > > populated, but that variable is set to disabled > > 2. Removing the idea of a disabled dialog in the frontend, and have it just > not > > show when there are no important sites. This way it's controlled by the > backend > > - if it's disabled, just return no important sites. > > > > I'd probably prefer #2 - so the frontend doesn't deal with enabled/disabled, > it > > just shows what it's given, if it's given anything. > > That sounds like a good idea but I have one question regarding the histogram: > What should actually be counted as "false"? > Android skips users that have the flag disabled. In order to do this I need to > know whether the dialog is enabled in the frontend. > I noticed that I currently also skip users where IsDialogDisabled() returned > false. That needs to be fixed. > I will try to implement #2 but I still need to pass the value of the flag to the > frontend to do the histogram correctly. ok, that sounds good to me. sorry for the weird cases - maybe it would have been better to make that histogram an ENUM, where we can say shown, notshown_disabled, notshown_no_sites
dullweber@chromium.org changed reviewers: + dominickn@chromium.org
dominickn@chromium.org: Please review changes in chrome/browser/engagement/important_sites_util.*
engagement lgtm, though perhaps consider just having the enum in the global namespace so the namespacing is a bit simpler?
On 2017/04/06 05:31:56, dominickn wrote: > engagement lgtm, though perhaps consider just having the enum in the global > namespace so the namespacing is a bit simpler? Thanks. I'm not sure if it is a good idea to put an enum in a global namespace? I looked into converting it to enum class but it is used for bit operations everywhere. But I added a using declaration to settings_clear_browsing_data_handler.cc to make the the line, where the enum is used, a bit easier to read.
dullweber@chromium.org changed reviewers: + mariakhomenko@chromium.org
mariakhomenko@chromium.org: Please review changes in chrome/browser/android/browsing_data/browsing_data_bridge.cc I moved a few lines that are used by both the java bridge and the javascript handler to a shared util function.
On 2017/04/06 08:55:58, dullweber wrote: > mailto:mariakhomenko@chromium.org: Please review changes in > chrome/browser/android/browsing_data/browsing_data_bridge.cc > > I moved a few lines that are used by both the java bridge and the javascript > handler to a shared util function. lgtm for browsing_data_bridge change
I expect I'll need to make another pass on this CL, it's large imo. General comment: please try running git cl format --js to format the JavaScript code. https://codereview.chromium.org/2716333002/diff/340001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/app/settings_st... chrome/app/settings_strings.grdp:854: This will clear cookies and cache for all sites, including: Please remove the trailing colon (it's normally correct, but UX made a style choice to omit them). https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_important_sites_util.cc (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_important_sites_util.cc:32: // Make sure |observer| doesn't wait for the filtered task. Nit: the comment here and on line 42 seem unnecessary. i.e. it seems evident from being in the else block of this if() statement. (If the comments are trying to convey something subtle/important then my feedback is that I'm not getting that subtle/important tip from the current wording). https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.h (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.h:29: // Do not change the values here, as they are used for UMA histograms. The comment is clear, but can it be extended to point the next coder to where the a change/addition could be made properly? Maybe a "See: ....." line here? https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:24: * importantSites: Array<ImportantSite>, How about importantSites: !Array<!ImportantSite>, https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:36: * @param importantSites {!Array<ImportantSite>} !Array<!ImportantSite> https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:44: * @return {!Promise<ImportantSitesResponse>} !Promise<!ImportantSitesResponse> https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:94: font-size: 80%; This size change is unexpected. Please help me see where this is spec'd in the UX design? (I don't want to say it's right or wrong without more information). https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:109: }.bind(this)); Try chaining the .thens out linearly rather than nesting them, e.g. this.browserProxy_.initialize() .then(function() { this.$.dialog.showModal(); return this.browserProxy_.fetchImportantSites(); }.bind(this)) .then(this.receiveImportantSites.bind(this)); https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.html (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:33: del extra new-line. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:41: <div>[[site.registerableDomain]]</div> Line 41 should be indented two more spaces. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:398: <structure name="IDR_IMPORTANT_SITES_CONTROLS_CHECKBOX_HTML" Please use the pattern: IDR_SETTINGS_CONTROLS_IMPORTANT_SITE_CHECKBOX_HTML i.e. the IDR mimics the path to the file. Also s/SITES/SITE/ since the file name is singular. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:54: const char* REGISTERABLE_DOMAIN_FIELD = "registerableDomain"; The style guide suggests using kCamelCase for constants unless there is a solid reason not to (is there in this case?). Also define an array rather than a pointer. e.g. const char kRegisterableDomainField[] = "registerableDomain"; https://codereview.chromium.org/2716333002/diff/340001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/common/chrome_f... chrome/common/chrome_features.h:85: extern const base::Feature kImportantSitesInCBD; Low case letters following the first letter in an acronym e.g. kImportantSitesInCbd (in camel case names I mean, not normally). https://codereview.chromium.org/2716333002/diff/340001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2716333002/diff/340001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:49: align-self: flex-start; All the --vars go before the other entries e.g. --paper-icon-button: -webkit-margin-end: align-self: margin-top: (I know that Polymer for example sorts alphabetically, ignoring the hyphens (so it would be correct there), while this code sorts with the hyphens as part of the label (so '-' comes before 'a')).
The CQ bit was checked by dullweber@chromium.org 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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Thanks for the review. I fixed the issues you mentioned. One automatic formatting change in clear_browsing_data_dialog.js:41 seems weird. I added a comment to the file. https://codereview.chromium.org/2716333002/diff/340001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/app/settings_st... chrome/app/settings_strings.grdp:854: This will clear cookies and cache for all sites, including: On 2017/04/07 00:02:05, dschuyler wrote: > Please remove the trailing colon > (it's normally correct, but UX made a style choice to omit them). Done, thanks! https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_important_sites_util.cc (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_important_sites_util.cc:32: // Make sure |observer| doesn't wait for the filtered task. On 2017/04/07 00:02:05, dschuyler wrote: > Nit: the comment here and on line 42 seem unnecessary. i.e. it seems evident > from being in the else block of this if() statement. > (If the comments are trying to convey something subtle/important then my > feedback is that I'm not getting that subtle/important tip from the current > wording). Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.h (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.h:29: // Do not change the values here, as they are used for UMA histograms. On 2017/04/07 00:02:05, dschuyler wrote: > The comment is clear, but can it be extended to point the next coder to where > the a change/addition could be made properly? Maybe a "See: ....." line here? I just moved this enum from the .cc to the header to make it accessible to other classes. I guess that new values could be appended but I have to ask Daniel about this first. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:24: * importantSites: Array<ImportantSite>, On 2017/04/07 00:02:05, dschuyler wrote: > How about > importantSites: !Array<!ImportantSite>, Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:36: * @param importantSites {!Array<ImportantSite>} On 2017/04/07 00:02:06, dschuyler wrote: > !Array<!ImportantSite> Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:44: * @return {!Promise<ImportantSitesResponse>} On 2017/04/07 00:02:06, dschuyler wrote: > !Promise<!ImportantSitesResponse> Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:94: font-size: 80%; On 2017/04/07 00:02:06, dschuyler wrote: > This size change is unexpected. Please help me see where this is spec'd in the > UX design? > (I don't want to say it's right or wrong without more information). You're right, it shouldn't be 80%. text_defaults_md.css defines the default font-size as 81.25%. According to the mocks the subtitle should be the same font-size as other normal text. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:109: }.bind(this)); On 2017/04/07 00:02:06, dschuyler wrote: > Try chaining the .thens out linearly rather than nesting them, e.g. > > this.browserProxy_.initialize() > .then(function() { > this.$.dialog.showModal(); > return this.browserProxy_.fetchImportantSites(); > }.bind(this)) > .then(this.receiveImportantSites.bind(this)); Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.html (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:33: On 2017/04/07 00:02:06, dschuyler wrote: > del extra new-line. Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:41: <div>[[site.registerableDomain]]</div> On 2017/04/07 00:02:06, dschuyler wrote: > Line 41 should be indented two more spaces. Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:398: <structure name="IDR_IMPORTANT_SITES_CONTROLS_CHECKBOX_HTML" On 2017/04/07 00:02:06, dschuyler wrote: > Please use the pattern: > IDR_SETTINGS_CONTROLS_IMPORTANT_SITE_CHECKBOX_HTML > i.e. the IDR mimics the path to the file. > > Also s/SITES/SITE/ since the file name is singular. Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:54: const char* REGISTERABLE_DOMAIN_FIELD = "registerableDomain"; On 2017/04/07 00:02:06, dschuyler wrote: > The style guide suggests using kCamelCase for constants unless there is a solid > reason not to (is there in this case?). > Also define an array rather than a pointer. e.g. > > const char kRegisterableDomainField[] = "registerableDomain"; Done. https://codereview.chromium.org/2716333002/diff/340001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2716333002/diff/340001/chrome/common/chrome_f... chrome/common/chrome_features.h:85: extern const base::Feature kImportantSitesInCBD; On 2017/04/07 00:02:06, dschuyler wrote: > Low case letters following the first letter in an acronym e.g. > kImportantSitesInCbd > (in camel case names I mean, not normally). Done. https://codereview.chromium.org/2716333002/diff/340001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2716333002/diff/340001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:49: align-self: flex-start; On 2017/04/07 00:02:06, dschuyler wrote: > All the --vars go before the other entries e.g. > > --paper-icon-button: > -webkit-margin-end: > align-self: > margin-top: > > (I know that Polymer for example sorts alphabetically, ignoring the hyphens (so > it would be correct there), while this code sorts with the hyphens as part of > the label (so '-' comes before 'a')). Done. https://codereview.chromium.org/2716333002/diff/360001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:41: // Will be filled as results are reported. This formatting by "git cl format --js" seems odd, I would expect the comment to be indented by 2 spaces. Should I file a clang-format bug?
The CQ bit was checked by dullweber@chromium.org 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.
https://codereview.chromium.org/2716333002/diff/360001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:41: // Will be filled as results are reported. On 2017/04/07 09:39:21, dullweber wrote: > This formatting by "git cl format --js" seems odd, I would expect the comment to > be indented by 2 spaces. Should I file a clang-format bug? I agree that's weird and I would expect a two space indent here. https://codereview.chromium.org/2716333002/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:42: } Hey that helps point out another issue that I missed before: This |value| will be like a class-static value, instead of an instance (object) value. Unless a class var is really desired (which should have a comment saying so) this should instead be: value: function() { return {}; }, So that it will be an instance var. Here's a demo to illustrate the difference: http://jsbin.com/gogonufazi/3/edit?html,console,output
https://codereview.chromium.org/2716333002/diff/360001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:41: // Will be filled as results are reported. On 2017/04/07 22:57:34, dschuyler wrote: > On 2017/04/07 09:39:21, dullweber wrote: > > This formatting by "git cl format --js" seems odd, I would expect the comment > to > > be indented by 2 spaces. Should I file a clang-format bug? > > I agree that's weird and I would expect a two space indent here. I submitted a bug for clang-format https://codereview.chromium.org/2716333002/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:42: } On 2017/04/07 22:57:34, dschuyler wrote: > Hey that helps point out another issue that I missed before: > This |value| will be like a class-static value, instead of an instance (object) > value. Unless a class var is really desired (which should have a comment saying > so) this should instead be: > > value: function() { return {}; }, > > So that it will be an instance var. > Here's a demo to illustrate the difference: > http://jsbin.com/gogonufazi/3/edit?html,console,output Ah, that looks interesting, thanks. I guess the same issue exists with the default array for importantSites_? I fixed that one too.
The CQ bit was checked by dullweber@chromium.org 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.
The CQ bit was checked by dullweber@chromium.org 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.
On 2017/03/16 09:07:08, dullweber wrote: > On 2017/03/15 18:14:13, Dan Beam wrote: > > who's the product manager and designer working on this? the UI does not look > > very consistent with the rest of settings > > Hi, > > dknow@ is the pm and chowse@ the designer working on this. There are slides with > mockups that I used for the implementation at go/isd-desktop. Something that really stands out to my eye is the UI having a checkbox, icon, and two-line-text in a row.
On 2017/03/23 09:49:23, dullweber wrote: > On 2017/03/23 01:35:53, dschuyler wrote: > > > https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... > > File chrome/browser/resources/settings/controls/important_site_checkbox.html > > (right): > > > > > https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... > > chrome/browser/resources/settings/controls/important_site_checkbox.html:54: > > </paper-checkbox> > > On 2017/03/22 15:34:23, dullweber wrote: > > > On 2017/03/20 19:22:38, dschuyler wrote: > > > > Much of important_site_checkbox.* appears to be a paste of > > settings_checkbox. > > > > Would it make sense to add a <content select...> to settings_checkbox? I'm > > > > concerned about adding a one-off specialization. > > > > > > I initially looked at settings_checkbox but then created this control > because > > > settings_checkbox is backed by a preference while important sites are not. > You > > > are right that there is some duplication because both checkboxes should look > > > about the same but the data behind them is different. Do you have a good > idea > > > how this could be improved? Extracting the html to a common base class seems > > > also not like a good solution because both paper-checkbox and the > > > cr-policy-pref-indicator access the pref. The biggest common part is the > > > stylesheet that could be reused for both. > > > > Hmm, does that mean that the user will need to adjust these checkboxes each > time > > they reach this dialog (rather than saving the state like the CBD UI does)? Is > > it reasonable to save the values? > > > > Is there any plans to enforce (like by policy) the choices here? > > > > If we want to save the selections or allow for policy enforcement then it will > > be getting closer to a settings checkbox. (For the record, I'm not trying to > go > > in that direction for the sake of using settings checkbox; instead I'm making > a > > prediction that those will be desired and we'll end up there in the near > > future). > > I think it would be a good idea to remember the last choice and preselect the > specific pages but I don't think that could be done with boolean preferences > because the list of important sites is dynamically created based on a number of > engagement metrics. (ImportantSitesUtil::GetImportantRegisterableDomains) > If an important site is not selected for a number of times it won't be shown > again. (ImportantSitesUtil::RecordBlacklistedAndIgnoredImportantSites). > > I don't think there is a way to enforce sites by policy but that sounds like a > good idea. > > The current approach is to bring the important sites dialog from Android to > Desktop. We should discuss with dknox@ and dmurph@ what they think about adding > these features but I think that could be done later. So would the important_sites_checkbox be removed at that point?
https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:84: font-size: 86.67%; /* (13px / 15px) * 100 */ nit: two spaces between the ; and the /* https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:110: <div class="body" hidden$="[[!isClearBrowsingDataDialog_(dialogState_)]]"> This looks like it is changing quite a bit of the guts of this dialog, putting two dialogs in one <dialog> entry. Can this be made into two <dialog>'s instead? https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:329: bool dialog_disabled = ImportantSitesUtil::IsDialogDisabled(profile); Please avoid the generic terminology / var names here: what is the 'dialog'. I'm guessing it may be talking about the dialog that that CBD (clear browsing data) is shown in, but this code shouldn't be so tied to the UI implementation (like whether it's a dialog or something else). (If that's not what it's referring to, then my comment stands because it's still unclear what dialog is being discussed). https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:339: entry->SetInteger(kReasonBitField, info.reason_bitfield); I'm iffy on whether a bit field is a good data type to use between C++ and JS. Please tell me more about why this is a good choice (like there being precedence for it; or a spec entry saying that the bit manipulation operations will be consistent). My concerns come from bit fields being handled inconsistently by different languages (or compilers of the same language) in the past. (Twenty years ago passing a bit field between environments would have been a risky thing to do; I'm trying to stay open to hearing whether this has changed - or isn't an issue in this specific case). https://codereview.chromium.org/2716333002/diff/440001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2716333002/diff/440001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:110: margin-top: 4px; I believe this dialog is used in a variety of places. Which uses of cr dialog have been tested with this change (i.e. does this impact other UIs badly)?
On 2017/04/11 23:48:57, dschuyler wrote: > On 2017/03/23 09:49:23, dullweber wrote: > > On 2017/03/23 01:35:53, dschuyler wrote: > > > > > > https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... > > > File chrome/browser/resources/settings/controls/important_site_checkbox.html > > > (right): > > > > > > > > > https://codereview.chromium.org/2716333002/diff/200001/chrome/browser/resourc... > > > chrome/browser/resources/settings/controls/important_site_checkbox.html:54: > > > </paper-checkbox> > > > On 2017/03/22 15:34:23, dullweber wrote: > > > > On 2017/03/20 19:22:38, dschuyler wrote: > > > > > Much of important_site_checkbox.* appears to be a paste of > > > settings_checkbox. > > > > > Would it make sense to add a <content select...> to settings_checkbox? > I'm > > > > > concerned about adding a one-off specialization. > > > > > > > > I initially looked at settings_checkbox but then created this control > > because > > > > settings_checkbox is backed by a preference while important sites are not. > > You > > > > are right that there is some duplication because both checkboxes should > look > > > > about the same but the data behind them is different. Do you have a good > > idea > > > > how this could be improved? Extracting the html to a common base class > seems > > > > also not like a good solution because both paper-checkbox and the > > > > cr-policy-pref-indicator access the pref. The biggest common part is the > > > > stylesheet that could be reused for both. > > > > > > Hmm, does that mean that the user will need to adjust these checkboxes each > > time > > > they reach this dialog (rather than saving the state like the CBD UI does)? > Is > > > it reasonable to save the values? > > > > > > Is there any plans to enforce (like by policy) the choices here? > > > > > > If we want to save the selections or allow for policy enforcement then it > will > > > be getting closer to a settings checkbox. (For the record, I'm not trying to > > go > > > in that direction for the sake of using settings checkbox; instead I'm > making > > a > > > prediction that those will be desired and we'll end up there in the near > > > future). > > > > I think it would be a good idea to remember the last choice and preselect the > > specific pages but I don't think that could be done with boolean preferences > > because the list of important sites is dynamically created based on a number > of > > engagement metrics. (ImportantSitesUtil::GetImportantRegisterableDomains) > > If an important site is not selected for a number of times it won't be shown > > again. (ImportantSitesUtil::RecordBlacklistedAndIgnoredImportantSites). > > > > I don't think there is a way to enforce sites by policy but that sounds like a > > good idea. > > > > The current approach is to bring the important sites dialog from Android to > > Desktop. We should discuss with dknox@ and dmurph@ what they think about > adding > > these features but I think that could be done later. > > So would the important_sites_checkbox be removed at that point? I don't think boolean preferences would work to store the state of these checkboxes because the list of important sites is dynamically created. We could store a list of important sites that were previously selected but I don't see how we could integrate this into the settings_checkbox. Both controls already use paper-checkbox which contains most of the common functionality. I think it would be best to keep them separate.
On 2017/04/11 23:45:41, dschuyler wrote: > On 2017/03/16 09:07:08, dullweber wrote: > > On 2017/03/15 18:14:13, Dan Beam wrote: > > > who's the product manager and designer working on this? the UI does not > look > > > very consistent with the rest of settings > > > > Hi, > > > > dknow@ is the pm and chowse@ the designer working on this. There are slides > with > > mockups that I used for the implementation at go/isd-desktop. > > Something that really stands out to my eye is the UI having a checkbox, icon, > and two-line-text in a row. I think then we should continue to discuss this on the bug about this feature? (http://crbug.com/698692) The whole dialog is still behind a flag, is it ok if we continue with this CL and make changes to the UI later?
Thanks, I changed the dialog implementation to use two separate dialogs but I have trouble show the second one after I close the first (see comments). I will update the tests when I get this fixed and if the general approach is ok. https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:84: font-size: 86.67%; /* (13px / 15px) * 100 */ On 2017/04/11 23:49:32, dschuyler wrote: > nit: two spaces between the ; and the /* Done. https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:110: <div class="body" hidden$="[[!isClearBrowsingDataDialog_(dialogState_)]]"> On 2017/04/11 23:49:32, dschuyler wrote: > This looks like it is changing quite a bit of the guts of this dialog, putting > two dialogs in one <dialog> entry. Can this be made into two <dialog>'s instead? I build the dialog similar to the bluetooth_device_dialog which also uses a few conditions to switch from one state to the next without closing and reopening the whole thing. I tried to rewrite the dialog into two dialogs but I currently have the problem that I can't open a second dialog after I closed the first dialog. Everything works fine if I just open the second dialog on top of the first. I guess the cr_dialog somehow closes the whole clear_browsing_data_dialog component? Do you know if there is a way to prevent the dialog from closing it's parent? https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:329: bool dialog_disabled = ImportantSitesUtil::IsDialogDisabled(profile); On 2017/04/11 23:49:32, dschuyler wrote: > Please avoid the generic terminology / var names here: what is the 'dialog'. > I'm guessing it may be talking about the dialog that that CBD (clear browsing > data) is shown in, but this code shouldn't be so tied to the UI implementation > (like whether it's a dialog or something else). (If that's not what it's > referring to, then my comment stands because it's still unclear what dialog is > being discussed). I added "important_sites_" to both of these bools to clarify that they are about the state of the dialog showing important sites, which will be shown after a user chooses to clear browsing data. https://codereview.chromium.org/2716333002/diff/440001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:339: entry->SetInteger(kReasonBitField, info.reason_bitfield); On 2017/04/11 23:49:32, dschuyler wrote: > I'm iffy on whether a bit field is a good data type to use between C++ and JS. > Please tell me more about why this is a good choice (like there being precedence > for it; or a spec entry saying that the bit manipulation operations will be > consistent). > > My concerns come from bit fields being handled inconsistently by different > languages (or compilers of the same language) in the past. (Twenty years ago > passing a bit field between environments would have been a risky thing to do; > I'm trying to stay open to hearing whether this has changed - or isn't an issue > in this specific case). The reason_bitfield is only accessed here and in ClearBrowsingdataHandler::HandleClearBrowsingData() to call ImportantSitesUtil::RecordBlacklistedAndIgnoredImportantSites. It is never accessed from Javascript. The only bit operation is done in line 345 to extract, whether this site has notifications enabled. The only expectation is that the integer will survive being passed to Javascript and back. reason_bitfield is an int32_t and Javascript should support 53bit precision for whole numbers. If that wouldn't work correctly, it would mean that any integer could change when beeing passed to Javascript? The same is done on the Java side when passing this bitfield to the Android UI and back. https://codereview.chromium.org/2716333002/diff/440001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2716333002/diff/440001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:110: margin-top: 4px; On 2017/04/11 23:49:32, dschuyler wrote: > I believe this dialog is used in a variety of places. Which uses of cr dialog > have been tested with this change (i.e. does this impact other UIs badly)? I checked the existing page of the clear_browsing_data dialog and it looks fine. Dialogs with only a single line in the title should not be changed by this. Is there any dialog with a larger title that I should check?
The CQ bit was checked by dullweber@chromium.org 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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/b94Okrngdeo.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented, so all entries show up as "0 B". The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/RKzDTZz8ufE.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented, so all entries show up as "0 B". The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dullweber@chromium.org 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...
On 2017/04/11 23:45:41, dschuyler wrote: > On 2017/03/16 09:07:08, dullweber wrote: > > On 2017/03/15 18:14:13, Dan Beam wrote: > > > who's the product manager and designer working on this? the UI does not > look > > > very consistent with the rest of settings > > > > Hi, > > > > dknow@ is the pm and chowse@ the designer working on this. There are slides > with > > mockups that I used for the implementation at go/isd-desktop. > > Something that really stands out to my eye is the UI having a checkbox, icon, > and two-line-text in a row. I removed the favicon and updated the screenshot in the description
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2716333002/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:169: $i18n{clearBrowsingData} nit: indented too far https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_important_sites_util.h (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_important_sites_util.h:18: // Deletes the types protected by Important Sites with a filter, and the rest Does this delete the types that 'are' or 'are not' protected? https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:170: // Closing one dialog closes everything, can this be prevented? Please see email with advice from dpapad@. https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.html (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:2: <link rel="import" href="chrome://resources/html/polymer.html"> I've learned that polymer.html should be imported first (which is an exception to the sorted imports rule). https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:37: <span> · </span> nit: If the can be avoided, please remove them. (Maybe with nowrap or similar) (I'm flexible on this, it would be nice to avoid the ) https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:13: /** The ImportantSite associated with this checkbox. */ nit: consider removing comment. (if the comment is kept, the /** should change to /*) https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:16: }, site: ImportantSite, https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:18: /** Whether this checkbox should be disabled. */ nit: consider removing comment - the code seems clear enough. https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:250: CHECK(site->GetString(kRegisterableDomainField, &domain)); please swap lines 249 and 250 so the pattern is consistent. https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:328: base::FeatureList::IsEnabled(features::kImportantSitesInCbd); move line 326 down to here (so that it's closer to where it's used). https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:340: entry->SetInteger(kReasonBitField, info.reason_bitfield); Consider a comment about how the kReasonBitField is intended to be opaque to JS.
Thanks for the review and asking dpapad@ about the dialog issue. His suggestion solved the problem. https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_important_sites_util.h (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_important_sites_util.h:18: // Deletes the types protected by Important Sites with a filter, and the rest On 2017/04/27 23:10:25, dschuyler wrote: > Does this delete the types that 'are' or 'are not' protected? This deletes data from types that are considered important storage (cache and cookies) by applying the filter from |filter_builder|. Other data types e.g. history is deleted completely. I changed the comment to make it more clear https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:170: // Closing one dialog closes everything, can this be prevented? On 2017/04/27 23:10:25, dschuyler wrote: > Please see email with advice from dpapad@. Thanks for asking him. event.stopPropagation() solved it :) https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.html (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:2: <link rel="import" href="chrome://resources/html/polymer.html"> On 2017/04/27 23:10:25, dschuyler wrote: > I've learned that polymer.html should be imported first (which is an exception > to the sorted imports rule). fixed, thanks! https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.html:37: <span> · </span> On 2017/04/27 23:10:25, dschuyler wrote: > nit: If the can be avoided, please remove them. (Maybe with nowrap or > similar) (I'm flexible on this, it would be nice to avoid the ) I replaced the spaces with padding. https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:13: /** The ImportantSite associated with this checkbox. */ On 2017/04/27 23:10:25, dschuyler wrote: > nit: consider removing comment. > (if the comment is kept, the /** should change to /*) Done. https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:16: }, On 2017/04/27 23:10:25, dschuyler wrote: > site: ImportantSite, Do you mean that I should remove the object with the type attribute and replace it by just "ImportantSite"? That doesn't seem to work: [110240:110240:0428/133408.736524:ERROR:CONSOLE(1740)] "Uncaught TypeError: Cannot read property 'observer' of undefined", source: chrome://resources/polymer/v1_0/polymer/polymer-extracted.js (1740) [110240:110240:0428/133408.739644:ERROR:CONSOLE(2047)] "Uncaught TypeError: Cannot read property 'site' of undefined", source: chrome://resources/polymer/v1_0/polymer/polymer-mini-extracted.js (2047) https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:18: /** Whether this checkbox should be disabled. */ On 2017/04/27 23:10:25, dschuyler wrote: > nit: consider removing comment - the code seems clear enough. I removed all comments. I thought public properties should be commented https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:250: CHECK(site->GetString(kRegisterableDomainField, &domain)); On 2017/04/27 23:10:26, dschuyler wrote: > please swap lines 249 and 250 so the pattern is consistent. Done. https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:328: base::FeatureList::IsEnabled(features::kImportantSitesInCbd); On 2017/04/27 23:10:25, dschuyler wrote: > move line 326 down to here (so that it's closer to where it's used). Done. https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:340: entry->SetInteger(kReasonBitField, info.reason_bitfield); On 2017/04/27 23:10:25, dschuyler wrote: > Consider a comment about how the kReasonBitField is intended to be opaque to JS. Done.
https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:16: }, On 2017/04/28 12:34:03, dullweber wrote: > On 2017/04/27 23:10:25, dschuyler wrote: > > site: ImportantSite, > > Do you mean that I should remove the object with the type attribute and replace > it by just "ImportantSite"? That doesn't seem to work: > > [110240:110240:0428/133408.736524:ERROR:CONSOLE(1740)] "Uncaught TypeError: > Cannot read property 'observer' of undefined", source: > chrome://resources/polymer/v1_0/polymer/polymer-extracted.js (1740) > [110240:110240:0428/133408.739644:ERROR:CONSOLE(2047)] "Uncaught TypeError: > Cannot read property 'site' of undefined", source: > chrome://resources/polymer/v1_0/polymer/polymer-mini-extracted.js (2047) My intention was that the three lines of code site: { type: ImportantSite, }, Could be replaced with the one line of code site: ImportantSite, i.e. that one is just a syntactic variation of the other. (And that three lines of code can be reduced to one line of code that is still easy to read).
https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:16: }, On 2017/04/28 19:31:13, dschuyler wrote: > On 2017/04/28 12:34:03, dullweber wrote: > > On 2017/04/27 23:10:25, dschuyler wrote: > > > site: ImportantSite, > > > > Do you mean that I should remove the object with the type attribute and > replace > > it by just "ImportantSite"? That doesn't seem to work: > > > > [110240:110240:0428/133408.736524:ERROR:CONSOLE(1740)] "Uncaught TypeError: > > Cannot read property 'observer' of undefined", source: > > chrome://resources/polymer/v1_0/polymer/polymer-extracted.js (1740) > > [110240:110240:0428/133408.739644:ERROR:CONSOLE(2047)] "Uncaught TypeError: > > Cannot read property 'site' of undefined", source: > > chrome://resources/polymer/v1_0/polymer/polymer-mini-extracted.js (2047) > > My intention was that the three lines of code > > site: { > type: ImportantSite, > }, > > Could be replaced with the one line of code > > site: ImportantSite, > > i.e. that one is just a syntactic variation of the other. (And that three lines > of code can be reduced to one line of code that is still easy to read). That doesn't seem to work. I guess the problem is that |ImportantSite| is declared as "var ImportantSite;" with some magic comments above it but its value is "undefined". Changing it to /** @type {ImportantSite} */ site: Object, works.
lgtm https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/520001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:16: }, On 2017/05/02 10:38:45, dullweber wrote: > On 2017/04/28 19:31:13, dschuyler wrote: > > On 2017/04/28 12:34:03, dullweber wrote: > > > On 2017/04/27 23:10:25, dschuyler wrote: > > > > site: ImportantSite, > > > > > > Do you mean that I should remove the object with the type attribute and > > replace > > > it by just "ImportantSite"? That doesn't seem to work: > > > > > > [110240:110240:0428/133408.736524:ERROR:CONSOLE(1740)] "Uncaught TypeError: > > > Cannot read property 'observer' of undefined", source: > > > chrome://resources/polymer/v1_0/polymer/polymer-extracted.js (1740) > > > [110240:110240:0428/133408.739644:ERROR:CONSOLE(2047)] "Uncaught TypeError: > > > Cannot read property 'site' of undefined", source: > > > chrome://resources/polymer/v1_0/polymer/polymer-mini-extracted.js (2047) > > > > My intention was that the three lines of code > > > > site: { > > type: ImportantSite, > > }, > > > > Could be replaced with the one line of code > > > > site: ImportantSite, > > > > i.e. that one is just a syntactic variation of the other. (And that three > lines > > of code can be reduced to one line of code that is still easy to read). > > That doesn't seem to work. I guess the problem is that |ImportantSite| is > declared as "var ImportantSite;" with some magic comments above it but its value > is "undefined". Changing it to > > /** @type {ImportantSite} */ > site: Object, > > works. You're right. Sorry, I totally wrote the wrong thing as an example. You did find the exact thing I was *trying* to reference, thanks!
dullweber@chromium.org changed reviewers: + michaelpg@chromium.org
michaelpg@chromium.org: Please review changes in ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html and chrome/browser/ui/webui/metrics_handler.* I changed the css in cr_dialog to align the close button with the top of the title. This doesn't change the layout for dialogs with a single line in the title but looks much better when there is more than one line. (more discussion and screenshots on the bug)
Uhh, the parts you asked me to review look fine, but I had some other notes :-P https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:10: /** add a comment here: what is an important site/what's it for? https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:84: font-size: 86.67%; /* (13px / 15px) * 100 */ nit: something like font-size: calc(13/15 * 100%); would remove the magic number and the need for a comment https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:111: receiveImportantSites: function(result) { add trailing underscore for @private https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:144: /** @private */ document @return value https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:148: if (!this.$.cookiesCheckbox.checked && !this.$.cacheCheckbox.checked) { nit: no {} here for consistency with above https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:7: * `important-site-checkbox` is a checkbox that displays an important site. again, try to explain what an important site is or does. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:21: * @param {string} site The url of the site to fetch the icon for. site => url to match parameter (is closure running on this?) https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:28: nit: remove blank line https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:408: file="controls/important_site_checkbox.js" add this to compiled_resources2.gyp so the closure compiler checks it https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:237: std::vector<std::string> excluding_domains; could these added lines be moved to a helper function to keep this function clear and manageable?
The CQ bit was checked by dullweber@chromium.org 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...
Thanks for looking at everything. I fixed the issues you mentioned https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:10: /** On 2017/05/03 21:48:05, michaelpg wrote: > add a comment here: what is an important site/what's it for? Done. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:84: font-size: 86.67%; /* (13px / 15px) * 100 */ On 2017/05/03 21:48:05, michaelpg wrote: > nit: something like > font-size: calc(13/15 * 100%); > would remove the magic number and the need for a comment thanks! I fixed it and also replaced the magic number in cr_dialog where I took this from. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:111: receiveImportantSites: function(result) { On 2017/05/03 21:48:05, michaelpg wrote: > add trailing underscore for @private Done. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:144: /** @private */ On 2017/05/03 21:48:05, michaelpg wrote: > document @return value Done. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:148: if (!this.$.cookiesCheckbox.checked && !this.$.cacheCheckbox.checked) { On 2017/05/03 21:48:05, michaelpg wrote: > nit: no {} here for consistency with above Done. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:7: * `important-site-checkbox` is a checkbox that displays an important site. On 2017/05/03 21:48:06, michaelpg wrote: > again, try to explain what an important site is or does. Done. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:21: * @param {string} site The url of the site to fetch the icon for. On 2017/05/03 21:48:06, michaelpg wrote: > site => url to match parameter (is closure running on this?) fixed and I added it to the closure compilation https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:28: On 2017/05/03 21:48:06, michaelpg wrote: > nit: remove blank line Done. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:408: file="controls/important_site_checkbox.js" On 2017/05/03 21:48:06, michaelpg wrote: > add this to compiled_resources2.gyp so the closure compiler checks it Done. https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/580001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:237: std::vector<std::string> excluding_domains; On 2017/05/03 21:48:06, michaelpg wrote: > could these added lines be moved to a helper function to keep this function > clear and manageable? I extracted the lines to a function that takes a base::ListValue and returns a BrowsingDataFilterBuilder. Thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks! lgtm
The CQ bit was checked by dullweber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org, dominickn@chromium.org, mariakhomenko@chromium.org, dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2716333002/#ps620001 (title: "fix comments")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2716333002/diff/620001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_important_sites_util.h (right): https://codereview.chromium.org/2716333002/diff/620001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_important_sites_util.h:21: void Remove(int remove_mask, When I call a method and pass it an |observer|, my expectation is that it will call |observer| once when finished. This method will always call it twice. Note that we are using a special class in browsing_data_bridge which converts the two callbacks into one. At the very least, please document the behavior; but ideally, move that class into browsing_data_important_sites_util.cc and merge the two callbacks. The caller has no need to know that we require two calls to BrowsingDataRemover to execute this deletion.
The CQ bit was checked by dullweber@chromium.org 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...
Thanks! I reduced the task_observers for android and javascript with some duplicate code to a single observer. https://codereview.chromium.org/2716333002/diff/620001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_important_sites_util.h (right): https://codereview.chromium.org/2716333002/diff/620001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_important_sites_util.h:21: void Remove(int remove_mask, On 2017/05/05 09:12:59, msramek (recovering) wrote: > When I call a method and pass it an |observer|, my expectation is that it will > call |observer| once when finished. > > This method will always call it twice. Note that we are using a special class in > browsing_data_bridge which converts the two callbacks into one. > > At the very least, please document the behavior; but ideally, move that class > into browsing_data_important_sites_util.cc and merge the two callbacks. The > caller has no need to know that we require two calls to BrowsingDataRemover to > execute this deletion. I moved the Observer class to the util file and pass a OnceCallback instead. Previously the task_observer was destroyed in HandleInitialize and JavascriptDisabled. I replaced these resets with a weak_ptr invalidation. This will also kill all other requests using the weak_ptr_factory but I guess that it was the intention to not receive responses from old requests after these events anyway?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. To your question - yes, but please document it! https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h:107: // A weak pointer factory for asynchronous calls referencing this class. Please document when the pointers are invalidated. The invalidations that we do in HandleInitialize() and OnJavascriptDisallowed() make sense, since they assure that requests using weak pointers would only live between opening and closing the dialog. This behavior also works fine for other users of this |weak_ptr_factory_|, which are the callbacks updating the footer. But it still might be unexpected for future users.
not lgtm yet https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:198: <dialog is="cr-dialog" id="importantSitesDialog" close-text="$i18n{close}" does this need to be in the same file? why isn't it behind an dom-if? https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:173: jsdoc https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:180: jsdoc https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:199: this.$.importantSitesDialog.close(); can we share this code between onHistoryDeletionDialogClose_? https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:233: remove extra \n https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:30: return 'background-image: ' + cr.icon.getFavicon(url); where is this used? https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:291: base::FeatureList::IsEnabled(features::kImportantSitesInCbd); can we just send this separately? and only ask for important sites if enabled? https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h:25: class BrowsingDataFilterBuilder; alpha
https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:198: <dialog is="cr-dialog" id="importantSitesDialog" close-text="$i18n{close}" On 2017/05/05 17:52:50, Dan Beam wrote: > does this need to be in the same file? > > why isn't it behind an dom-if? It doesn't need to be in the same file but it reduces duplication between the dialog and the clearBrowsingDataDialog because both dialogs have to access clearingInProgress_ and both might have to call clearBrowsingData_() when confirmed. I can put the dialog behind a dom-if. What is the advantage? Doesn't the dialog already handle this itself by being hidden until dialog.open() is called? https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:173: On 2017/05/05 17:52:51, Dan Beam wrote: > jsdoc Done. https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:180: On 2017/05/05 17:52:51, Dan Beam wrote: > jsdoc Done. https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:199: this.$.importantSitesDialog.close(); On 2017/05/05 17:52:51, Dan Beam wrote: > can we share this code between onHistoryDeletionDialogClose_? Done. https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:233: On 2017/05/05 17:52:51, Dan Beam wrote: > remove extra \n Done. https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/important_site_checkbox.js (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/controls/important_site_checkbox.js:30: return 'background-image: ' + cr.icon.getFavicon(url); On 2017/05/05 17:52:51, Dan Beam wrote: > where is this used? It actually isn't used anymore. Thanks https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:291: base::FeatureList::IsEnabled(features::kImportantSitesInCbd); On 2017/05/05 17:52:51, Dan Beam wrote: > can we just send this separately? and only ask for important sites if enabled? I could send this separately but I already skip fetching important sites if the flag is disabled or ImportantSitesUtil::IsDialogDisabled() returns true. Splitting this into two methods would not make it any faster. The only reason to send this flag to Javascript is to correctly log the "History.ClearBrowsingData.ImportantDialogShown" histogram. https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h:25: class BrowsingDataFilterBuilder; On 2017/05/05 17:52:51, Dan Beam wrote: > alpha I sorted the declarations https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h:107: // A weak pointer factory for asynchronous calls referencing this class. On 2017/05/05 15:41:55, msramek (recovering) wrote: > Please document when the pointers are invalidated. > > The invalidations that we do in HandleInitialize() and OnJavascriptDisallowed() > make sense, since they assure that requests using weak pointers would only live > between opening and closing the dialog. This behavior also works fine for other > users of this |weak_ptr_factory_|, which are the callbacks updating the footer. > > But it still might be unexpected for future users. Done.
i'm not confident your code compiles or runs correctly. can we fix that? https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:78: remove extra \n https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:204: closeDialogs_(); fairly sure this should be this.closeDialogs_() and otherwise this will blow up at runtime https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:213: closeDialogs_() { this is an ES6 feature (object literal extensions) i don't think your code will compile when use_vulcanize = true in your GN args (which is the default) https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:244: closeDialogs_(); same (should be this.closeDialogs_()) https://codereview.chromium.org/2716333002/diff/660001/chrome/test/data/webui... File chrome/test/data/webui/settings/privacy_page_test.js (right): https://codereview.chromium.org/2716333002/diff/660001/chrome/test/data/webui... chrome/test/data/webui/settings/privacy_page_test.js:271: ] ] -> ];
https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:198: <dialog is="cr-dialog" id="importantSitesDialog" close-text="$i18n{close}" On 2017/05/09 08:56:39, dullweber wrote: > On 2017/05/05 17:52:50, Dan Beam wrote: > > does this need to be in the same file? > > > > why isn't it behind an dom-if? > > It doesn't need to be in the same file but it reduces duplication between the > dialog and the clearBrowsingDataDialog because both dialogs have to access > clearingInProgress_ and both might have to call clearBrowsingData_() when > confirmed. > > I can put the dialog behind a dom-if. What is the advantage? Doesn't the dialog > already handle this itself by being hidden until dialog.open() is called? it potentially slows down showing the clear browsing data dialog https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:291: base::FeatureList::IsEnabled(features::kImportantSitesInCbd); On 2017/05/09 08:56:39, dullweber wrote: > On 2017/05/05 17:52:51, Dan Beam wrote: > > can we just send this separately? and only ask for important sites if > enabled? > > I could send this separately but I already skip fetching important sites if the > flag is disabled or ImportantSitesUtil::IsDialogDisabled() returns true. > Splitting this into two methods would not make it any faster. > The only reason to send this flag to Javascript is to correctly log the > "History.ClearBrowsingData.ImportantDialogShown" histogram. i'd just like to do as little as possible when this flag is off. it doesn't make much sense to make to ask for the important sites if there's an easy way to know the feature is disabled.
The CQ bit was checked by dbeam@chromium.org 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...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dullweber@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dullweber@chromium.org 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.
https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:198: <dialog is="cr-dialog" id="importantSitesDialog" close-text="$i18n{close}" On 2017/05/09 19:25:03, Dan Beam wrote: > On 2017/05/09 08:56:39, dullweber wrote: > > On 2017/05/05 17:52:50, Dan Beam wrote: > > > does this need to be in the same file? > > > > > > why isn't it behind an dom-if? > > > > It doesn't need to be in the same file but it reduces duplication between the > > dialog and the clearBrowsingDataDialog because both dialogs have to access > > clearingInProgress_ and both might have to call clearBrowsingData_() when > > confirmed. > > > > I can put the dialog behind a dom-if. What is the advantage? Doesn't the > dialog > > already handle this itself by being hidden until dialog.open() is called? > > it potentially slows down showing the clear browsing data dialog ok, I moved the dialog into a dom-if https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:291: base::FeatureList::IsEnabled(features::kImportantSitesInCbd); On 2017/05/09 19:25:03, Dan Beam wrote: > On 2017/05/09 08:56:39, dullweber wrote: > > On 2017/05/05 17:52:51, Dan Beam wrote: > > > can we just send this separately? and only ask for important sites if > > enabled? > > > > I could send this separately but I already skip fetching important sites if > the > > flag is disabled or ImportantSitesUtil::IsDialogDisabled() returns true. > > Splitting this into two methods would not make it any faster. > > The only reason to send this flag to Javascript is to correctly log the > > "History.ClearBrowsingData.ImportantDialogShown" histogram. > > i'd just like to do as little as possible when this flag is off. > > it doesn't make much sense to make to ask for the important sites if there's an > easy way to know the feature is disabled. Is there another way to access flags from Javascript? If I have one method to get the flag and another one to get the sites, I would do the same amount of work when the flag is disabled and more work when the flag is enabled. I already don't fetch important_sites when they are disabled (line 297). The plan is also to enable this flag when the feature launches, so that we would always have to retrieve important sites. https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:78: On 2017/05/09 19:23:18, Dan Beam wrote: > remove extra \n Done. https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:204: closeDialogs_(); On 2017/05/09 19:23:18, Dan Beam wrote: > fairly sure this should be this.closeDialogs_() and otherwise this will blow up > at runtime Done. https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:213: closeDialogs_() { On 2017/05/09 19:23:18, Dan Beam wrote: > this is an ES6 feature (object literal extensions) > > i don't think your code will compile when use_vulcanize = true in your GN args > (which is the default) sorry, I compiled without vulcanize and I also just noticed that the try bots failed. Now it's tested with vulcanize enabled https://codereview.chromium.org/2716333002/diff/660001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:244: closeDialogs_(); On 2017/05/09 19:23:18, Dan Beam wrote: > same (should be this.closeDialogs_()) Done. https://codereview.chromium.org/2716333002/diff/660001/chrome/test/data/webui... File chrome/test/data/webui/settings/privacy_page_test.js (right): https://codereview.chromium.org/2716333002/diff/660001/chrome/test/data/webui... chrome/test/data/webui/settings/privacy_page_test.js:271: ] On 2017/05/09 19:23:18, Dan Beam wrote: > ] -> ]; Done.
this is looking really good! https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/640001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:291: base::FeatureList::IsEnabled(features::kImportantSitesInCbd); On 2017/05/11 13:13:43, dullweber wrote: > On 2017/05/09 19:25:03, Dan Beam wrote: > > On 2017/05/09 08:56:39, dullweber wrote: > > > On 2017/05/05 17:52:51, Dan Beam wrote: > > > > can we just send this separately? and only ask for important sites if > > > enabled? > > > > > > I could send this separately but I already skip fetching important sites if > > the > > > flag is disabled or ImportantSitesUtil::IsDialogDisabled() returns true. > > > Splitting this into two methods would not make it any faster. > > > The only reason to send this flag to Javascript is to correctly log the > > > "History.ClearBrowsingData.ImportantDialogShown" histogram. > > > > i'd just like to do as little as possible when this flag is off. > > > > it doesn't make much sense to make to ask for the important sites if there's > an > > easy way to know the feature is disabled. > > Is there another way to access flags from Javascript? > > If I have one method to get the flag and another one to get the sites, I would > do the same amount of work when the flag is disabled and more work when the flag > is enabled. I already don't fetch important_sites when they are disabled (line > 297). The plan is also to enable this flag when the feature launches, so that we > would always have to retrieve important sites. yes, here's an example of passing a boolean of whether a feature is enabled to JS: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/md_sett... you'd want to do something like html_source->AddBoolean("importantSitesInCbd", base::FeatureList::IsEnabled(features::kImportantSitesInCbd)); inside of AddClearBrowsingDataStrings(): https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/md_sett... which should generally be available fairly easily from JS: if (loadTimeData.getBoolean('importantSitesInCbd')) { ... } you can also just give a `value: function() {...}` to a property to compute the initial value: properties: { impotantSitesInCbd_: { type: Boolean, value: function() { return loadTimeData.getBoolean('importantSitesInCbd'); }, }, }, https://codereview.chromium.org/2716333002/diff/700001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/700001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:181: Polymer.dom.flush(); // Wait for dom-if. i think using this.async() is preferred to Polymer.dom.flush() https://codereview.chromium.org/2716333002/diff/700001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:220: } nit: no curlies
The CQ bit was checked by dullweber@chromium.org 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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Thanks for explaining how to use flags in Javascript. I fixed this and the other issues you mentioned. https://codereview.chromium.org/2716333002/diff/700001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/700001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:181: Polymer.dom.flush(); // Wait for dom-if. On 2017/05/16 03:25:25, Dan Beam wrote: > i think using this.async() is preferred to Polymer.dom.flush() Done. https://codereview.chromium.org/2716333002/diff/700001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:220: } On 2017/05/16 03:25:25, Dan Beam wrote: > nit: no curlies Done.
The CQ bit was checked by dullweber@chromium.org 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_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dullweber@chromium.org 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_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/2716333002/diff/760001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:32: var ImportantSitesResponse; why can't we just return !Array<!ImportantSite> and drop this @typedef? https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:47: * Fetches a list of important sites. ident off https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:51: fetchImportantSites: function() {}, we have many methods that ask the C++ for data, but most of them start with get. can this be getImportantSites? https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:230: $i18n{importantSitesConfirm} indent off (should be 2 spaces less) https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:119: initializePromise.then(this.fetchImportantSites_.bind(this)); why can't this just be this.browserProxy_.initialize().then(function() { this.$.clearBrowsingDataDialog.showModal(); }); if (this.importantSitesFlagEnabled_) { this.browserProxy_.getImportantSites().then(function(sites) { this.importantSites_ = sites; }.bind(this)); } that'd simplify a bunch of stuff does the dialog needs to be initialized just to store this.importantSites_? https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:289: ImportantSitesUtil::IsDialogDisabled(profile); can we just DCHECK(!ImportantSitesUtil::IsDialogDisabled(profile)); or something now? https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:315: result->Set(kImportantSitesField, std::move(important_sites_list)); why are we creating a dictionary with one field rather than just resolving with important_sites_list directly? https://codereview.chromium.org/2716333002/diff/760001/chrome/test/data/webui... File chrome/test/data/webui/settings/privacy_page_test.js (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/test/data/webui... chrome/test/data/webui/settings/privacy_page_test.js:28: this.importantSitesResponse_ = {importantSites: []}; if we change to just an array all of this codes gets a little simpler as well
Thanks, I changed the dictionary response to a list. https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:32: var ImportantSitesResponse; On 2017/05/16 19:46:08, Dan Beam wrote: > why can't we just return !Array<!ImportantSite> and drop this @typedef? That works https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:47: * Fetches a list of important sites. On 2017/05/16 19:46:09, Dan Beam wrote: > ident off Done. https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:51: fetchImportantSites: function() {}, On 2017/05/16 19:46:09, Dan Beam wrote: > we have many methods that ask the C++ for data, but most of them start with get. > can this be getImportantSites? sure https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:230: $i18n{importantSitesConfirm} On 2017/05/16 19:46:09, Dan Beam wrote: > indent off (should be 2 spaces less) Done. https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:119: initializePromise.then(this.fetchImportantSites_.bind(this)); On 2017/05/16 19:46:09, Dan Beam wrote: > why can't this just be > > this.browserProxy_.initialize().then(function() { > this.$.clearBrowsingDataDialog.showModal(); > }); > > if (this.importantSitesFlagEnabled_) { > this.browserProxy_.getImportantSites().then(function(sites) { > this.importantSites_ = sites; > }.bind(this)); > } > > that'd simplify a bunch of stuff > > does the dialog needs to be initialized just to store this.importantSites_? It doesn't need to wait for the dialog but I thought that I let it do the work that is needed immediately first and then get important sites but it probably doesn't matter. I simplified it. https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:289: ImportantSitesUtil::IsDialogDisabled(profile); On 2017/05/16 19:46:09, Dan Beam wrote: > can we just > > DCHECK(!ImportantSitesUtil::IsDialogDisabled(profile)); > > or something now? ImportantSitesUtil::IsDialogDisabled is not checking for the flag but for other conditions like "did the user ignore the important sites dialog recently". https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:315: result->Set(kImportantSitesField, std::move(important_sites_list)); On 2017/05/16 19:46:09, Dan Beam wrote: > why are we creating a dictionary with one field rather than just resolving with > important_sites_list directly? Done. https://codereview.chromium.org/2716333002/diff/760001/chrome/test/data/webui... File chrome/test/data/webui/settings/privacy_page_test.js (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/test/data/webui... chrome/test/data/webui/settings/privacy_page_test.js:28: this.importantSitesResponse_ = {importantSites: []}; On 2017/05/16 19:46:09, Dan Beam wrote: > if we change to just an array all of this codes gets a little simpler as well Done.
lgtm, thanks for sticking with it! https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:289: ImportantSitesUtil::IsDialogDisabled(profile); On 2017/05/17 09:57:37, dullweber wrote: > On 2017/05/16 19:46:09, Dan Beam wrote: > > can we just > > > > DCHECK(!ImportantSitesUtil::IsDialogDisabled(profile)); > > > > or something now? > > ImportantSitesUtil::IsDialogDisabled is not checking for the flag but for other > conditions like "did the user ignore the important sites dialog recently". then maybe DCHECK() that the feature is enabled? (which is what I thought this was checking) https://codereview.chromium.org/2716333002/diff/780001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2716333002/diff/780001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:33: * @param importantSites {!Array<!ImportantSite>} nit: we use this order everywhere else @param {!Array<!ImportantSite>} importantSites https://codereview.chromium.org/2716333002/diff/780001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:40: * Get a list of important sites. nit: this description doesn't tell us much more than getImportantSites already does
The CQ bit was checked by dullweber@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dullweber@chromium.org 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...
Thanks! I had to fix a few presubmit failures in privacy_page_test.js due to the checks you added yesterday. (Please only use this.$.localId, not element.$.localId) https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2716333002/diff/760001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:289: ImportantSitesUtil::IsDialogDisabled(profile); On 2017/05/17 20:57:28, Dan Beam wrote: > On 2017/05/17 09:57:37, dullweber wrote: > > On 2017/05/16 19:46:09, Dan Beam wrote: > > > can we just > > > > > > DCHECK(!ImportantSitesUtil::IsDialogDisabled(profile)); > > > > > > or something now? > > > > ImportantSitesUtil::IsDialogDisabled is not checking for the flag but for > other > > conditions like "did the user ignore the important sites dialog recently". > > then maybe DCHECK() that the feature is enabled? (which is what I thought this > was checking) DCHECK added https://codereview.chromium.org/2716333002/diff/780001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js (right): https://codereview.chromium.org/2716333002/diff/780001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:33: * @param importantSites {!Array<!ImportantSite>} On 2017/05/17 20:57:28, Dan Beam wrote: > nit: we use this order everywhere else > > @param {!Array<!ImportantSite>} importantSites Done. https://codereview.chromium.org/2716333002/diff/780001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js:40: * Get a list of important sites. On 2017/05/17 20:57:28, Dan Beam wrote: > nit: this description doesn't tell us much more than getImportantSites already > does removed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dullweber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, mariakhomenko@chromium.org, dmurph@chromium.org, dschuyler@chromium.org, michaelpg@chromium.org, msramek@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2716333002/#ps820001 (title: "change element.$.someId to element.$$(#someId)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 820001, "attempt_start_ts": 1495110623348400,
"parent_rev": "1d5239a5dc73c22ee1a752ed2717d2c14881c292", "commit_rev":
"f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e"}
Message was sent while issue was closed.
Description was changed from ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/RKzDTZz8ufE.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented, so all entries show up as "0 B". The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement important sites dialog for desktop. - Implement ui for important sites - Add BrowserProxy method to fetch important sites - Pass important sites back to the CBDHandler on deletion https://screenshot.googleplex.com/RKzDTZz8ufE.png The dialog will only show up if the "important-sites-in-cbd" flag is enabled. The size of the site is not yet implemented, so all entries show up as "0 B". The CBD dialog will switch to Important Sites without an animation similar to how the bluetooth_device_dialog works. If we want a nice swipe animation I would need to investigate further on how this could be done inside the Dialog component. BUG=698692 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2716333002 Cr-Commit-Position: refs/heads/master@{#472789} Committed: https://chromium.googlesource.com/chromium/src/+/f5de69bdf1d350ce1bc8b4500b54... ==========
Message was sent while issue was closed.
Committed patchset #42 (id:820001) as https://chromium.googlesource.com/chromium/src/+/f5de69bdf1d350ce1bc8b4500b54...
Message was sent while issue was closed.
On 2017/05/18 10:41:54, dullweber wrote:
> Thanks! I had to fix a few presubmit failures in privacy_page_test.js due to
the
> checks you added yesterday. (Please only use this.$.localId, not
> element.$.localId)
yeah, it wasn't intentional to check files outside of chrome/browser/resources.
I think it's probably OK to use item.$.localId from /test/ code (I saw you just
switched to $$('#localId') to get around -- good workaround!). I'm fixing that
glitch here:
https://codereview.chromium.org/2889113002/diff/60001/tools/web_dev_style/js_...
sorry for the inconvenience
|
