Cleanup Tool WebUI: Add functional HTML and skeleton JS for main content

Cleanup Tool WebUI: Add functional HTML and skeleton JS for main content

Also add two icons, including a custom icon.

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

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

[proberge, 2017-03-30 20:08:27]

Description was changed from

==========
Cleanup Tool WebUI: Add functional HTML and skeleton JS for main content

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

to

==========
Cleanup Tool WebUI: Add functional HTML and skeleton JS for main content

Also add two icons, including a custom icon.

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

[proberge, 2017-03-30 20:08:27]

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

[tommycli, 2017-03-30 20:18:00]

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
File chrome/browser/resources/cleanup_tool/manager.html (right):

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.html:107: class$="icon
[[getIconClassName_(hasScanResults, isInfected)]]">
Can we just give this icon an id instead of a class called 'icon'?

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.html:119: <paper-spinner
active="[[isRunningScanner]]"></paper-spinner>
If it's hidden, can this paper-spinner always be active? (No need to hide parent
and also change if this one is active)

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
File chrome/browser/resources/cleanup_tool/manager.js (right):

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.js:99: requestLastScanResult:
function() {
I think this can be private if the suggestion on line 128 makes sense to you.

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.js:128:
cleanup.Manager.get().requestLastScanResult();
I think you don't need to define Manager inside a cr.define and keep a
singelton.

Can you just call this.requestLastScanResult within the attached lifecycle
callback for this element?

[proberge, 2017-03-30 21:04:33]

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
File chrome/browser/resources/cleanup_tool/manager.html (right):

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.html:107: class$="icon
[[getIconClassName_(hasScanResults, isInfected)]]">
On 2017/03/30 20:18:00, tommycli wrote:
> Can we just give this icon an id instead of a class called 'icon'?

I previously gave this icon a class "icon" and an id (clean-icon or
infected-icon). The method to get the id was named getIconId_, which was unclear
when seen in the Javascript file (not obvious that it means CSS id).
getIconClassName_, on the other hand, is quite descriptive.

Is $= frowned upon?

(or do you mean that I should replicate the height:32px, width:32px rules in
.clean-icon and .infected-icon?)

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.html:119: <paper-spinner
active="[[isRunningScanner]]"></paper-spinner>
On 2017/03/30 20:18:00, tommycli wrote:
> If it's hidden, can this paper-spinner always be active? (No need to hide
parent
> and also change if this one is active)

