Migrate the one-time notice into the MD settings CBD dialog

Migrate the one-time notice into the MD settings CBD dialog

Migrate the one-time notice about other forms of browsing history
(introduced in https://codereview.chromium.org/1813023002/) to the
MD settings.

The C++ side is migrated more or less verbatim from ClearBrowserDataHandler.
The JS side adds a new 'settings-other-forms-of-browsing-history-notice'.

We add a test to verify that the one-time notice is shown, and that closing
it will close its parent CBD dialog.

BUG=595580
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f310cbf5b0e999df047e77e9834b46dd396987c7
Cr-Commit-Position: refs/heads/master@{#403152}

[msramek, 2016-06-23 19:33:13]

Description was changed from

==========
Migrate the one-time notice into the MD settings CBD dialog

Migrate the one-time notice about other forms of browsing history
(introduced in https://codereview.chromium.org/1813023002/) to the
MD settings.

The C++ side is migrated more or less verbatim from ClearBrowserDataHandler.
The JS side adds a new 'settings-other-forms-of-browsing-history-notice'.

We add a test to verify that the one-time notice is shown, and that closing
it will close its parent CBD dialog.

BUG=595580
==========

to

==========
Migrate the one-time notice into the MD settings CBD dialog

Migrate the one-time notice about other forms of browsing history
(introduced in https://codereview.chromium.org/1813023002/) to the
MD settings.

The C++ side is migrated more or less verbatim from ClearBrowserDataHandler.
The JS side adds a new 'settings-other-forms-of-browsing-history-notice'.

We add a test to verify that the one-time notice is shown, and that closing
it will close its parent CBD dialog.

BUG=595580
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[msramek, 2016-06-23 19:38:46]

Patchset #1 (id:1) has been deleted

[msramek, 2016-06-23 19:41:24]

msramek@chromium.org changed reviewers:
+ dbeam@chromium.org, dschuyler@chromium.org

[msramek, 2016-06-23 19:41:24]

Hello again guys,

this is another CL about the CBD dialog. Please have a look!

Thanks,
Martin

[dschuyler, 2016-06-25 01:34:54]

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
(right):

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:93:
with-backdrop="[[!showOtherFormsOfBrowsingHistoryNotice_]]">
The length of the names, such as 
showOtherFormsOfBrowsingHistoryNotice_
is more to mentally parse than I think
is comfortable. Is there some way to 
shorten the long-ish names used in this
CL (not just this one place)?

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:189:
notice_shown_times < kMaxTimesHistoryNoticeShown;
nit: not a big deal, but if #2 and #3 were swapped,
we may avoid calling prefs-GetBoolean when
notice_shown_times < kMaxTimesHistoryNoticeShown
is false.

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:194:
notice_shown_times + 1);
trivia: it looks like there is a potential race
condition here. It also looks like the effect of
that race are harmless. This note is just to ask
that it be given consideration (whether the race
is an issue). If (as I expect) it's harmless, then
an 'ack' on this note is plenty.

https://codereview.chromium.org/2098633002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2098633002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/privacy_page_test.js:169: // the dom-if template
is notified and actually creates it.
Can a Polymer.dom.flush() be used rather than
spinning the update loop?

[msramek, 2016-06-27 15:24:59]

I rebased and addressed your comments. PTAL!

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
(right):

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:93:
with-backdrop="[[!showOtherFormsOfBrowsingHistoryNotice_]]">
On 2016/06/25 01:34:54, dschuyler wrote:
> The length of the names, such as 
> showOtherFormsOfBrowsingHistoryNotice_
> is more to mentally parse than I think
> is comfortable. Is there some way to 
> shorten the long-ish names used in this
> CL (not just this one place)?

Done.

I have been struggling with this since the very beginning of the project.

1. "history notice(s)" kinda implies it's about the browsing history checkbox.

2. "footer notice" and "dialog notice" both collide for different reasons. There
is other information in the footer. This is a dialog in a dialog, so which one
does "dialog" refer to?

Let me call it "history deletion dialog", since it's a dialog that pops up after
history deletion. That's what I used on the C++ side, so it's at least going to
be consistent. I'll rename the new element that way as well.

I also shortened a few other things, where possible...

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:189:
notice_shown_times < kMaxTimesHistoryNoticeShown;
On 2016/06/25 01:34:54, dschuyler wrote:
> nit: not a big deal, but if #2 and #3 were swapped,
> we may avoid calling prefs-GetBoolean when
> notice_shown_times < kMaxTimesHistoryNoticeShown
> is false.

Done.

https://codereview.chromium.org/2098633002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:194:
notice_shown_times + 1);
On 2016/06/25 01:34:54, dschuyler wrote:
> trivia: it looks like there is a potential race
> condition here. It also looks like the effect of
> that race are harmless. This note is just to ask
> that it be given consideration (whether the race
> is an issue). If (as I expect) it's harmless, then
> an 'ack' on this note is plenty.

Acknowledged. Thanks for pointing it out.

https://codereview.chromium.org/2098633002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2098633002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/privacy_page_test.js:169: // the dom-if template
is notified and actually creates it.
On 2016/06/25 01:34:54, dschuyler wrote:
> Can a Polymer.dom.flush() be used rather than
> spinning the update loop?

Yes! Thanks :)

[Dan Beam, 2016-06-28 02:35:46]

https://codereview.chromium.org/2098633002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
(right):

https://codereview.chromium.org/2098633002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:172:
on-closed="onHistoryDeletionDialogClose_">
can we use on-iron-overlay-closed instead of on-closed so we don't add another
custom event?

https://codereview.chromium.org/2098633002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/history_deletion_dialog.html
(right):

https://codereview.chromium.org/2098633002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/history_deletion_dialog.html:3:
<link rel="import" href="/i18n_setup.html">
i don't think you need i18n_setup.html, do you?

[msramek, 2016-06-28 19:23:43]

https://codereview.chromium.org/2098633002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
(right):

https://codereview.chromium.org/2098633002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:172:
on-closed="onHistoryDeletionDialogClose_">
On 2016/06/28 02:35:46, Dan Beam wrote:
> can we use on-iron-overlay-closed instead of on-closed so we don't add another
> custom event?

Done.

This seems to work fine when I run the browser (the event bubbles up from this
cr-dialog through settings-history-deletion-dialog to
settings-clear-browsing-data-dialog).

However, in the browsertest, it is consumed by settings-history-deletion-dialog.
Unless I catch it and manually fire fire('iron-overlay-closed'),
settings-clear-browsing-data-dialog does not learn of it. Let me investigate...

https://codereview.chromium.org/2098633002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/history_deletion_dialog.html
(right):

https://codereview.chromium.org/2098633002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/history_deletion_dialog.html:3:
<link rel="import" href="/i18n_setup.html">
On 2016/06/28 02:35:46, Dan Beam wrote:
> i don't think you need i18n_setup.html, do you?

Done. Nope.

[Dan Beam, 2016-06-29 00:38:55]

lgtm if you fix the tests

[msramek, 2016-06-29 18:59:37]

I fixed the test - the event was actually fired and propagated properly, but
after it finished, I had to yield to the message loop once again.

I originally made the wrong assumption that chaining promises would allow me to
keep jumping to the end of the message loop, but this is apparently not the
case. The message loop is spinned, but I'm not on the end when it's the next
Promise's turn. Only "setTimeout(fnc, 0)" achieves that.

Dave - do you have additional comments?

[dschuyler, 2016-06-30 01:34:16]

LGTM with one small request.

https://codereview.chromium.org/2098633002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js
(right):

https://codereview.chromium.org/2098633002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:56:
showHistoryDeletionDialog_: Boolean,
Please give this an explicit default value.

[msramek, 2016-06-30 11:05:36]

Thanks guys!

https://codereview.chromium.org/2098633002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js
(right):

https://codereview.chromium.org/2098633002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js:56:
showHistoryDeletionDialog_: Boolean,
On 2016/06/30 01:34:15, dschuyler wrote:
> Please give this an explicit default value.

Done.

[msramek, 2016-06-30 11:05:52]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-06-30 11:05:52]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2098633002/#ps120001
(title: "Initialize to false.")

