Cleanup Tool WebUI: Add logs upload checkbox and minor polishing

Cleanup Tool WebUI: Add logs upload checkbox and minor polishing

As a follow-up to https://codereview.chromium.org/2966453002/, where we
added a logs upload permission checkbox to the modal dialog, this change
adds the same checkbox to the WebUI. The checkbox should be synchronized
with the dialog and other opened settings pages.

Also adds a "Learn More" link to Chrome Cleanup's help center article,
minor fixes for RTL languages, repositions the "Show files to be removed"
button and uses standard colors.

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

Review-Url: https://codereview.chromium.org/2971823002
Cr-Commit-Position: refs/heads/master@{#484917}
Committed: https://chromium.googlesource.com/chromium/src/+/b9d3533f6baa7f4958089d651a2f927e4acc1c7d

[proberge, 2017-07-05 15:28:03]

Description was changed from

==========
Cleanup Tool WebUI: Add logs upload checkbox and minor polishing

As a follow-up to https://codereview.chromium.org/2966453002/, where we
added a logs upload permission checkbox to the modal dialog, this change
adds the same checkbox to the WebUI. The checkbox should be synchronized
with the dialog and other opened settings pages.

Also adds a "Learn More" link to Chrome Cleanup's help center article,
minor fixes for RTL languages, repositions the "Show files to be removed"
button and uses standard colors.

BUG=736465
==========

to

==========
Cleanup Tool WebUI: Add logs upload checkbox and minor polishing

As a follow-up to https://codereview.chromium.org/2966453002/, where we
added a logs upload permission checkbox to the modal dialog, this change
adds the same checkbox to the WebUI. The checkbox should be synchronized
with the dialog and other opened settings pages.

Also adds a "Learn More" link to Chrome Cleanup's help center article,
minor fixes for RTL languages, repositions the "Show files to be removed"
button and uses standard colors.

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

[proberge, 2017-07-05 15:37:10]

proberge@chromium.org changed reviewers:
+ ftirelo@chromium.org

[ftirelo, 2017-07-05 19:02:26]

Thanks a lot for all the minor fixes as well! I only have a question and a few
nits. Other than that, this looks fine.

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

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.html:29:
-webkit-margin-start: calc(var(--settings-button-edge-spacing) * -1);
Please describe why you need this incantation.

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.html:86:
id="logsUploadControl"
Nit: chromeCleanupLogsUploadControl just to be uniform with the label variable
chromeCleanupLogsUploadPermission.

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.js
(right):

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.js:102:
return /** @type {chrome.settingsPrivate.PrefObject} */ ({});
Are () required here?

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_proxy.js
(right):

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_proxy.js:18:
startCleanup(logs_upload_enabled) {}
Nit: shouldn't this be logsUploadEnabled ?

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

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:163: CHECK_EQ(1U,
args->GetSize());
CHECK_EQ -> DCHECK_EQ

We use DCHECK in other arg list validations.

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:171: CHECK_EQ(1U,
args->GetSize());
Ditto.

https://codereview.chromium.org/2971823002/diff/20001/chrome/common/url_const...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/2971823002/diff/20001/chrome/common/url_const...
chrome/common/url_constants.cc:639:
"https://support.google.com/chrome/answer/6086368";
Shouldn't we show the text in the user's current language?

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

https://codereview.chromium.org/2971823002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/chrome_cleanup_page_test.js:86:
function(logs_upload_enabled) {
logsUploadEnabled?

[proberge, 2017-07-05 20:14:19]

proberge@chromium.org changed reviewers:
+ tommycli@chromium.org

[proberge, 2017-07-05 20:14:21]

@ftirelo thanks for the quick review
@tommycli PTAL. I could also use some help figuring out why the test's
MockInteraction (see chrome_cleanup_page_test:152) is not working properly.

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

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.html:29:
-webkit-margin-start: calc(var(--settings-button-edge-spacing) * -1);
On 2017/07/05 19:02:25, ftirelo wrote:
> Please describe why you need this incantation.

Done.

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.html:86:
id="logsUploadControl"
On 2017/07/05 19:02:25, ftirelo wrote:
> Nit: chromeCleanupLogsUploadControl just to be uniform with the label variable
> chromeCleanupLogsUploadPermission.

Done.

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.js
(right):

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.js:102:
return /** @type {chrome.settingsPrivate.PrefObject} */ ({});
On 2017/07/05 19:02:25, ftirelo wrote:
> Are () required here?

Maybe not, but all examples I could find do:
https://cs.chromium.org/search/?q=return.*chrome.settingsPrivate.PrefObject+l...

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_proxy.js
(right):

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_proxy.js:18:
startCleanup(logs_upload_enabled) {}
On 2017/07/05 19:02:25, ftirelo wrote:
> Nit: shouldn't this be logsUploadEnabled ?

Done.

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

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:163: CHECK_EQ(1U,
args->GetSize());
On 2017/07/05 19:02:25, ftirelo wrote:
> CHECK_EQ -> DCHECK_EQ
> 
> We use DCHECK in other arg list validations.

Here the param is actually strictly needed.

https://codereview.chromium.org/2971823002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:171: CHECK_EQ(1U,
args->GetSize());
On 2017/07/05 19:02:25, ftirelo wrote:
> Ditto.

Ditto^2

https://codereview.chromium.org/2971823002/diff/20001/chrome/common/url_const...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/2971823002/diff/20001/chrome/common/url_const...
chrome/common/url_constants.cc:639:
"https://support.google.com/chrome/answer/6086368";
On 2017/07/05 19:02:25, ftirelo wrote:
> Shouldn't we show the text in the user's current language?

Added some logic to md_settings_localized_strings_provider to do so.

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

https://codereview.chromium.org/2971823002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/chrome_cleanup_page_test.js:86:
function(logs_upload_enabled) {
On 2017/07/05 19:02:26, ftirelo wrote:
> logsUploadEnabled?

Done.

[proberge, 2017-07-05 20:16:29]

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/chrome_cleanup_page_test.js (right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/chrome_cleanup_page_test.js:152:
MockInteractions.tap(control);
Does not work here, but works for a similar case:
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/privacy_...

[ftirelo, 2017-07-05 20:24:50]

LGTM with a small nit.

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:805:
std::string cleanup_learn_more_url =
Nitty nit: this can be const.

[tommycli, 2017-07-06 19:00:14]

lgtm except below

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.html
(right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.html:17:
#about-link {
I don't see an about-link id element in here.

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/chrome_cleanup_page/compiled_resources2.gyp
(right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/compiled_resources2.gyp:23:
'<(EXTERNS_GYP):settings_private',
should go below <(DEPTH) entries.

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/chrome_cleanup_page_test.js (right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/chrome_cleanup_page_test.js:152:
MockInteractions.tap(control);
On 2017/07/05 20:16:29, proberge wrote:
> Does not work here, but works for a similar case:
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/privacy_...

Hmm nothing immediately stands out to me as wrong here...

Did you try putting a dom flush above or below this call?

[proberge, 2017-07-06 21:12:56]

Thanks for the quick review!

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.html
(right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/chrome_cleanup_page.html:17:
#about-link {
On 2017/07/06 19:00:13, tommycli wrote:
> I don't see an about-link id element in here.

Good catch! Removed it.

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/chrome_cleanup_page/compiled_resources2.gyp
(right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/chrome_cleanup_page/compiled_resources2.gyp:23:
'<(EXTERNS_GYP):settings_private',
On 2017/07/06 19:00:13, tommycli wrote:
> should go below <(DEPTH) entries.

Done.

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:805:
std::string cleanup_learn_more_url =
On 2017/07/05 20:24:50, ftirelo wrote:
> Nitty nit: this can be const.

Done.

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/chrome_cleanup_page_test.js (right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/chrome_cleanup_page_test.js:152:
MockInteractions.tap(control);
On 2017/07/06 19:00:13, tommycli wrote:
> On 2017/07/05 20:16:29, proberge wrote:
> > Does not work here, but works for a similar case:
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/privacy_...
> 
> Hmm nothing immediately stands out to me as wrong here...
> 
> Did you try putting a dom flush above or below this call?

That didn't work either. Going to leave a TODO for now.

[proberge, 2017-07-06 21:13:17]

The CQ bit was checked by proberge@chromium.org

[proberge, 2017-07-06 21:13:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
ftirelo@chromium.org, tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2971823002/#ps60001
(title: "Address review comments on #3, simplify idle error cases")

[commit-bot: I haz the power, 2017-07-06 21:13:50]

CQ is trying da patch.

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

[tommycli, 2017-07-06 21:23:12]

no problem! One more comment below

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/chrome_cleanup_page_test.js (right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/chrome_cleanup_page_test.js:152:
MockInteractions.tap(control);
On 2017/07/06 21:12:56, proberge wrote:
> On 2017/07/06 19:00:13, tommycli wrote:
> > On 2017/07/05 20:16:29, proberge wrote:
> > > Does not work here, but works for a similar case:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/privacy_...
> > 
> > Hmm nothing immediately stands out to me as wrong here...
> > 
> > Did you try putting a dom flush above or below this call?
> 
> That didn't work either. Going to leave a TODO for now.

Err hmm I'm not super satisfied that we're committing something with unexplained
behavior... but I guess it's okay if you're going to follow up in the very next
CL...

[proberge, 2017-07-06 21:25:52]

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/chrome_cleanup_page_test.js (right):

https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/chrome_cleanup_page_test.js:152:
MockInteractions.tap(control);
On 2017/07/06 21:23:12, tommycli wrote:
> On 2017/07/06 21:12:56, proberge wrote:
> > On 2017/07/06 19:00:13, tommycli wrote:
> > > On 2017/07/05 20:16:29, proberge wrote:
> > > > Does not work here, but works for a similar case:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/privacy_...
> > > 
> > > Hmm nothing immediately stands out to me as wrong here...
> > > 
> > > Did you try putting a dom flush above or below this call?
> > 
> > That didn't work either. Going to leave a TODO for now.
> 
> Err hmm I'm not super satisfied that we're committing something with
unexplained
> behavior... but I guess it's okay if you're going to follow up in the very
next
> CL...

Yeah, it's not great :(

I spent several hours yesterday trying to figure out why it wasn't working. I
can't afford to spend any more time on it at the moment and will follow-up in a
week or two when I have more time.

[commit-bot: I haz the power, 2017-07-06 21:55:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 21:55:49]

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_...)

[tommycli, 2017-07-06 23:57:36]

On 2017/07/06 21:25:52, proberge wrote:
>
https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/chrome_cleanup_page_test.js (right):
> 
>
https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/chrome_cleanup_page_test.js:152:
> MockInteractions.tap(control);
> On 2017/07/06 21:23:12, tommycli wrote:
> > On 2017/07/06 21:12:56, proberge wrote:
> > > On 2017/07/06 19:00:13, tommycli wrote:
> > > > On 2017/07/05 20:16:29, proberge wrote:
> > > > > Does not work here, but works for a similar case:
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/privacy_...
> > > > 
> > > > Hmm nothing immediately stands out to me as wrong here...
> > > > 
> > > > Did you try putting a dom flush above or below this call?
> > > 
> > > That didn't work either. Going to leave a TODO for now.
> > 
> > Err hmm I'm not super satisfied that we're committing something with
> unexplained
> > behavior... but I guess it's okay if you're going to follow up in the very
> next
> > CL...
> 
> Yeah, it's not great :(
> 
> I spent several hours yesterday trying to figure out why it wasn't working. I
> can't afford to spend any more time on it at the moment and will follow-up in
a
> week or two when I have more time.

Hmm... is this code going to go in front of users? Or is it gated behind a
feature flag?

[proberge, 2017-07-07 00:34:09]

On 2017/07/06 23:57:36, tommycli wrote:
> On 2017/07/06 21:25:52, proberge wrote:
> >
>
https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
> > File chrome/test/data/webui/settings/chrome_cleanup_page_test.js (right):
> > 
> >
>
https://codereview.chromium.org/2971823002/diff/40001/chrome/test/data/webui/...
> > chrome/test/data/webui/settings/chrome_cleanup_page_test.js:152:
> > MockInteractions.tap(control);
> > On 2017/07/06 21:23:12, tommycli wrote:
> > > On 2017/07/06 21:12:56, proberge wrote:
> > > > On 2017/07/06 19:00:13, tommycli wrote:
> > > > > On 2017/07/05 20:16:29, proberge wrote:
> > > > > > Does not work here, but works for a similar case:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/privacy_...
> > > > > 
> > > > > Hmm nothing immediately stands out to me as wrong here...
> > > > > 
> > > > > Did you try putting a dom flush above or below this call?
> > > > 
> > > > That didn't work either. Going to leave a TODO for now.
> > > 
> > > Err hmm I'm not super satisfied that we're committing something with
> > unexplained
> > > behavior... but I guess it's okay if you're going to follow up in the very
> > next
> > > CL...
> > 
> > Yeah, it's not great :(
> > 
> > I spent several hours yesterday trying to figure out why it wasn't working.
I
> > can't afford to spend any more time on it at the moment and will follow-up
in
> a
> > week or two when I have more time.
> 
> Hmm... is this code going to go in front of users? Or is it gated behind a
> feature flag?

It's gated behind InBrowserCleanerUI

[proberge, 2017-07-07 00:34:17]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2017-07-07 00:34:34]

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, 2017-07-07 02:03:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 02:03:02]

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_...)

[proberge, 2017-07-07 13:13:22]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2017-07-07 13:13:32]

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, 2017-07-07 14:09:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499433202769130,
"parent_rev": "364b6e997ac9d0181be7ee5e313abc04fe7e20de", "commit_rev":
"b9d3533f6baa7f4958089d651a2f927e4acc1c7d"}

[commit-bot: I haz the power, 2017-07-07 14:09:27]

Description was changed from

==========
Cleanup Tool WebUI: Add logs upload checkbox and minor polishing

As a follow-up to https://codereview.chromium.org/2966453002/, where we
added a logs upload permission checkbox to the modal dialog, this change
adds the same checkbox to the WebUI. The checkbox should be synchronized
with the dialog and other opened settings pages.

Also adds a "Learn More" link to Chrome Cleanup's help center article,
minor fixes for RTL languages, repositions the "Show files to be removed"
button and uses standard colors.

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

to

==========
Cleanup Tool WebUI: Add logs upload checkbox and minor polishing

As a follow-up to https://codereview.chromium.org/2966453002/, where we
added a logs upload permission checkbox to the modal dialog, this change
adds the same checkbox to the WebUI. The checkbox should be synchronized
with the dialog and other opened settings pages.

Also adds a "Learn More" link to Chrome Cleanup's help center article,
minor fixes for RTL languages, repositions the "Show files to be removed"
button and uses standard colors.

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

Review-Url: https://codereview.chromium.org/2971823002
Cr-Commit-Position: refs/heads/master@{#484917}
Committed:
https://chromium.googlesource.com/chromium/src/+/b9d3533f6baa7f4958089d651a2f...
==========

[commit-bot: I haz the power, 2017-07-07 14:09:29]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b9d3533f6baa7f4958089d651a2f...