MD Settings: Creating WebUIListenerBehavior helper.

MD Settings: Creating WebUIListenerBehavior helper.

Any Polymer element that wants to automatically remove WebUI listeners
when detached, should use the new Behavior class.

Committed: https://crrev.com/1ea0684508a371582f62aed5a2b501e554144671
Cr-Commit-Position: refs/heads/master@{#377041}

[dpapad, 2016-02-16 23:38:16]

Description was changed from

==========
MD Settings: Creating WebUIListenerBehavior helper.

BUG=
==========

to

==========
MD Settings: Creating WebUIListenerBehavior helper.

Any Polymer element that wants to automatically remove WebUI listeners
when detached, should use the new Behavior class.

BUG=
==========

[dpapad, 2016-02-16 23:40:56]

Patchset #3 (id:40001) has been deleted

[dpapad, 2016-02-19 18:45:51]

Description was changed from

==========
MD Settings: Creating WebUIListenerBehavior helper.

Any Polymer element that wants to automatically remove WebUI listeners
when detached, should use the new Behavior class.

BUG=
==========

to

==========
MD Settings: Creating WebUIListenerBehavior helper.

Any Polymer element that wants to automatically remove WebUI listeners
when detached, should use the new Behavior class.
==========

[dpapad, 2016-02-19 18:58:50]

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

[Dan Beam, 2016-02-19 20:00:28]

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

[Dan Beam, 2016-02-19 20:00:29]

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:13: var
SettingsBehaviors = SettingsBehaviors || {};
wat?

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:16:
SettingsBehaviors.WebUIListenerBehavior = {
Behaviors Behavior?

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:25: value: [],
function() { return []; },

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:45: var listener =
this.webUIListeners_.splice(0, 1)[0];
shift() or pop()

[dpapad, 2016-02-19 20:21:24]

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:13: var
SettingsBehaviors = SettingsBehaviors || {};
On 2016/02/19 at 20:00:29, Dan Beam wrote:
> wat?

From https://www.polymer-project.org/1.0/docs/devguide/behaviors.html.

"When creating your own behaviors, you should use some other namespace to avoid
collisions with future Polymer behaviors. For example:
window.MyBehaviors = window.MyBehaviors || {};"

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:16:
SettingsBehaviors.WebUIListenerBehavior = {
On 2016/02/19 at 20:00:29, Dan Beam wrote:
> Behaviors Behavior?

I am just using SettingsBehaviors as a namespace, see previous comment.

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:25: value: [],
On 2016/02/19 at 20:00:29, Dan Beam wrote:
> function() { return []; },

Our presubmit script complains when I do that (missing @return for function with
non-trivial return).

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:45: var listener =
this.webUIListeners_.splice(0, 1)[0];
On 2016/02/19 at 20:00:29, Dan Beam wrote:
> shift() or pop()

Done.

[tommycli, 2016-02-19 20:30:34]

lgtm sans below comment:

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:16:
SettingsBehaviors.WebUIListenerBehavior = {
On 2016/02/19 20:21:24, dpapad wrote:
> On 2016/02/19 at 20:00:29, Dan Beam wrote:
> > Behaviors Behavior?
> 
> I am just using SettingsBehaviors as a namespace, see previous comment.

Would it be possible to reuse the existing cr-defined 'settings' namespace
instead of making a new SettingsBehaviors namespace?

[dpapad, 2016-02-19 20:40:14]

On 2016/02/19 at 20:30:34, tommycli wrote:
> lgtm sans below comment:
> 
>
https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
> File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):
> 
>
https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
> chrome/browser/resources/settings/web_ui_listener_behavior.js:16:
SettingsBehaviors.WebUIListenerBehavior = {
> On 2016/02/19 20:21:24, dpapad wrote:
> > On 2016/02/19 at 20:00:29, Dan Beam wrote:
> > > Behaviors Behavior?
> > 
> > I am just using SettingsBehaviors as a namespace, see previous comment.
> 
> Would it be possible to reuse the existing cr-defined 'settings' namespace
instead of making a new SettingsBehaviors namespace?

I will try this, but whatever we end up choosing, should probably be cosistent
across all our behaviors, see other existing behavior at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...,
which is just blindly setting a new global.

[dpapad, 2016-02-19 22:29:17]

On 2016/02/19 at 20:40:14, dpapad wrote:
> On 2016/02/19 at 20:30:34, tommycli wrote:
> > lgtm sans below comment:
> > 
> >
https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
> > File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):
> > 
> >
https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
> > chrome/browser/resources/settings/web_ui_listener_behavior.js:16:
SettingsBehaviors.WebUIListenerBehavior = {
> > On 2016/02/19 20:21:24, dpapad wrote:
> > > On 2016/02/19 at 20:00:29, Dan Beam wrote:
> > > > Behaviors Behavior?
> > > 
> > > I am just using SettingsBehaviors as a namespace, see previous comment.
> > 
> > Would it be possible to reuse the existing cr-defined 'settings' namespace
instead of making a new SettingsBehaviors namespace?
> 
> I will try this, but whatever we end up choosing, should probably be cosistent
across all our behaviors, see other existing behavior at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...,
which is just blindly setting a new global.

Done, using cr.define now. I am not updating existing Behaviors in this CL
though, since we have about 5-6 of them, probably better to do in a follow up.

[Dan Beam, 2016-02-19 22:32:09]

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:25: value: [],
On 2016/02/19 20:21:24, dpapad wrote:
> On 2016/02/19 at 20:00:29, Dan Beam wrote:
> > function() { return []; },
> 
> Our presubmit script complains when I do that (missing @return for function
with
> non-trivial return).

this class will share |this.webUIListener_| across all instances if you don't
put this.

don't really care much about the presubmit.

[dpapad, 2016-02-19 22:37:28]

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:16:
SettingsBehaviors.WebUIListenerBehavior = {
On 2016/02/19 at 20:30:34, tommycli wrote:
> On 2016/02/19 20:21:24, dpapad wrote:
> > On 2016/02/19 at 20:00:29, Dan Beam wrote:
> > > Behaviors Behavior?
> > 
> > I am just using SettingsBehaviors as a namespace, see previous comment.
> 
> Would it be possible to reuse the existing cr-defined 'settings' namespace
instead of making a new SettingsBehaviors namespace?

Done.

https://codereview.chromium.org/1702063003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:25: value: [],
On 2016/02/19 at 22:32:09, Dan Beam wrote:
> On 2016/02/19 20:21:24, dpapad wrote:
> > On 2016/02/19 at 20:00:29, Dan Beam wrote:
> > > function() { return []; },
> > 
> > Our presubmit script complains when I do that (missing @return for function
with
> > non-trivial return).
> 
> this class will share |this.webUIListener_| across all instances if you don't
put this.
> 
> don't really care much about the presubmit.

Done. Thanks for the context, makes sense.

[dpapad, 2016-02-22 21:48:31]

+Michael,

This CL is another reason why I chose not to wrap the cr.addWebUIListener in the
proxy. Adding and removing handlers automatically will not be as transparent if
the BrowserProxy is the one actually calling cr.addWebUIListener.

[dpapad, 2016-02-22 21:50:13]

On 2016/02/22 at 21:48:31, dpapad wrote:
> +Michael,
> 
> This CL is another reason why I chose not to wrap the cr.addWebUIListener in
the proxy. Adding and removing handlers automatically will not be as transparent
if the BrowserProxy is the one actually calling cr.addWebUIListener.

Anyway, I can modify this CL to work either way though (explicitly add the
listener to an array held by the behavior), so extracting this as as behavior is
not a differentiator on whether to wrap cr.addWebUIListenerCallback or not.

[Dan Beam, 2016-02-23 02:38:28]

lgtm

https://codereview.chromium.org/1702063003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):

https://codereview.chromium.org/1702063003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:42: detached:
function() {
it may make more sense for this to be named removeAllListeners and for those
this behavior to call from detached()

this is OK for now

[Dan Beam, 2016-02-23 02:51:07]

https://codereview.chromium.org/1702063003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/web_ui_listener_behavior.js (right):

https://codereview.chromium.org/1702063003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/web_ui_listener_behavior.js:42: detached:
function() {
On 2016/02/23 02:38:28, Dan Beam wrote:
> it may make more sense for this to be named removeAllListeners and for those
> this behavior to call from detached()

for those that want this behavior*

> 
> this is OK for now

[dpapad, 2016-02-23 18:06:43]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-02-23 18:06:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1702063003/#ps200001
(title: "Resolve conflicts with ToT")

[commit-bot: I haz the power, 2016-02-23 18:13:18]

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

[commit-bot: I haz the power, 2016-02-23 19:20:07]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-02-23 19:22:06]

Description was changed from

==========
MD Settings: Creating WebUIListenerBehavior helper.

Any Polymer element that wants to automatically remove WebUI listeners
when detached, should use the new Behavior class.
==========

to

==========
MD Settings: Creating WebUIListenerBehavior helper.

Any Polymer element that wants to automatically remove WebUI listeners
when detached, should use the new Behavior class.

Committed: https://crrev.com/1ea0684508a371582f62aed5a2b501e554144671
Cr-Commit-Position: refs/heads/master@{#377041}
==========

[commit-bot: I haz the power, 2016-02-23 19:22:06]

Patchset 10 (id:??) landed as
https://crrev.com/1ea0684508a371582f62aed5a2b501e554144671
Cr-Commit-Position: refs/heads/master@{#377041}