Add a notice about other forms of history to the CBD dialog

Add a notice about other forms of history to the CBD dialog

We are adding messaging to the Clear Browsing Data dialog informing users that their Chrome browsing history (which can be deleted in the dialog) is not the same as other forms of browsing history (such as Google Search history; which can NOT be deleted in the dialog).

There are two messages:

1. In the footer of the dialog. Since there are currently three pieces of information in the Desktop version, we divided them into two paragraphs and added icons: "i" icon for the general information paragraph, and "G" icon for the paragraph about other forms of browsing history (see mocks).

2. A one-time dialog that pops up after the first deletion of browsing history. This is represented by the class ClearBrowserDataHistoryNotice.

Design doc: https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/
Mocks: https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC1cuEjM/

BUG=595332
Committed: https://crrev.com/4b87c75a9276e4290c073e23d06dcc0cdb3ce589
Cr-Commit-Position: refs/heads/master@{#384891}

[msramek, 2016-03-17 19:24:22]

msramek@chromium.org changed reviewers:
+ estade@chromium.org

[msramek, 2016-03-17 19:24:22]

Hi Evan,

please have a look!

Thanks,
Martin

[Evan Stade, 2016-03-17 21:43:52]

estade@chromium.org changed reviewers:
+ dbeam@chromium.org

[Evan Stade, 2016-03-17 21:43:53]

dbeam is a better review for webui, especially settings (since he's working on
overhauling settings)

[Dan Beam, 2016-03-18 22:48:40]

where is this pref registered?  why is it a number that counts up to 1 instead
of a boolean?

also note: we're rewriting settings right now in the Material Design style and
this code will be dropped when we're done.  if you'd like this change to live
on, this functionality will need to be ported to the Material Design version.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
File
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.html
(right):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.html:9:
i18n-content="ok" onclick="PageManager.closeOverlay();">
don't use on*= attributes in HTML, put this in a separate .js instead (which is
looks like you already have?)

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
File
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.js
(right):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.js:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.js:35:
this.handleClose.bind(this);
nit: why not

$('clear-browser-data-history-notice-ok').onclick = function() {
  PageManager.closeOverlay();
  ClearBrowserDataOverlay.dismiss();
};

?

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.css:56:
padding-left: 33px;
you should try this UI in RTL (you probably need -webkit-padding-start: 33px
instead)

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.css:65:
background-position: left center;
this also needs to be mirrored in RTL, i.e.

html[dir='rtl'] #clear-browser-data-history-footer,
html[dir='rtl'] #clear-browser-data-general-footer {
  background-position: right center;
}

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.css:68: margin: 0 0
0.8em 0;
is this intentionally zeroing top/left/right margins or could this just be
margin-buttom: 0.8em?

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/info.svg (left):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/info.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" width="18px" height="18px" viewBox="0 0 48
48" fill="#000000">
there's some hack that allows you to use -webkit-image-mask(url(info.svg));
background: #757575; or something like that to use the same SVG with a different
fill color

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:322: PrefService*
prefs = profile->GetPrefs();
nit: PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs();

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:327: #endif
I would not commit this

[msramek, 2016-03-21 16:30:25]

Thanks, Dan. PTAL!

The preference is an integer, since the original plan was to show the notice
several times. The current agreement is to show it once, but it's not unlikely
that it changes again, as the entire design is still not finished. I'd like to
be ready for that, i.e. not have to write a migration and preference cleanup,
and I think that using integer instead of boolean is a small price for that.

The preference is now registered in browser_ui_prefs.cc - I forgot to include
that in the previous patchset.

I am aware of the MD redesign, but we want this notice to appear in M51 already
(see the design doc). I'll take care of the MD update in M52.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
File
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.html
(right):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.html:9:
i18n-content="ok" onclick="PageManager.closeOverlay();">
On 2016/03/18 22:48:39, Dan Beam wrote:
> don't use on*= attributes in HTML, put this in a separate .js instead (which
is
> looks like you already have?)

Sorry, this is just a forgotten testing code. Yes, it's already in the JS file.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
File
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.js
(right):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.js:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/03/18 22:48:39, Dan Beam wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_history_notice_overlay.js:35:
this.handleClose.bind(this);
On 2016/03/18 22:48:39, Dan Beam wrote:
> nit: why not
> 
> $('clear-browser-data-history-notice-ok').onclick = function() {
>   PageManager.closeOverlay();
>   ClearBrowserDataOverlay.dismiss();
> };
> 
> ?

