i'm not a huge fan of the current design, can we work to improve it?

https://screenshot.googleplex.com/nhyaSYeH0da.png

Could you provide a bit more context on what part of the design you'd like to see changes for, along with why the current design isn't working for you?

A few issues we had discussed:
1) The checkbox + favicon + text rows look off. I don't know of any other MD UI that has a control + icon on the left like that.
2) The bright red "Notifications will be disabled" was changed from the original mocks and seems a bit too aggressive
3) I don't know what the "0" means in each of the rows
4) The "This will clear cookies..." text shouldn't be a part of the dialog header, it should be in the content area.

I'll let bettes@ weigh in if there's anything else he noticed.

All of these sound reasonable. +chowse in case he has any thoughts.

1) What is the usual pattern for this? Moving to that SGTM.
2) Agreed that the bright red is a little harsh here. Perhaps we should just use the normal grey?
3) Me either :) @Chris/Christian, any insight?
4) That also sounds fine to move!

1.) No precedent AFAIK. The icons could move to the right, but then they'd become completely decoupled from the list item's text. +bettes if he has any suggestions.

2.) *Definitely* too aggressive. I'm actually tempted to just revert it back to the standard secondary text color. The difference in string length alone should help call attention it.

3.) Isn't that supposed to be the amount of storage being used by the origin? If so, it should have a size qualifier, even if it's empty (e.g. "0 KB" or "0 MB").

4.) If by "in the content area" you mean inside the grey lines, I disagree. Visually, I think that would appear bizarre and would prefer to keep it out of the scrollable region. +bettes if he has any opinion on this.

Other observations:

5.) The X button should be a fixed distance from the top-right corner. It shouldn't be vertically centered relative to the header/intro text.

6.) The horizontal padding around the Confirm button seems too narrow to me. I thought 16dp was the standard for Material/Harmony buttons.

2.) I will remove the red color here.
3.) Yes that is the size indicator. The 0 is currently more of a placeholder. I'm currently working on a second CL to fetch the used quota and localstorage size for each entry but I was ooo for the last days. The size will have a proper size qualifier then.
5 + 6) Thanks, I will fix the layout here.

I changed the positioning of the close button and removed the red color from the notification warning. https://screenshot.googleplex.com/b94Okrngdeo.png

