Cleanup Tool WebUI: Add HTML content and styling with dummy JS

Cleanup Tool WebUI: Add HTML content and styling with dummy JS

Hardcoded strings instead of messages to avoid translation of prototype
strings.

See https://goto.google.com/fypdm for screenshots. Note that this CL
doesn't include all the content in the screenshots. All slides except
the last are at 150% zoom.

Note that I renamed main.html to cleanup.html to try and to follow the pattern in md_bookmarks/md_downloads/md_extensions.

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

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

[proberge, 2017-03-29 17:20:17]

Description was changed from

==========
Cleanup Tool WebUI: Add HTML content and styling with dummy JS

Hardcoded strings instead of messages to avoid translation of prototype
strings.

See https://goto.google.com/fypdm for screenshots. Note that this CL
doesn't include all the content in the screenshots. All slides except
the last are at 150% zoom.

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

to

==========
Cleanup Tool WebUI: Add HTML content and styling with dummy JS

Hardcoded strings instead of messages to avoid translation of prototype
strings.

See https://goto.google.com/fypdm for screenshots. Note that this CL
doesn't include all the content in the screenshots. All slides except
the last are at 150% zoom.

Note that I renamed main.html to cleanup.html to try and to follow the pattern
in md_bookmarks/md_downloads/md_extensions.

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

[proberge, 2017-03-29 17:21:47]

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

[ftirelo, 2017-03-29 20:59:11]

LGTM with some nits.

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/cleanup_tool/icons.html (right):

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/icons.html:16: <g id="verified-user"><path
d="M12 1L3 5v6c0 5.55 3.84 10.74 9 12 5.16-1.26 9-6.45 9-12V5l-9-4zm-2 16l-4-4
1.41-1.41L10 14.17l6.59-6.59L18 9l-8 8z"></path></g>
Maybe rename this to "clean-user" to be consistent with class "clean-icon"? If
you prefer to use a different name, please rename the class as well for
consistency.

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/cleanup_tool/manager.js (right):

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/manager.js:61: if(hasScanResults &&
isInfected)
Nit: what about

  return hasScanResults && isInfected ? "infected-icon" : "clean-icon";

