[MD settings] handle being detached on cr-lazy-render and settings-idle-render

[MD settings] handle being detached on cr-lazy-render and settings-idle-render

This CL causes a detached lazy/idle render to return null from .get() if the template has been detached.

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

[dschuyler, 2017-02-02 22:08:14]

Description was changed from

==========
[MD settings] use assert in render_ of cr-lazy-render and settings-idle-render

This CL changes some conditionals to asserts, since the conditions
should not occur (and therefor an error message is preferred).

BUG=None
==========

to

==========
[MD settings] use assert in render_ of cr-lazy-render and settings-idle-render

This CL changes some conditionals to asserts, since the conditions
should not occur (and therefor an error message is preferred).

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

[dschuyler, 2017-02-02 22:08:16]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 22:08:43]

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

[dschuyler, 2017-02-02 22:10:11]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 22:10:23]

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

[dschuyler, 2017-02-02 22:10:32]

Patchset #1 (id:1) has been deleted

[dschuyler, 2017-02-02 22:12:15]

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

[dschuyler, 2017-02-02 22:12:15]

Follow up CL for https://codereview.chromium.org/2660383002/

[commit-bot: I haz the power, 2017-02-02 22:23:01]

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

[commit-bot: I haz the power, 2017-02-02 22:23:02]

Dry run: 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...)

[tsergeant, 2017-02-02 22:54:21]

Generally lg, although looks like you need to make assert() visible to closure
and the tests

[dschuyler, 2017-02-02 23:17:12]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 23:17:43]

Dry run: 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-02-03 00:06:21]

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

[commit-bot: I haz the power, 2017-02-03 00:06:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dschuyler, 2017-02-03 01:32:14]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 01:33:16]

Dry run: 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-02-03 02:33:35]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Dan Beam, 2017-02-03 15:54:24]

I think cr-lazy-render will blow up if .get() is called before being attached

whereas settings-idle-render doesn't have this problem (I think)

[dschuyler, 2017-02-06 20:05:43]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 20:06:25]

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

[dschuyler, 2017-02-06 20:11:52]

On 2017/02/03 15:54:24, Dan Beam (slow) wrote:
> I think cr-lazy-render will blow up if .get() is called before being attached
> 
> whereas settings-idle-render doesn't have this problem (I think)

Like in path 3 there would be an issue, or prior (or post) this CL there would
be an issue.

There was an issue that I just addressed about the callback, triggering after a
.get(). Patch 4 fixes that. Does that fix also handle this concern?

[commit-bot: I haz the power, 2017-02-06 21:06:53]

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

[commit-bot: I haz the power, 2017-02-06 21:06:54]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-02-08 05:31:36]

https://codereview.chromium.org/2669153003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_idle_render.js (right):

https://codereview.chromium.org/2669153003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_idle_render.js:32: detached:
function() {
because you have a detached method, I assume this element can be removed from
the DOM.

if it's removed from the DOM, this.parentNode may be null.

get() may call render_(), which requires this.parentNode to be non-null.

so I think:

document.createElement('template', 'settings-idle-render').get()

would assert.  am I right?  can we change to return undefined if !parentNode
when calling render_() the first time?  or at least document this requirement?

relevant: same issue in dom-if https://github.com/Polymer/polymer/pull/2696

https://codereview.chromium.org/2669153003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2669153003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:51: ROOT_PATH +
'ui/webui/resources/js/assert.js',
please don't add dependencies for production code in tests only.

please, instead, add a <link rel="import"
href="chrome://resources/html/assert.html"> in settings_idle_render.html

backstory: dpapad@ and I had to remove a bunch of "only load this file in tests"
via importHtml(), loadScript(), and extraLibraries because folks were adding
dependencies only in tests when they should just be included in the code that
uses them.

[dschuyler, 2017-02-09 00:31:26]

Description was changed from

==========
[MD settings] use assert in render_ of cr-lazy-render and settings-idle-render

This CL changes some conditionals to asserts, since the conditions
should not occur (and therefor an error message is preferred).

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

to

==========
[MD settings] handle being detached on cr-lazy-render and settings-idle-render

This CL causes a detached lazy/idle render to return null from .get() if the
template has been detached.

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

[dschuyler, 2017-02-09 00:40:56]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 00:41:30]

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

[dschuyler, 2017-02-09 00:42:48]

Dan's question about calling .get on a detached template lead me to rethink this
CL. It's changed quite a bit, so I've updated the description.

https://codereview.chromium.org/2669153003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_idle_render.js (right):

https://codereview.chromium.org/2669153003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_idle_render.js:32: detached:
function() {
On 2017/02/08 05:31:36, Dan Beam wrote:
> because you have a detached method, I assume this element can be removed from
> the DOM.
> 
> if it's removed from the DOM, this.parentNode may be null.
> 
> get() may call render_(), which requires this.parentNode to be non-null.
> 
> so I think:
> 
> document.createElement('template', 'settings-idle-render').get()
> 
> would assert.  am I right?  can we change to return undefined if !parentNode
> when calling render_() the first time?  or at least document this requirement?
> 
> relevant: same issue in dom-if https://github.com/Polymer/polymer/pull/2696

That makes sense. Rather than checking !parentNode, the child_ can be set to
null on detach and the rest of the logic looks like it works.
Since this situation could also occur with cr-lazy-render, a child_ = null has
been added there as well.

https://codereview.chromium.org/2669153003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2669153003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:51: ROOT_PATH +
'ui/webui/resources/js/assert.js',
On 2017/02/08 05:31:36, Dan Beam wrote:
> please don't add dependencies for production code in tests only.
> 
> please, instead, add a <link rel="import"
> href="chrome://resources/html/assert.html"> in settings_idle_render.html
> 
> backstory: dpapad@ and I had to remove a bunch of "only load this file in
tests"
> via importHtml(), loadScript(), and extraLibraries because folks were adding
> dependencies only in tests when they should just be included in the code that
> uses them.

Thanks. With the other note about detached settings-idle-render having .get()
called, I found that the asserts no longer applied.

[commit-bot: I haz the power, 2017-02-09 02:45:16]

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

[commit-bot: I haz the power, 2017-02-09 02:45:18]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-02-11 23:01:35]

https://codereview.chromium.org/2669153003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_idle_render.js (right):

https://codereview.chromium.org/2669153003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:33:
this.child_ = null;
but wait, does this mean it could get double-rendered?

can we maybe rename child_ to something less cryptic?

[tsergeant, 2017-02-15 06:31:40]

https://codereview.chromium.org/2669153003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_idle_render.js (right):

https://codereview.chromium.org/2669153003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:33:
this.child_ = null;
On 2017/02/11 23:01:35, Dan Beam wrote:
> but wait, does this mean it could get double-rendered?
> 
> can we maybe rename child_ to something less cryptic?

Yeah, if you detach the settings-idle-render, its internal state is reset to
having no child, but that child can still be rendered and attached to the DOM
tree.

dom-if handles this by ensuring that the rendered template instance is destroyed
when the template is detached:

https://github.com/Polymer/polymer/blob/6fc567fbf4e69ab1be2623212d5254eb38515...