[commit-bot: I haz the power, 2016-06-30 11:06:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 12:27:21]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-06-30 12:27:24]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 12:28:56]

Description was changed from

==========
Migrate the one-time notice into the MD settings CBD dialog

Migrate the one-time notice about other forms of browsing history
(introduced in https://codereview.chromium.org/1813023002/) to the
MD settings.

The C++ side is migrated more or less verbatim from ClearBrowserDataHandler.
The JS side adds a new 'settings-other-forms-of-browsing-history-notice'.

We add a test to verify that the one-time notice is shown, and that closing
it will close its parent CBD dialog.

BUG=595580
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Migrate the one-time notice into the MD settings CBD dialog

Migrate the one-time notice about other forms of browsing history
(introduced in https://codereview.chromium.org/1813023002/) to the
MD settings.

The C++ side is migrated more or less verbatim from ClearBrowserDataHandler.
The JS side adds a new 'settings-other-forms-of-browsing-history-notice'.

We add a test to verify that the one-time notice is shown, and that closing
it will close its parent CBD dialog.

BUG=595580
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f310cbf5b0e999df047e77e9834b46dd396987c7
Cr-Commit-Position: refs/heads/master@{#403152}
==========

[commit-bot: I haz the power, 2016-06-30 12:28:57]

Patchset 6 (id:??) landed as
https://crrev.com/f310cbf5b0e999df047e77e9834b46dd396987c7
Cr-Commit-Position: refs/heads/master@{#403152}