[MD settings] idle load basic and advanced pages

chrome/browser/resources/settings/controls/settings_idle_render.js

Index: chrome/browser/resources/settings/controls/settings_idle_render.js
diff --git a/chrome/browser/resources/settings/controls/settings_idle_render.js b/chrome/browser/resources/settings/controls/settings_idle_render.js
new file mode 100644
index 0000000000000000000000000000000000000000..8e224c9e5414b54437e08f556a7411fdbb2a9868
--- /dev/null
+++ b/chrome/browser/resources/settings/controls/settings_idle_render.js
@@ -0,0 +1,76 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview
+ * settings-idle-render is a simple variant of dom-if designed for lazy
+ * rendering of elements that are accessed imperatively.
+ * If a use for idle time expansion is found outside of settings, this code
+ * should be replaced by cr-lazy-render after that feature is merged into
+ * ui/webui/resources/cr_elements/cr_lazy_render/cr_lazy_render.js
+ */
+
+Polymer({
+  is: 'settings-idle-render',
+  extends: 'template',
+
+  behaviors: [Polymer.Templatizer],
+
+  /** @private {TemplatizerNode} */
+  child_: null,
+
+  /** @private {number} */
+  idleCallback_: 0,
+
+  /** @override */
+  attached: function() {
+    this.idleCallback_ = requestIdleCallback(this.render_.bind(this));
+  },
+
+  /** @override */
+  detached: function() {
+    // No-op if callback already fired.
+    cancelIdleCallback(this.idleCallback_);
+  },
+
+  /**
+   * Stamp the template into the DOM tree synchronously
+   * @return {Element} Child element which has been stamped into the DOM tree.
+   */
+  get: function() {
+    if (!this.child_)
+      this.render_();
+    return this.child_;
+  },
+
+  /** @private */
+  render_: function() {
+    if (!this.ctor)
+      this.templatize(this);
+    var parentNode = this.parentNode;

[michaelpg, 2017/02/01 07:53:50]

is it really worth creating a variable for this.parentNode?

[dschuyler, 2017/02/01 21:52:30]

I'm curious about the concern. Most reviewers so far have asked to make a var if
something is used twice. (Conversely, it's split about 50/50 on whether to
inline something that is only used once).

[michaelpg, 2017/02/02 00:00:35]

That may be true for some variables, but we're talking about a property/"member
variable". We don't do this every time we use a property more than once in a
function:

    var foo = this.foo;
    doSomething(foo);
    if (foo)
      doAnotherThing();

This function (and _forwardParentProp) doesn't use "var child = this.child_"
even though it would be used twice.

If anything, I'll argue that creating the variable removes context -- with
|this.foo| the reader immediately knows where that comes from.

[Dan Beam, 2017/02/02 00:19:36]

i don't think it matters either way

[Dan Beam, 2017/02/02 00:19:36]

waiiiiit, why do you have to check parentNode?  does this mean if you call
.get() when this is detached we return undefined?  would that cause your
assert() to fail?

[dschuyler, 2017/02/02 02:28:15]

I agree that it's unlikely to matter either way and I'd prefer to keep this in
sync with cr-lazy-render unless there is a solid reason for diverging.

[dschuyler, 2017/02/02 02:28:15]

I asked Tim about this (he's the original author), his answer is that this is
what polymer does. But it seems very sensible, since it would also crash/throw
on line 55 (where it's dereferenced) if it were invalid.
Yes, because a template must expand onto something. A template connects a child
to a parent and the different templates just differ by how that is triggered. A
(any) template must have both a child and a parent to be expanded.
I made a reply in the other comment where the assert is located.

[Dan Beam, 2017/02/02 03:56:17]

I guess I mean: the idle callback is created on attached(), and cancelled on
detached.  so i don't see a case where this could fire without this.isAttached /
this.parentNode being tru{e,thy}.  can we assert() this instead?

[dschuyler, 2017/02/02 22:13:54]

Ah. I've made a follow-up CL to explore that question at CL 2669153003.
tsergent@ is also a reviewer on that one because I'd like to change
cr-lazy-render in a similar way.

+    if (parentNode && !this.child_) {
+      var instance = this.stamp({});
+      this.child_ = instance.root.firstElementChild;
+      parentNode.insertBefore(instance.root, this);
+    }
+  },
+
+  /**
+   * @param {string} prop
+   * @param {Object} value
+   */
+  _forwardParentProp: function(prop, value) {
+    if (this.child_)
+      this.child_._templateInstance[prop] = value;

[michaelpg, 2017/02/01 07:53:50]

this bypasses __setProperty which does some stuff and is used by Polymer's
dom-if, dom-repeat, etc. in their overrides of _forwardParentProp. do we know
why it's safe to set the property directly (does it trigger all of the effects,
is there a performance optimization this skips)?

[dschuyler, 2017/02/01 21:52:31]

It's done this way in cr-lazy-render (which settings-idle-render is modeled
from). If this is incorrect, it should be addressed  in both classes. So we know
it's been done elsewhere in our code. (Long term, I'd like to merge this code
with cr-lazy-render so that's another reason I'd like them to be consistent with
each other). Is there something specific to test for to know whether this is an
issue?

Are you suggesting this(?):

if (this.child_)
  this.child_.__setProperty(prop, value, true);

[tsergeant, 2017/02/01 22:14:59]

FWIW, cr-lazy-render used to use _setProperty here, but changed to the direct
assignment version in https://codereview.chromium.org/2344803002. Like the CL
description says, this matches what iron-list does, which means that it's
hopefully a safe way to do it.

[michaelpg, 2017/02/02 00:00:35]

SGTM, thanks for the info.

+  },
+
+  /**
+   * @param {string} path
+   * @param {Object} value
+   */
+  _forwardParentPath: function(path, value) {
+    if (this.child_)
+      this.child_._templateInstance.notifyPath(path, value, true);
+  }
+});