Sure :) Done.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.css:56:
padding-left: 33px;
On 2016/03/18 22:48:39, Dan Beam wrote:
> you should try this UI in RTL (you probably need -webkit-padding-start: 33px
> instead)

Thanks for the sanity check, I forgot about that. It's still not clear to me how
to convince my Linux Chromium to use a different display language, but I used
<html dir="rtl"> and style: {direction: 'rtl'} to approximate it. The two
suggestions you made mirror the footer correctly.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.css:65:
background-position: left center;
On 2016/03/18 22:48:39, Dan Beam wrote:
> this also needs to be mirrored in RTL, i.e.
> 
> html[dir='rtl'] #clear-browser-data-history-footer,
> html[dir='rtl'] #clear-browser-data-general-footer {
>   background-position: right center;
> }

Done.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.css:68: margin: 0 0
0.8em 0;
On 2016/03/18 22:48:39, Dan Beam wrote:
> is this intentionally zeroing top/left/right margins or could this just be
> margin-buttom: 0.8em?

It is intentional, same as "margin: 0" in #clear-browser-data-general-footer
below. Just that in this case, we also want a bit of spacing between the two
paragraphs.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/info.svg (left):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/info.svg:1: <svg
xmlns="http://www.w3.org/2000/svg" width="18px" height="18px" viewBox="0 0 48
48" fill="#000000">
On 2016/03/18 22:48:39, Dan Beam wrote:
> there's some hack that allows you to use -webkit-image-mask(url(info.svg));
> background: #757575; or something like that to use the same SVG with a
different
> fill color

Interesting :)

However, the source which diff found is
third_party/WebKit/Source/devtools/front_end/Images/ic_info_black_18dp.svg. I
don't think it's correct to include that from here.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:322: PrefService*
prefs = profile->GetPrefs();
On 2016/03/18 22:48:39, Dan Beam wrote:
> nit: PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs();

Done.

https://codereview.chromium.org/1813023002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:327: #endif
On 2016/03/18 22:48:39, Dan Beam wrote:
> I would not commit this

Hmm, OK. Given that it's just one line of code, I guess I can add it when
needed.

https://codereview.chromium.org/1813023002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

https://codereview.chromium.org/1813023002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/clear_browser_data_overlay.css:69: min-height:
16px;
Since the icons are only in the background, I added min-height to make sure that
the <p> element is always large enough. It shouldn't be necessary, as the
default line height is 21px, but it doesn't hurt either.

[msramek, 2016-03-23 19:01:29]

In CL 1829733002 (not landed yet, but the essence is not going to change), we're
replacing HasOtherFormsOfBrowsingHistory() with an asynchronous call, so I
updated the callsites in Patchset #6.

[msramek, 2016-03-28 10:04:54]

Friendly ping!

[msramek, 2016-03-30 16:56:59]

Patchset #8 (id:140001) has been deleted

[msramek, 2016-03-30 16:58:04]

And another update, since the requirements keep changing :/

- added parameters to the history.google.com links, indicating the source
- the one-time notice now uses different conditions than the permanent notice

[Dan Beam, 2016-03-31 04:47:01]

looks pretty good to me

https://codereview.chromium.org/1813023002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

https://codereview.chromium.org/1813023002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/clear_browser_data_overlay.css:65:
background-position: left center;
what extra space is there? I.e. why do you need the "left" here that has to be
mirrored rather than use a different mechanism?

https://codereview.chromium.org/1813023002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/clear_browser_data_overlay.css:66:
background-repeat: no-repeat;
nit: background shorthand

https://codereview.chromium.org/1813023002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/1813023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:109:
RefreshHistoryNotice();
it would be ideal if this was transmitted at load time in loadTimeData, but
/options/ is more-or-less deprecated so I'll live

https://codereview.chromium.org/1813023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:335: bool
show_notice =
nit: const bool

https://codereview.chromium.org/1813023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:415:
should_show_history_deletion_dialog_ = show;
maybe add a comment as to where this actually takes effect (i.e.
// OnBrowsingDataRemoverDone takes this into account.
or something)

[msramek, 2016-03-31 18:19:57]

Thanks, Dan!

https://codereview.chromium.org/1813023002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/clear_browser_data_overlay.css (right):

https://codereview.chromium.org/1813023002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/clear_browser_data_overlay.css:65:
background-position: left center;
On 2016/03/31 04:47:00, Dan Beam wrote:
> what extra space is there? I.e. why do you need the "left" here that has to be
> mirrored rather than use a different mechanism?