(or even better, what about an aux function that will be reused by
getStatusIcon_?

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/manager.js:74: if(hasScanResults &&
isInfected)
Same as above.

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/cleanup_tool/toolbar.html (right):

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/toolbar.html:22: 
Delete extra line?

[proberge, 2017-03-29 21:29:36]

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

[proberge, 2017-03-29 21:29:36]

++tommycli for a more thorough WebUI review.

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/cleanup_tool/icons.html (right):

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/icons.html:16: <g id="verified-user"><path
d="M12 1L3 5v6c0 5.55 3.84 10.74 9 12 5.16-1.26 9-6.45 9-12V5l-9-4zm-2 16l-4-4
1.41-1.41L10 14.17l6.59-6.59L18 9l-8 8z"></path></g>
On 2017/03/29 20:59:11, ftirelo wrote:
> Maybe rename this to "clean-user" to be consistent with class "clean-icon"? If
> you prefer to use a different name, please rename the class as well for
> consistency.

The icon is named verified-user in
https://www.webcomponents.org/element/PolymerElements/iron-icons/demo/demo/in....
Keeping the same name is probably desired (but not explicitly stated by
http://dev.chromium.org/developers/updating-webui-for-material-design/using-p...).

Not sure about updating the CSS name. Clean and verified don't mean the same
thing and I'd rather have an inconsistency than a misleading name.

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/cleanup_tool/manager.js (right):

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/manager.js:61: if(hasScanResults &&
isInfected)
On 2017/03/29 20:59:11, ftirelo wrote:
> Nit: what about
> 
>   return hasScanResults && isInfected ? "infected-icon" : "clean-icon";
> 
> (or even better, what about an aux function that will be reused by
> getStatusIcon_?

Done. Simple enough to not need a helper function.

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/manager.js:74: if(hasScanResults &&
isInfected)
On 2017/03/29 20:59:11, ftirelo wrote:
> Same as above.

Done.

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/cleanup_tool/toolbar.html (right):

https://codereview.chromium.org/2784653004/diff/20001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/toolbar.html:22: 
On 2017/03/29 20:59:11, ftirelo wrote:
> Delete extra line?

Done.

[tommycli, 2017-03-29 22:09:06]

Hey,

I sent you comments on the C++ handler part only. I recommend you break the HTML
parts into a separate CL so we can review and commit things in smaller pieces.

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc (right):

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc:25:
web_ui()->CallJavascriptFunctionUnsafe("cleanup.Manager.setDetectionStatus",
So we aren't supposed to add any more CallJavascriptFunctionUnsafe calls.

All you have to do is call AllowJavascript() at the top of the method and use
CallJavascript instead.

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc:25:
web_ui()->CallJavascriptFunctionUnsafe("cleanup.Manager.setDetectionStatus",
For this call-and-response pattern, we use cr.sendWithPromise. See here for a
pattern of usage: https://cs.chromium.org/search/?q=areAllProfilesLocked&type=cs

That way, the caller does not have to be a singleton to receive a response...
and it enhances test-ability.

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h (right):

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h:21: void
HandleRequestLastScanResult(const base::ListValue* args);
This should be private. And ... it probably doesn't need a comment, but it's up
to you.

[proberge, 2017-03-30 17:57:21]

Split most of the HTML/JS into a separate CL.

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc (right):

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc:25:
web_ui()->CallJavascriptFunctionUnsafe("cleanup.Manager.setDetectionStatus",
On 2017/03/29 22:09:06, tommycli wrote:
> So we aren't supposed to add any more CallJavascriptFunctionUnsafe calls.
> 
> All you have to do is call AllowJavascript() at the top of the method and use
> CallJavascript instead.

Done.

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc:25:
web_ui()->CallJavascriptFunctionUnsafe("cleanup.Manager.setDetectionStatus",
On 2017/03/29 22:09:06, tommycli wrote:
> For this call-and-response pattern, we use cr.sendWithPromise. See here for a
> pattern of usage:
https://cs.chromium.org/search/?q=areAllProfilesLocked&type=cs
> 
> That way, the caller does not have to be a singleton to receive a response...
> and it enhances test-ability.

Done.

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h (right):

https://codereview.chromium.org/2784653004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h:21: void
HandleRequestLastScanResult(const base::ListValue* args);
On 2017/03/29 22:09:06, tommycli wrote:
> This should be private. And ... it probably doesn't need a comment, but it's
up
> to you.

Done.

[tommycli, 2017-03-30 20:11:40]

lgtm

[proberge, 2017-03-30 20:13:35]

The CQ bit was checked by proberge@chromium.org

[proberge, 2017-03-30 20:13:35]

The patchset sent to the CQ was uploaded after l-g-t-m from ftirelo@chromium.org
Link to the patchset: https://codereview.chromium.org/2784653004/#ps100001
(title: "Fix broken i18n commands")

[commit-bot: I haz the power, 2017-03-30 20:14:35]

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-03-30 20:25:50]

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

[commit-bot: I haz the power, 2017-03-30 20:25:51]

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

[proberge, 2017-03-30 20:33:40]

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

[proberge, 2017-03-30 20:34:06]

++dbeam for browser_resources.grd and compiled_resources2.gyp

[Dan Beam, 2017-04-04 00:00:01]

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/cleanup.html (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:9: <script
src="chrome://resources/js/load_time_data.js"></script>
can you import load_time_data.html instead?

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:10: <script
src="strings.js"></script>
can you put these scripts at the bottom of the page?

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:36: <script
src="chrome://resources/js/i18n_template.js"></script>
indent off

why are you using i18n_template.js?

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/toolbar.html (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/toolbar.html:17: font-size: 16px;
can you use %, em, or rem instead of px for font-size?  this would allow the
page to respect the user's default font sizes from settings

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/toolbar.html:19: padding-left: 18px;
-left doesn't work in RTL, can you use -webkit-padding-start?

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/toolbar.html:23: <div id="page-title"
i18n-content="title"></div>
can you just use <div id="page-title">$i18n{title}</div> instead of
i18n-content?  this would allow you to remove the i18n_template.js

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc:10: #include
"ui/base/l10n/l10n_util.h"
why are these bottom 2 #includes needed?

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc:25: bool success
= args->GetString(0, &webui_callback_id);
can you just get this as a base::Value instead of as a std::string?

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h:15:
CleanupActionHandler();
maybe create a virtual destructor as well?

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cleanup_tool/cleanup_tool_ui.cc (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cleanup_tool/cleanup_tool_ui.cc:24:
html_source->AddString("cleanAction", "RUN CHROME CLEANUP");
i know these are dummies, but in general the strings always stay normal case
(sentence or title), and we use text-transform: uppercase; in CSS to give them
the "SCREAMING AT YOU" appearance (which we might eventually back away from in
the future)

[proberge, 2017-04-04 16:10:11]

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/cleanup.html (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:9: <script
src="chrome://resources/js/load_time_data.js"></script>
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> can you import load_time_data.html instead?

Done.

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:10: <script
src="strings.js"></script>
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> can you put these scripts at the bottom of the page?

Done.

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:36: <script
src="chrome://resources/js/i18n_template.js"></script>
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> indent off
> 
> why are you using i18n_template.js?

It was needed when I was using i18n-content in toolbar.html. Removed it.

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/toolbar.html (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/toolbar.html:17: font-size: 16px;
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> can you use %, em, or rem instead of px for font-size?  this would allow the
> page to respect the user's default font sizes from settings

Done.

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/toolbar.html:19: padding-left: 18px;
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> -left doesn't work in RTL, can you use -webkit-padding-start?

Good catch, thanks!

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/toolbar.html:23: <div id="page-title"
i18n-content="title"></div>
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> can you just use <div id="page-title">$i18n{title}</div> instead of
> i18n-content?  this would allow you to remove the i18n_template.js

Done.

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc:10: #include
"ui/base/l10n/l10n_util.h"
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> why are these bottom 2 #includes needed?

They were used before I split this CL. Removed for now.

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc:25: bool success
= args->GetString(0, &webui_callback_id);
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> can you just get this as a base::Value instead of as a std::string?

I copied this pattern from
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/user_mana...
and others.

Keeping as is to stay consistent, we can clean all of them up in a later CL.

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h:15:
CleanupActionHandler();
On 2017/04/04 00:00:00, Dan Beam (slow) wrote:
> maybe create a virtual destructor as well?

Done.

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/cleanup_tool/cleanup_tool_ui.cc (right):

https://codereview.chromium.org/2784653004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/cleanup_tool/cleanup_tool_ui.cc:24:
html_source->AddString("cleanAction", "RUN CHROME CLEANUP");
On 2017/04/04 00:00:01, Dan Beam (slow) wrote:
> i know these are dummies, but in general the strings always stay normal case
> (sentence or title), and we use text-transform: uppercase; in CSS to give them
> the "SCREAMING AT YOU" appearance (which we might eventually back away from in
> the future)

Done.

[Dan Beam, 2017-04-04 17:40:57]

lgtm

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/browser...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/browser...
chrome/browser/browser_resources.grd:18:
file="resources\cleanup_tool\cleanup.html" type="chrome_html" />
why are you wrapping like this?  can you just follow what the rest of the file
does and not wrap?

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/cleanup.html (right):

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:19: :root {
fwiw: :root and html are probably equivalent here

[proberge, 2017-04-04 19:08:33]

Thanks for the review!

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/browser...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/browser...
chrome/browser/browser_resources.grd:18:
file="resources\cleanup_tool\cleanup.html" type="chrome_html" />
On 2017/04/04 17:40:57, Dan Beam (slow) wrote:
> why are you wrapping like this?  can you just follow what the rest of the file
> does and not wrap?

Done.

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/cleanup.html (right):

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:19: :root {
On 2017/04/04 17:40:57, Dan Beam (slow) wrote:
> fwiw: :root and html are probably equivalent here

There's no uses of --my_css_rule: foo within html or body style tags, and a few
uses within root:

https://cs.chromium.org/search/?q=f:resources/+md_+%5C-%5C-+html%5Cs%7B&type=cs
https://cs.chromium.org/search/?q=f:resources/+md_+%5C-%5C-+root%5Cs%7B&type=cs

Leaving as-is for now.

[proberge, 2017-04-04 19:08:38]

The CQ bit was checked by proberge@chromium.org

[proberge, 2017-04-04 19:08:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
ftirelo@chromium.org, tommycli@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2784653004/#ps160001
(title: "Remove line breaks in browser_resources.grd")

[commit-bot: I haz the power, 2017-04-04 19:09:53]

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-04-04 19:35:41]

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

[commit-bot: I haz the power, 2017-04-04 19:35:43]

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

[Dan Beam, 2017-04-04 19:53:38]

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/cleanup.html (right):

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:19: :root {
On 2017/04/04 19:08:33, proberge wrote:
> On 2017/04/04 17:40:57, Dan Beam (slow) wrote:
> > fwiw: :root and html are probably equivalent here
> 
> There's no uses of --my_css_rule: foo within html or body style tags, and a
few
> uses within root:
> 
>
https://cs.chromium.org/search/?q=f:resources/+md_+%5C-%5C-+html%5Cs%7B&type=cs
>
https://cs.chromium.org/search/?q=f:resources/+md_+%5C-%5C-+root%5Cs%7B&type=cs
> 
> Leaving as-is for now.
> 

https://developer.mozilla.org/en-US/docs/Web/CSS/:root

[proberge, 2017-04-04 20:48:16]

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/cleanup.html (right):

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:19: :root {
On 2017/04/04 19:53:37, Dan Beam (slow) wrote:
> On 2017/04/04 19:08:33, proberge wrote:
> > On 2017/04/04 17:40:57, Dan Beam (slow) wrote:
> > > fwiw: :root and html are probably equivalent here
> > 
> > There's no uses of --my_css_rule: foo within html or body style tags, and a
> few
> > uses within root:
> > 
> >
>
https://cs.chromium.org/search/?q=f:resources/+md_+%5C-%5C-+html%5Cs%7B&type=cs
> >
>
https://cs.chromium.org/search/?q=f:resources/+md_+%5C-%5C-+root%5Cs%7B&type=cs
> > 
> > Leaving as-is for now.
> > 
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/:root

I'm sorry, my comment was a bit unclear. I wasn't implying that you were wrong.
The two are indeed equivalent. 
Since the two are equivalent, I'm leaning towards using the option which is
consistent with other WebUIs. Please let me know if you would prefer to have the
rules inside the html block.

[proberge, 2017-04-04 20:48:23]

The CQ bit was checked by proberge@chromium.org

[proberge, 2017-04-04 20:48:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
ftirelo@chromium.org, dbeam@chromium.org, tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2784653004/#ps180001
(title: "Move build include to is_win section")

[commit-bot: I haz the power, 2017-04-04 20:49:05]

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

[Dan Beam, 2017-04-04 21:07:14]

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/cleanup_tool/cleanup.html (right):

https://codereview.chromium.org/2784653004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/cleanup_tool/cleanup.html:19: :root {
On 2017/04/04 20:48:16, proberge wrote:
> On 2017/04/04 19:53:37, Dan Beam (slow) wrote:
> > On 2017/04/04 19:08:33, proberge wrote:
> > > On 2017/04/04 17:40:57, Dan Beam (slow) wrote:
> > > > fwiw: :root and html are probably equivalent here
> > > 
> > > There's no uses of --my_css_rule: foo within html or body style tags, and
a
> > few
> > > uses within root:
> > > 
> > >
> >
>
https://cs.chromium.org/search/?q=f:resources/+md_+%5C-%5C-+html%5Cs%7B&type=cs
> > >
> >
>
https://cs.chromium.org/search/?q=f:resources/+md_+%5C-%5C-+root%5Cs%7B&type=cs
> > > 
> > > Leaving as-is for now.
> > > 
> > 
> > https://developer.mozilla.org/en-US/docs/Web/CSS/:root
> 
> I'm sorry, my comment was a bit unclear. I wasn't implying that you were
wrong.
> The two are indeed equivalent. 
> Since the two are equivalent, I'm leaning towards using the option which is
> consistent with other WebUIs. Please let me know if you would prefer to have
the
> rules inside the html block.

you obviously didn't want to do this:

:root {
  --cleanup-card-margin: 25px;
}

:root {
  --cleanup-card-width: 800px;
}

and neither do "other WebUIs".  this is basically what your code is equivalent
to in my head right now: just extra, unnecessary selectors.

if you see other stuff using both :root {} and html {} from a top-level context
(like this), we should fix those, too

[commit-bot: I haz the power, 2017-04-04 22:15:48]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491338903504550,
"parent_rev": "efc241285631bb8f93f709b8cf35c3eee7511ad5", "commit_rev":
"e51fced5944c4d9c29f1fe70c1f31ebb7690d6c3"}

[commit-bot: I haz the power, 2017-04-04 22:16:38]

Description was changed from

==========
Cleanup Tool WebUI: Add HTML content and styling with dummy JS

Hardcoded strings instead of messages to avoid translation of prototype
strings.

See https://goto.google.com/fypdm for screenshots. Note that this CL
doesn't include all the content in the screenshots. All slides except
the last are at 150% zoom.

Note that I renamed main.html to cleanup.html to try and to follow the pattern
in md_bookmarks/md_downloads/md_extensions.

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

to

==========
Cleanup Tool WebUI: Add HTML content and styling with dummy JS

Hardcoded strings instead of messages to avoid translation of prototype
strings.

See https://goto.google.com/fypdm for screenshots. Note that this CL
doesn't include all the content in the screenshots. All slides except
the last are at 150% zoom.

Note that I renamed main.html to cleanup.html to try and to follow the pattern
in md_bookmarks/md_downloads/md_extensions.

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

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

[commit-bot: I haz the power, 2017-04-04 22:16:40]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e51fced5944c4d9c29f1fe70c1f3...