Previous dialogs with the default title height of 52px will show no visual difference (https://screenshot.googleplex.com/mf9wgzS9jrd.png), dialogs with more height will have the X move up a bit with 16px margin to the top and right.

The margin of the confirm button is currently 10px on the top and 12px on the bottom in all dialogs. Should this be changed to 16,16 everywhere or should I leave it as is?

During codereview Dave mentioned again that the combination of a checkbox, a favicon and two lines of text look weird. (https://codereview.chromium.org/2716333002/#msg97)

Does anyone have a good idea on how this could be improved?
I think the favicons are helpful to quickly identify a page, so they should appear somewhere. 
The new clear browsing data dialog on Android has icons and checkboxes on opposite sites of the dialog, maybe that would be a better solution? http://docs/presentation/d/1NI9SR2DpFKAsAmvB0-Bn1QydWKruwNzzTro_B88EsTo/edit#slide=id.g1b4e5636bf_1_5

I spoke with one of our visual designers, and she recommended either removing the favicon altogether (ex. https://screenshot.googleplex.com/pmmiDPSVLg5.png) or to lay the content out like a table (ex. https://screenshot.googleplex.com/bpsuDjPVBzc.png). My preference would be for the former, since moving the size/warning to the right feels a little too detached and less scannable. We could also shift the favicons to the right (ex. https://screenshot.googleplex.com/rReHSXS54AC.png), but I think it eliminates the primary benefit of the favicons, which is to make the list more scannable (your gaze would have to zig-zag to make the connection between icons and what's being cleared). Moving the checkbox to the right doesn't follow Harmony guidelines, so I wouldn't recommend it either.

Strawman idea: The favicon is related to the name of the website (primary checkbox text), but not the counter (secondary text). Therefore, what if we inlined it with the primary text?

Thanks! I removed the favicon from the pending cl. The dialog layout changed a bit due to changes to the clear browsing data dialog and now it looks like this: https://screenshot.googleplex.com/RKzDTZz8ufE.png

@dullweber That looks fine. On nit: I don't know if it relates to the CBD changes, but what happened to the grey dividers at the top and bottom of the list view (e.g. https://screenshot.googleplex.com/1hfx54GMcFg.png)? Without them, the gap between the heading and the first list item looks way too big.

@msramkek I explored that option earlier (see https://screenshot.googleplex.com/QqBWE4DWmBc.png), but dismissed it since a) the standard 16x16 favicon looks too large positioned inline with 14dp text, b) creates a ragged left edge that's more difficult to scan and c) is not consistent with any other Material/Harmony patterns I'm aware of (except Android N+ notifications, which use a much smaller monochrome icon).

The divider was removed in a CL unrelated to my change. The Clear browsing data dialog is affected too (https://screenshot.googleplex.com/RKzDTZz8ufE.png). It also has a big gap but the gap in the important-sites dialog is a bit bigger because the list item has some top-padding. I think the space between the last list item and the button is also too big but these things should probably be fixed for the base dialog?

In my opinion there is also way too much empty space between the list entries. The CBD dialog in the old UI is about half the size of the new one.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e

commit f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e
Author: dullweber <dullweber@chromium.org>
Date: Thu May 18 13:20:37 2017

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}

[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/app/settings_strings.grdp
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/BUILD.gn
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/about_flags.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/android/browsing_data/browsing_data_bridge.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/android/chrome_feature_list.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/android/chrome_feature_list.h
[add] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/browsing_data/browsing_data_important_sites_util.cc
[add] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/browsing_data/browsing_data_important_sites_util.h
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/engagement/important_sites_util.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/engagement/important_sites_util.h
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/resources/settings/controls/compiled_resources2.gyp
[add] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/resources/settings/controls/important_site_checkbox.html
[add] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/resources/settings/controls/important_site_checkbox.js
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/ui/webui/metrics_handler.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/ui/webui/metrics_handler.h
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/common/chrome_features.cc
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/common/chrome_features.h
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/chrome/test/data/webui/settings/privacy_page_test.js
[modify] https://crrev.com/f5de69bdf1d350ce1bc8b4500b542ef3bc1ebe5e/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4870daad28213d17ea5bcea9de843a13f0006d67

commit 4870daad28213d17ea5bcea9de843a13f0006d67
Author: dullweber <dullweber@chromium.org>
Date: Thu May 18 15:06:10 2017

Get the size of local storage and used quota for important sites.

BUG=698692

Review-Url: https://codereview.chromium.org/2752263003
Cr-Commit-Position: refs/heads/master@{#472813}

[modify] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/BUILD.gn
[modify] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/android/browsing_data/browsing_data_bridge.cc
[add] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/engagement/important_sites_usage_counter.cc
[add] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/engagement/important_sites_usage_counter.h
[add] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/engagement/important_sites_usage_counter_unittest.cc
[modify] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/engagement/important_sites_util.cc
[modify] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/engagement/important_sites_util.h
[modify] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
[modify] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h
[modify] https://crrev.com/4870daad28213d17ea5bcea9de843a13f0006d67/chrome/test/BUILD.gn

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86c6630be664b9e45726d93c8fa0017967d56ddf

commit 86c6630be664b9e45726d93c8fa0017967d56ddf
Author: dullweber <dullweber@chromium.org>
Date: Mon May 22 08:54:58 2017

Change checkbox size for ImportantSites dialog

Make checkboxes in ImportantSites consistent with Clear Browsing Data.

https://screenshot.googleplex.com/WBX7VZCsg6s.png

BUG=698692
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2892033003
Cr-Commit-Position: refs/heads/master@{#473528}

[modify] https://crrev.com/86c6630be664b9e45726d93c8fa0017967d56ddf/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
[modify] https://crrev.com/86c6630be664b9e45726d93c8fa0017967d56ddf/chrome/browser/resources/settings/controls/important_site_checkbox.html

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17d5597c638435b1b8fe60fe6e34da8dd7da3daa

commit 17d5597c638435b1b8fe60fe6e34da8dd7da3daa
Author: dullweber <dullweber@chromium.org>
Date: Tue May 23 09:48:22 2017

Only show important sites dialog for cookies

The dialog should only be shown for cookies and not if just a cache
deletion is selected.

BUG=698692
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2897863003
Cr-Commit-Position: refs/heads/master@{#473843}

[modify] https://crrev.com/17d5597c638435b1b8fe60fe6e34da8dd7da3daa/chrome/app/settings_strings.grdp
[modify] https://crrev.com/17d5597c638435b1b8fe60fe6e34da8dd7da3daa/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js

 Issue 672117  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f151da52049d5db6abd58ddc30551bee99f6ec4

commit 2f151da52049d5db6abd58ddc30551bee99f6ec4
Author: dullweber <dullweber@chromium.org>
Date: Mon Jun 12 10:16:37 2017

Add string for cache and cookies to important sites

The important sites dialog on desktop is only triggered when cookies
are selected for deletion but cache is also protected, so the string
needs to be different in case cookies and cache are supposed to be
deleted.

BUG=698692
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2934453003
Cr-Commit-Position: refs/heads/master@{#478579}

[modify] https://crrev.com/2f151da52049d5db6abd58ddc30551bee99f6ec4/chrome/app/settings_strings.grdp
[modify] https://crrev.com/2f151da52049d5db6abd58ddc30551bee99f6ec4/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
[modify] https://crrev.com/2f151da52049d5db6abd58ddc30551bee99f6ec4/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

Added a component so this shows on triage dashboards. Please reclassify if I'm wrong.

dullweber@: It seems like this work was completed or fell off your plate. Can you please provide an update?

We put the project on hold as we (privacy) and the ui team were not happy about the way the feature worked.

When clicking "Clear Data", you can't be sure that the important sites UI will be shown and whether the site you would like to excempt will be available. The heuristics that supress specific sites or whole ui, make the feature very unpredictable. 
We thought about adding a way to excempt sites directly in the CBD UI but didn't reach a good solution.

The code has been removed some time ago to simplify settings maintenance: https://crrev.com/c/1069008

I think this was the proposed future that everyone liked:
https://docs.google.com/document/d/1zn0DQQK1E7OYlBlfEzONawkWWfxh8xw62u9stW2DNC8/edit#

Yes, I think the concept is good but the "mocks" are not quite ready ;)