The footer also contains text (starting at -webkit-padding-start: 33px defined
above). The image is positioned inside that padding (i.e. left by default and
right in RTL).

https://codereview.chromium.org/1813023002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/clear_browser_data_overlay.css:66:
background-repeat: no-repeat;
On 2016/03/31 04:47:00, Dan Beam wrote:
> nit: background shorthand

Done.

https://codereview.chromium.org/1813023002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/1813023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:109:
RefreshHistoryNotice();
On 2016/03/31 04:47:01, Dan Beam wrote:
> it would be ideal if this was transmitted at load time in loadTimeData, but
> /options/ is more-or-less deprecated so I'll live

The value is not known at load time, as it is obtained asynchronously. We were
discussing sending the asynchronous ping sooner and caching the value, but we
also want the footer to reflect the current state on the server, hence the
current solution where we refresh at each initialization.

https://codereview.chromium.org/1813023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:335: bool
show_notice =
On 2016/03/31 04:47:00, Dan Beam wrote:
> nit: const bool

Done.

https://codereview.chromium.org/1813023002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:415:
should_show_history_deletion_dialog_ = show;
On 2016/03/31 04:47:00, Dan Beam wrote:
> maybe add a comment as to where this actually takes effect (i.e.
> // OnBrowsingDataRemoverDone takes this into account.
> or something)

Done.

[Dan Beam, 2016-04-01 15:18:53]

lgtm

[msramek, 2016-04-01 19:41:45]

msramek@chromium.org changed reviewers:
+ sky@chromium.org

[msramek, 2016-04-01 19:41:45]

Thanks, Dan!

+Scott, can you please review the remaining tiny change in chrome/browser/ui?
The BUILD file and the pref registration. Thanks!

[sky, 2016-04-01 20:33:25]

LGTM

[msramek, 2016-04-04 11:39:06]

Similarly as in the corresponding CL for history, I updated the strings in the
last patchset to match the latest design (that has changed since I started
writing the CL).

[msramek, 2016-04-04 11:39:24]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-04-04 11:39:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1813023002/#ps220001
(title: "Update strings.")

[commit-bot: I haz the power, 2016-04-04 11:39:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813023002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813023002/220001

[commit-bot: I haz the power, 2016-04-04 13:15:53]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-04-04 13:17:06]

Description was changed from

==========
Add a notice about other forms of history to the CBD dialog

We are adding messaging to the Clear Browsing Data dialog informing users that
their Chrome browsing history (which can be deleted in the dialog) is not the
same as other forms of browsing history (such as Google Search history; which
can NOT be deleted in the dialog).

There are two messages:

1. In the footer of the dialog. Since there are currently three pieces of
information in the Desktop version, we divided them into two paragraphs and
added icons: "i" icon for the general information paragraph, and "G" icon for
the paragraph about other forms of browsing history (see mocks).

2. A one-time dialog that pops up after the first deletion of browsing history.
This is represented by the class ClearBrowserDataHistoryNotice.

Design doc:
https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/
Mocks:
https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC...

BUG=595332
==========

to

==========
Add a notice about other forms of history to the CBD dialog

We are adding messaging to the Clear Browsing Data dialog informing users that
their Chrome browsing history (which can be deleted in the dialog) is not the
same as other forms of browsing history (such as Google Search history; which
can NOT be deleted in the dialog).

There are two messages:

1. In the footer of the dialog. Since there are currently three pieces of
information in the Desktop version, we divided them into two paragraphs and
added icons: "i" icon for the general information paragraph, and "G" icon for
the paragraph about other forms of browsing history (see mocks).

2. A one-time dialog that pops up after the first deletion of browsing history.
This is represented by the class ClearBrowserDataHistoryNotice.

Design doc:
https://docs.google.com/document/d/1ZMDSAd44KmzKhqXPjobZOf9rZezs6VqiBnqpDD0auCU/
Mocks:
https://docs.google.com/presentation/d/1rpR3xB3aYFzXD0U--piuMq3y4XAmoYdawA2HC...

BUG=595332

Committed: https://crrev.com/4b87c75a9276e4290c073e23d06dcc0cdb3ce589
Cr-Commit-Position: refs/heads/master@{#384891}
==========

[commit-bot: I haz the power, 2016-04-04 13:17:08]

Patchset 11 (id:??) landed as
https://crrev.com/4b87c75a9276e4290c073e23d06dcc0cdb3ce589
Cr-Commit-Position: refs/heads/master@{#384891}