Done.

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
File chrome/browser/resources/cleanup_tool/manager.js (right):

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.js:99: requestLastScanResult:
function() {
On 2017/03/30 20:18:00, tommycli wrote:
> I think this can be private if the suggestion on line 128 makes sense to you.

Acknowledged.

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.js:128:
cleanup.Manager.get().requestLastScanResult();
On 2017/03/30 20:18:00, tommycli wrote:
> I think you don't need to define Manager inside a cr.define and keep a
> singelton.
> 
> Can you just call this.requestLastScanResult within the attached lifecycle
> callback for this element?

The document.addEventListener is outside of the cr.define. In the anonymous
function's scope, 'this' refers to the document. The document doesn't have a
requestLastScanResult. 

In existing code, it's fairly rare that the addEventListener will be inside the
cr.define. See
https://cs.chromium.org/search/?q=document.addEventListener.*DOMContent+f:bro...

[Fabio Tirelo, 2017-04-05 15:28:25]

ftirelo@google.com changed reviewers:
+ ftirelo@google.com

[Fabio Tirelo, 2017-04-05 15:28:31]

lgtm

[tommycli, 2017-04-05 20:47:26]

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
File chrome/browser/resources/cleanup_tool/manager.html (right):

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.html:107: class$="icon
[[getIconClassName_(hasScanResults, isInfected)]]">
On 2017/03/30 21:04:33, proberge wrote:
> On 2017/03/30 20:18:00, tommycli wrote:
> > Can we just give this icon an id instead of a class called 'icon'?
> 
> I previously gave this icon a class "icon" and an id (clean-icon or
> infected-icon). The method to get the id was named getIconId_, which was
unclear
> when seen in the Javascript file (not obvious that it means CSS id).
> getIconClassName_, on the other hand, is quite descriptive.
> 
> Is $= frowned upon?
> 
> (or do you mean that I should replicate the height:32px, width:32px rules in
> .clean-icon and .infected-icon?)

I think the getIconClassName_ is just fine. I'm just suggesting giving this an
id, because you have a css rule for .icon, which only applies to that single
element...

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
File chrome/browser/resources/cleanup_tool/manager.js (right):

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.js:128:
cleanup.Manager.get().requestLastScanResult();
On 2017/03/30 21:04:33, proberge wrote:
> On 2017/03/30 20:18:00, tommycli wrote:
> > I think you don't need to define Manager inside a cr.define and keep a
> > singelton.
> > 
> > Can you just call this.requestLastScanResult within the attached lifecycle
> > callback for this element?
> 
> The document.addEventListener is outside of the cr.define. In the anonymous
> function's scope, 'this' refers to the document. The document doesn't have a
> requestLastScanResult. 
> 
> In existing code, it's fairly rare that the addEventListener will be inside
the
> cr.define. See
>
https://cs.chromium.org/search/?q=document.addEventListener.*DOMContent+f:bro...

Hey... I don't think you need document.addEventListener at all.

If you use the 'attached' lifecycle callback for the element, you can put the
requestLastScanResultCode within there, and it will fire once the element is
added to the document.

[proberge, 2017-04-05 21:34:06]

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
File chrome/browser/resources/cleanup_tool/manager.html (right):

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.html:107: class$="icon
[[getIconClassName_(hasScanResults, isInfected)]]">
On 2017/04/05 20:47:25, tommycli wrote:
> On 2017/03/30 21:04:33, proberge wrote:
> > On 2017/03/30 20:18:00, tommycli wrote:
> > > Can we just give this icon an id instead of a class called 'icon'?
> > 
> > I previously gave this icon a class "icon" and an id (clean-icon or
> > infected-icon). The method to get the id was named getIconId_, which was
> unclear
> > when seen in the Javascript file (not obvious that it means CSS id).
> > getIconClassName_, on the other hand, is quite descriptive.
> > 
> > Is $= frowned upon?
> > 
> > (or do you mean that I should replicate the height:32px, width:32px rules in
> > .clean-icon and .infected-icon?)
> 
> I think the getIconClassName_ is just fine. I'm just suggesting giving this an
> id, because you have a css rule for .icon, which only applies to that single
> element...

Done.

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
File chrome/browser/resources/cleanup_tool/manager.js (right):

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.js:99: requestLastScanResult:
function() {
On 2017/03/30 21:04:33, proberge wrote:
> On 2017/03/30 20:18:00, tommycli wrote:
> > I think this can be private if the suggestion on line 128 makes sense to
you.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2788743002/diff/1/chrome/browser/resources/cl...
chrome/browser/resources/cleanup_tool/manager.js:128:
cleanup.Manager.get().requestLastScanResult();
On 2017/04/05 20:47:26, tommycli wrote:
> On 2017/03/30 21:04:33, proberge wrote:
> > On 2017/03/30 20:18:00, tommycli wrote:
> > > I think you don't need to define Manager inside a cr.define and keep a
> > > singelton.
> > > 
> > > Can you just call this.requestLastScanResult within the attached lifecycle
> > > callback for this element?
> > 
> > The document.addEventListener is outside of the cr.define. In the anonymous
> > function's scope, 'this' refers to the document. The document doesn't have a
> > requestLastScanResult. 
> > 
> > In existing code, it's fairly rare that the addEventListener will be inside
> the
> > cr.define. See
> >
>
https://cs.chromium.org/search/?q=document.addEventListener.*DOMContent+f:bro...
> 
> Hey... I don't think you need document.addEventListener at all.
> 
> If you use the 'attached' lifecycle callback for the element, you can put the
> requestLastScanResultCode within there, and it will fire once the element is
> added to the document.

Oops yeah you're completely right. I don't know enough polymer and didn't
recognize 'attached' as a polymer object method. Done.

[tommycli, 2017-04-07 19:23:07]

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

https://codereview.chromium.org/2788743002/diff/60001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/manager.js:128: };
I suspect that the cr.define, the Manager.get, and the return {Manager: Manager}
are all unnecessary

[proberge, 2017-04-07 20:21:53]

On 2017/04/07 19:23:07, tommycli wrote:
>
https://codereview.chromium.org/2788743002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/cleanup_tool/manager.js (right):
> 
>
https://codereview.chromium.org/2788743002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/cleanup_tool/manager.js:128: };
> I suspect that the cr.define, the Manager.get, and the return {Manager:
Manager}
> are all unnecessary

The Manager.get() is indeed no longer necessary.

For cr.define(... function() { return Manager: Manager }), I looked through a
few Polymer files in browser/resources. 
Both patterns (with cr.define and without) seem to be used. 
Do we typically wrap in cr.define only when the polymer object will be accessed
by other javascript files?

[tommycli, 2017-04-07 20:23:38]

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

https://codereview.chromium.org/2788743002/diff/60001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/manager.js:128: };
On 2017/04/07 19:23:07, tommycli wrote:
> I suspect that the cr.define, the Manager.get, and the return {Manager:
Manager}
> are all unnecessary

The cr.define is to namespace Manager into the 'cleanup' namespace, but if
you're not defining Manager at all, and just calling Polymer({...}) (which is
just a plain function), there's nothing added to the namespace anyways.

[Dan Beam, 2017-04-07 21:49:00]

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

https://codereview.chromium.org/2788743002/diff/60001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/manager.js:128: };
On 2017/04/07 20:23:38, tommycli wrote:
> On 2017/04/07 19:23:07, tommycli wrote:
> > I suspect that the cr.define, the Manager.get, and the return {Manager:
> Manager}
> > are all unnecessary
> 
> The cr.define is to namespace Manager into the 'cleanup' namespace, but if
> you're not defining Manager at all, and just calling Polymer({...}) (which is
> just a plain function), there's nothing added to the namespace anyways.

that's not totally true, but close enough (if the result is not used,
CleanupManagerElement is exported as a closure compiler type)

[tommycli, 2017-04-07 22:17:20]

On 2017/04/07 21:49:00, Dan Beam wrote:
>
https://codereview.chromium.org/2788743002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/cleanup_tool/manager.js (right):
> 
>
https://codereview.chromium.org/2788743002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/cleanup_tool/manager.js:128: };
> On 2017/04/07 20:23:38, tommycli wrote:
> > On 2017/04/07 19:23:07, tommycli wrote:
> > > I suspect that the cr.define, the Manager.get, and the return {Manager:
> > Manager}
> > > are all unnecessary
> > 
> > The cr.define is to namespace Manager into the 'cleanup' namespace, but if
> > you're not defining Manager at all, and just calling Polymer({...}) (which
is
> > just a plain function), there's nothing added to the namespace anyways.
> 
> that's not totally true, but close enough (if the result is not used,
> CleanupManagerElement is exported as a closure compiler type)

tru dat

[proberge, 2017-04-08 02:03:11]

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

https://codereview.chromium.org/2788743002/diff/60001/chrome/browser/resource...
chrome/browser/resources/cleanup_tool/manager.js:128: };
On 2017/04/07 21:48:59, Dan Beam wrote:
> On 2017/04/07 20:23:38, tommycli wrote:
> > On 2017/04/07 19:23:07, tommycli wrote:
> > > I suspect that the cr.define, the Manager.get, and the return {Manager:
> > Manager}
> > > are all unnecessary
> > 
> > The cr.define is to namespace Manager into the 'cleanup' namespace, but if
> > you're not defining Manager at all, and just calling Polymer({...}) (which
is
> > just a plain function), there's nothing added to the namespace anyways.
> 
> that's not totally true, but close enough (if the result is not used,
> CleanupManagerElement is exported as a closure compiler type)

Gotcha, thanks!

[proberge, 2017-04-08 02:04:24]

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

[proberge, 2017-04-08 02:04:25]

++dbeam for OWNERs review of browser_resources.grd

[tommycli, 2017-04-10 20:31:46]

lgtm great

[Dan Beam, 2017-04-10 20:42:00]

browser_resources.grd lgtm

[proberge, 2017-04-10 20:45:30]

The CQ bit was checked by proberge@chromium.org

[proberge, 2017-04-10 20:45:31]

The patchset sent to the CQ was uploaded after l-g-t-m from ftirelo@google.com
Link to the patchset: https://codereview.chromium.org/2788743002/#ps80001
(title: "Remove unused cr.define and Manager.get")

[commit-bot: I haz the power, 2017-04-10 20:46:11]

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-10 21:01:38]

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

[commit-bot: I haz the power, 2017-04-10 21:01:40]

Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[proberge, 2017-04-10 21:26:31]

The CQ bit was checked by proberge@chromium.org

[proberge, 2017-04-10 21:26:31]

The patchset sent to the CQ was uploaded after l-g-t-m from ftirelo@google.com,
dbeam@chromium.org, tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2788743002/#ps100001
(title: "Fix typo and make manager.js closure deps include
cleanup_browser_proxy")

[commit-bot: I haz the power, 2017-04-10 21:26:59]

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-10 22:38:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491859591058740,
"parent_rev": "305068050c006f0d1e46f3c9d5df74f7519cf3a4", "commit_rev":
"a1257442d4f1d35e9a7527ee51672c43502f18f2"}

[commit-bot: I haz the power, 2017-04-10 22:39:20]

Description was changed from

==========
Cleanup Tool WebUI: Add functional HTML and skeleton JS for main content

Also add two icons, including a custom icon.

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

to

==========
Cleanup Tool WebUI: Add functional HTML and skeleton JS for main content

Also add two icons, including a custom icon.

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

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

[commit-bot: I haz the power, 2017-04-10 22:39:21]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a1257442d4f1d35e9a7527ee5167...