Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2660383002:
[MD settings] idle load basic and advanced pages (Closed)
|
Created:
3 years, 10 months ago by dschuyler Modified:
3 years, 10 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, tsergeant Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] idle load basic and advanced pages
This CL uses lazy loading templates to preload the advanced page while
the basic page is idle. The result is that the advanced page toggles
much faster (addressing the 'jank' mentioned in the bug).
This CL is replacing CL 2652443002.
BUG=681238
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2660383002
Cr-Commit-Position: refs/heads/master@{#448757}
Committed: https://chromium.googlesource.com/chromium/src/+/f7247aef78ab5a3aa4cc8d61431893bdb6b4e68f
Patch Set 1 #
Total comments: 6
Patch Set 2 : review changes #
Total comments: 8
Patch Set 3 : review changes #
Total comments: 8
Patch Set 4 : added unit test #
Total comments: 32
Patch Set 5 : merge with master; changed test name #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 60 (36 generated)
Description was changed from ========== [MD settings] lazy load basic and advanced pages This CL uses lazy loading templates to preload the advanced page while the basic page is idle. The result is that the advanced page toggles much faster (addressing the 'jank' mentioned in the bug). BUG=681238 ========== to ========== [MD settings] lazy load basic and advanced pages This CL uses lazy loading templates to preload the advanced page while the basic page is idle. The result is that the advanced page toggles much faster (addressing the 'jank' mentioned in the bug). BUG=681238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
as i mentioned, i like placing thing in the narrowest scope required, so keeping this settings-specific helps us know who's using this and how and makes it easier for us to change for now. so I vote this stays in settings until used elsewhere, just like the rest of our custom elements in c/b/r/settings/controls/. https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/basic_page/basic_page.html:160: render-when-idle> why would we need render-when-idle if this element is only used here? why not just invoke requestIdleCallback() from attached()? https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/settings_lazy_render.js (right): https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/settings_lazy_render.js:15: is: 'settings-lazy-render', settings-idle-render? https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/settings_lazy_render.js:66: getIfExists: function() { where is this used?
Description was changed from ========== [MD settings] lazy load basic and advanced pages This CL uses lazy loading templates to preload the advanced page while the basic page is idle. The result is that the advanced page toggles much faster (addressing the 'jank' mentioned in the bug). BUG=681238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] idle load basic and advanced pages This CL uses lazy loading templates to preload the advanced page while the basic page is idle. The result is that the advanced page toggles much faster (addressing the 'jank' mentioned in the bug). BUG=681238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dschuyler@chromium.org changed reviewers: + dpapad@chromium.org
Description was changed from ========== [MD settings] idle load basic and advanced pages This CL uses lazy loading templates to preload the advanced page while the basic page is idle. The result is that the advanced page toggles much faster (addressing the 'jank' mentioned in the bug). BUG=681238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] idle load basic and advanced pages This CL uses lazy loading templates to preload the advanced page while the basic page is idle. The result is that the advanced page toggles much faster (addressing the 'jank' mentioned in the bug). This CL is replacing CL 2652443002. BUG=681238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This includes changes by dpapad@ from the other CL. https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/basic_page/basic_page.html:160: render-when-idle> On 2017/01/31 03:19:21, Dan Beam wrote: > why would we need render-when-idle if this element is only used here? why not > just invoke requestIdleCallback() from attached()? We'd also want the .get() and .render_(). It seems cleaner to keep it separate as it's a separate concept. https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/settings_lazy_render.js (right): https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/settings_lazy_render.js:15: is: 'settings-lazy-render', On 2017/01/31 03:19:21, Dan Beam wrote: > settings-idle-render? Done. https://codereview.chromium.org/2660383002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/settings_lazy_render.js:66: getIfExists: function() { On 2017/01/31 03:19:21, Dan Beam wrote: > where is this used? It was to keep parity with cr_lazy_render; which has this used in the languages page. I can remove it for now (in case these files never merge) and re-add it later if/when these files are merged.
https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.js:135: * Inform the idle render code that the advanced page to load. Syntax is confusing, probably something is missing from this sentence. https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:26: renderWhenIdle: { Is this attribute necessary? In other words, does it make sense to use that subclass without the 'render-when-idle' attribute set to true? If not, can we simply make this the default behavior and remove this attribute completely (along with the change observer)? https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:45: if (this.idleCallback_) Can we avoid implicit boolean conversions? It is OK in this case, but in general it is often the source of subtle bugs in JS code. if (this.idleCallback_ != 0) ... https://codereview.chromium.org/2660383002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:65: var idleRender = page.$$('[is=settings-idle-render]') Can we be more specific in the selector, as follows? page.$$('template[is=settings-idle-render]) This makes it more obvious that a settings-idle-render attribute is only expected in a <template> tag.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.js:135: * Inform the idle render code that the advanced page to load. On 2017/01/31 23:10:13, dpapad wrote: > Syntax is confusing, probably something is missing from this sentence. Done. https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:26: renderWhenIdle: { On 2017/01/31 23:10:13, dpapad wrote: > Is this attribute necessary? In other words, does it make sense to use that > subclass without the 'render-when-idle' attribute set to true? > > If not, can we simply make this the default behavior and remove this attribute > completely (along with the change observer)? Done. https://codereview.chromium.org/2660383002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:45: if (this.idleCallback_) On 2017/01/31 23:10:13, dpapad wrote: > Can we avoid implicit boolean conversions? It is OK in this case, but in general > it is often the source of subtle bugs in JS code. > > if (this.idleCallback_ != 0) > ... Done. https://codereview.chromium.org/2660383002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:65: var idleRender = page.$$('[is=settings-idle-render]') On 2017/01/31 23:10:13, dpapad wrote: > Can we be more specific in the selector, as follows? > page.$$('template[is=settings-idle-render]) > > This makes it more obvious that a settings-idle-render attribute is only > expected in a <template> tag. Done.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.html (right): https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.html:3: <script src="settings_idle_render.js"></script> controls/ folder is currently used for form elements (radio button, dropdown, checkbox, slider). Does a <template> subclass belong in the same folder? Perhaps better to move it one level up under settings/ instead, along with other generic infrastructure elements (like direction_delegate.html)? https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:28: this.idleCallback_ = requestIdleCallback(this.render_.bind(this)); In both tests we are calling get() explicitly. Is the case where the requestIdleCallback takes effect tested somehow (directly or indirectly)? https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:34: cancelIdleCallback(this.idleCallback_); By the time this runs, the callback might have already executed, so canceling could be a no-op. Should we do the following instead? if (this.idleCallback != 0 && !this.child_) cancelIdleCallback(this.idleCallback_); I would also further argue that the framework guarantees that attached() is called before detached() so the first part of the check could be omitted. https://codereview.chromium.org/2660383002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:65: var idleRender = page.$$('template[is=settings-idle-render]') Semicolon missing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Added a browser test js file for settings-idle-render (based on patterns in other browser tests). https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.html (right): https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.html:3: <script src="settings_idle_render.js"></script> On 2017/02/01 00:30:04, dpapad wrote: > controls/ folder is currently used for form elements (radio button, dropdown, > checkbox, slider). Does a <template> subclass belong in the same folder? Perhaps > better to move it one level up under settings/ instead, along with other generic > infrastructure elements (like direction_delegate.html)? Hmm, well Dan said this earlier in this CL > so I vote this stays in settings until used elsewhere, just like the rest of our > custom elements in c/b/r/settings/controls/. I'm concerned about cluttering the parent directory. Maybe a new directory should be made for these things or maybe the name of the 'controls' directory should change to something else. https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:28: this.idleCallback_ = requestIdleCallback(this.render_.bind(this)); On 2017/02/01 00:30:04, dpapad wrote: > In both tests we are calling get() explicitly. Is the case where the > requestIdleCallback takes effect tested somehow (directly or indirectly)? Added browser tests. Done. https://codereview.chromium.org/2660383002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:34: cancelIdleCallback(this.idleCallback_); On 2017/02/01 00:30:04, dpapad wrote: > By the time this runs, the callback might have already executed, so canceling > could be a no-op. Should we do the following instead? > > if (this.idleCallback != 0 && !this.child_) > cancelIdleCallback(this.idleCallback_); > > I would also further argue that the framework guarantees that attached() is > called before detached() so the first part of the check could be omitted. After chatting offline, we reasoned that no conditional at all was a good choice. I included the comment as requested: // No-op if callback already fired. https://codereview.chromium.org/2660383002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:65: var idleRender = page.$$('template[is=settings-idle-render]') On 2017/02/01 00:30:04, dpapad wrote: > Semicolon missing. Done.
LGTM. Thanks for adding tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; is it really worth creating a variable for this.parentNode? https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:65: this.child_._templateInstance[prop] = value; 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)? https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_idle_render_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_idle_render_browsertest.js:56: test('not stamped before idle or get()', function(done) { this test is testing 'stamped after idle', not 'not stamped before idle or get()'. can you add an assert before the requestIdleCallback? https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_main_test.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_main_test.js:179: expectedAdvanced, page.$.advancedPageTemplate.get().style.display); This is confusing: asserting visibility has a side effect of stamping the advanced page if it isn't stamped yet. https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:70: return page; add a comment that getting the basic page force-renders the advanced page (why are these test-behavioral changes needed if the current advanced page is already wrapped in a dom-if?)
https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_main_test.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_main_test.js:179: expectedAdvanced, page.$.advancedPageTemplate.get().style.display); On 2017/02/01 at 07:53:50, michaelpg wrote: > This is confusing: asserting visibility has a side effect of stamping the advanced page if it isn't stamped yet. Good catch! We should be querying the DOM directly for #advancedPage like before, to not have the assertion itself stamping the advanced page div.
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; On 2017/02/01 07:53:50, michaelpg wrote: > is it really worth creating a variable for this.parentNode? 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). https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:65: this.child_._templateInstance[prop] = value; On 2017/02/01 07:53:50, michaelpg wrote: > 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)? 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); https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_idle_render_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_idle_render_browsertest.js:56: test('not stamped before idle or get()', function(done) { On 2017/02/01 07:53:50, michaelpg wrote: > this test is testing 'stamped after idle', not 'not stamped before idle or > get()'. can you add an assert before the requestIdleCallback? That's done on line 46. For each 'test' the 'setup' executes just before hand. So line 46 happens just before line 57. done. https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_main_test.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_main_test.js:179: expectedAdvanced, page.$.advancedPageTemplate.get().style.display); On 2017/02/01 07:53:50, michaelpg wrote: > This is confusing: asserting visibility has a side effect of stamping the > advanced page if it isn't stamped yet. The .get only expands the idle render template. Whether the advanced page is hidden is determined by a div and not the template. The idle render template is _not_ for visibility use. https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_main_test.js:179: expectedAdvanced, page.$.advancedPageTemplate.get().style.display); On 2017/02/01 18:10:37, dpapad wrote: > On 2017/02/01 at 07:53:50, michaelpg wrote: > > This is confusing: asserting visibility has a side effect of stamping the > advanced page if it isn't stamped yet. > > Good catch! We should be querying the DOM directly for #advancedPage like > before, to not have the assertion itself stamping the advanced page div. I can see where using templates for dom-if would make this confusing, but this is different from a dom-if. This template is not controlling visibility (it's not a dom-if). I believe the usage is correct. The display style is controlled by the div within the template (that's what is being tested). https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:70: return page; On 2017/02/01 07:53:50, michaelpg wrote: > add a comment that getting the basic page force-renders the advanced page > > (why are these test-behavioral changes needed if the current advanced page is > already wrapped in a dom-if?) Whether the contents are hidden or not is controlled within the settings-idle-render. This get() dose not change the hidden attribute on the contents.
tsergeant@chromium.org changed reviewers: + tsergeant@chromium.org
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:65: this.child_._templateInstance[prop] = value; On 2017/02/01 21:52:31, dschuyler wrote: > On 2017/02/01 07:53:50, michaelpg wrote: > > 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)? > > 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); 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.
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; On 2017/02/01 21:52:30, dschuyler wrote: > On 2017/02/01 07:53:50, michaelpg wrote: > > is it really worth creating a variable for this.parentNode? > > 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). 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. https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:65: this.child_._templateInstance[prop] = value; On 2017/02/01 22:14:59, tsergeant wrote: > On 2017/02/01 21:52:31, dschuyler wrote: > > On 2017/02/01 07:53:50, michaelpg wrote: > > > 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)? > > > > 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); > > 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. SGTM, thanks for the info. https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_idle_render_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_idle_render_browsertest.js:56: test('not stamped before idle or get()', function(done) { On 2017/02/01 21:52:31, dschuyler wrote: > On 2017/02/01 07:53:50, michaelpg wrote: > > this test is testing 'stamped after idle', not 'not stamped before idle or > > get()'. can you add an assert before the requestIdleCallback? > > That's done on line 46. For each 'test' the 'setup' executes just before hand. > So line 46 happens just before line 57. > > done. ah, you're right. no assert needed, just change the name. https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_main_test.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_main_test.js:179: expectedAdvanced, page.$.advancedPageTemplate.get().style.display); On 2017/02/01 21:52:31, dschuyler wrote: > On 2017/02/01 07:53:50, michaelpg wrote: > > This is confusing: asserting visibility has a side effect of stamping the > > advanced page if it isn't stamped yet. > > The .get only expands the idle render template. Whether the advanced page is > hidden is determined by a div and not the template. The idle render template is > _not_ for visibility use. The existing function assumes <div id="advancedPage"> exists, and will throw an error if page.$$('#advancedPage') returns null on line 179. Tests using that function will fail if showAdvancedPage_() returns false. That may not have been intentional. (If it was, we should outright assert that the advanced page is stamped.) But it's a check that is being relaxed by the new function. https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:70: return page; On 2017/02/01 21:52:31, dschuyler wrote: > On 2017/02/01 07:53:50, michaelpg wrote: > > add a comment that getting the basic page force-renders the advanced page > > > > (why are these test-behavioral changes needed if the current advanced page is > > already wrapped in a dom-if?) > > Whether the contents are hidden or not is controlled within the > settings-idle-render. This get() dose not change the hidden attribute on the > contents. The contents might not exist before get(). So getPage can now create the Advanced page by triggering settings-idle-render to render, which AFAICT is entirely new behavior by this function. Not concerned whether it's hidden. Take testSubPages/testSubPage in settings_subpage_browsertest.js as an example. Previously we would test each Basic section without the Advanced sections even existing. Now they will exist (but be hidden maybe) which no longer reflects how the page actually loads.
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:4: <link rel="import" href="/controls/settings_idle_render.html"> please rebase https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; On 2017/02/02 00:00:35, michaelpg wrote: > On 2017/02/01 21:52:30, dschuyler wrote: > > On 2017/02/01 07:53:50, michaelpg wrote: > > > is it really worth creating a variable for this.parentNode? > > > > 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). > > 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. i don't think it matters either way https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; 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?
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.js:105: query, assert(this.$.advancedPageTemplate.get()))); could this assert() fail?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:4: <link rel="import" href="/controls/settings_idle_render.html"> On 2017/02/02 00:19:36, Dan Beam (slow) wrote: > please rebase Done. https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.js:105: query, assert(this.$.advancedPageTemplate.get()))); On 2017/02/02 01:00:50, Dan Beam wrote: > could this assert() fail? My expectation is that it will fail or succeed in the same conditions as it did before this CL. So I believe it would fail (before this CL and with this CL) if the basic page dom-if template were not expanded. The failures should line up with common uses of assert, meaning that if it fails it's because something is wrong and should be fixed (it's helping detect an error). I'm not seeing where it could cause a false positive. https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; On 2017/02/02 00:19:36, Dan Beam (slow) wrote: > On 2017/02/02 00:00:35, michaelpg wrote: > > On 2017/02/01 21:52:30, dschuyler wrote: > > > On 2017/02/01 07:53:50, michaelpg wrote: > > > > is it really worth creating a variable for this.parentNode? > > > > > > 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). > > > > 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. > > i don't think it matters either way 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. https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; On 2017/02/02 00:19:36, Dan Beam (slow) wrote: > waiiiiit, why do you have to check parentNode? 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. > does this mean if you call .get() when this is detached we return undefined? 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. > would that cause your assert() to fail? I made a reply in the other comment where the assert is located. https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_idle_render_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_idle_render_browsertest.js:56: test('not stamped before idle or get()', function(done) { On 2017/02/02 00:00:35, michaelpg wrote: > On 2017/02/01 21:52:31, dschuyler wrote: > > On 2017/02/01 07:53:50, michaelpg wrote: > > > this test is testing 'stamped after idle', not 'not stamped before idle or > > > get()'. can you add an assert before the requestIdleCallback? > > > > That's done on line 46. For each 'test' the 'setup' executes just before hand. > > So line 46 happens just before line 57. > > > > done. > > ah, you're right. no assert needed, just change the name. Done. https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_main_test.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_main_test.js:179: expectedAdvanced, page.$.advancedPageTemplate.get().style.display); On 2017/02/02 00:00:35, michaelpg wrote: > On 2017/02/01 21:52:31, dschuyler wrote: > > On 2017/02/01 07:53:50, michaelpg wrote: > > > This is confusing: asserting visibility has a side effect of stamping the > > > advanced page if it isn't stamped yet. > > > > The .get only expands the idle render template. Whether the advanced page is > > hidden is determined by a div and not the template. The idle render template > is > > _not_ for visibility use. > > The existing function assumes <div id="advancedPage"> exists, and will throw an > error if page.$$('#advancedPage') returns null on line 179. Tests using that > function will fail if showAdvancedPage_() returns false. > > That may not have been intentional. (If it was, we should outright assert that > the advanced page is stamped.) But it's a check that is being relaxed by the new > function. I agree that if that test were desired it should be tested explicitly. (Or a comment could be made if the unexpected side effect were being relied on). https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:70: return page; On 2017/02/02 00:00:35, michaelpg wrote: > On 2017/02/01 21:52:31, dschuyler wrote: > > On 2017/02/01 07:53:50, michaelpg wrote: > > > add a comment that getting the basic page force-renders the advanced page > > > > > > (why are these test-behavioral changes needed if the current advanced page > is > > > already wrapped in a dom-if?) > > > > Whether the contents are hidden or not is controlled within the > > settings-idle-render. This get() dose not change the hidden attribute on the > > contents. > > The contents might not exist before get(). So getPage can now create the > Advanced page by triggering settings-idle-render to render, which AFAICT is > entirely new behavior by this function. Not concerned whether it's hidden. > > Take testSubPages/testSubPage in settings_subpage_browsertest.js as an example. > Previously we would test each Basic section without the Advanced sections even > existing. Now they will exist (but be hidden maybe) which no longer reflects how > the page actually loads. testSubPages/testSubPage appear to be measuring the performance rather than testing the subpages (maybe it could be timeSubPages instead?) The idle time expansion will expand the dom that holds the subpages. It doesn't actually expand the subpages. IMO, the testSubPages/testSubPage remain as valid with this CL (or without it). If there were an unexpected change in the timing results, they could be acknowledged/accounted for. Aside from the explanation above, it's worth considering that this will be the way pages are loaded after this CL lands. So it will reflect the way pages are loaded at that time.
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.js:105: query, assert(this.$.advancedPageTemplate.get()))); On 2017/02/02 at 02:28:15, dschuyler wrote: > On 2017/02/02 01:00:50, Dan Beam wrote: > > could this assert() fail? > > My expectation is that it will fail or succeed in the same conditions as it did before this CL. So I believe it would fail (before this CL and with this CL) if the basic page dom-if template were not expanded. The failures should line up with common uses of assert, meaning that if it fails it's because something is wrong and should be fixed (it's helping detect an error). I'm not seeing where it could cause a false positive. My understanding is that this assert is a left-over since before the advanced page was behind a cr-lazy-render (used a plain dom-if instead), and it existed to make the compiler happy. As it stands now, I don't think this assert can ever fail, nor is it needed to satisfy the compiler anymore.
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; On 2017/02/02 02:28:15, dschuyler wrote: > On 2017/02/02 00:19:36, Dan Beam (slow) wrote: > > waiiiiit, why do you have to check parentNode? > 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. > > does this mean if you call .get() when this is detached we return undefined? > 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. > > would that cause your assert() to fail? > I made a reply in the other comment where the assert is located. 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?
removing myself as reviewer since you have enough reviewers, and my questions have been answered/acknowledged (thanks!)
michaelpg@chromium.org changed reviewers: - michaelpg@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_idle_render.js (right): https://codereview.chromium.org/2660383002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_idle_render.js:51: var parentNode = this.parentNode; On 2017/02/02 03:56:17, Dan Beam (slow) wrote: > On 2017/02/02 02:28:15, dschuyler wrote: > > On 2017/02/02 00:19:36, Dan Beam (slow) wrote: > > > waiiiiit, why do you have to check parentNode? > > 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. > > > does this mean if you call .get() when this is detached we return undefined? > > > 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. > > > would that cause your assert() to fail? > > I made a reply in the other comment where the assert is located. > > 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? 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.
tsergeant@chromium.org changed reviewers: - tsergeant@chromium.org
(also -me as reviewer, since that was just rietveld doing it's thing)
lgtm w/nit https://codereview.chromium.org/2660383002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_idle_render_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_idle_render_browsertest.js:17: __proto__: testing.Test.prototype, why is this in its own file? instead of being a settings test?
https://codereview.chromium.org/2660383002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_idle_render_browsertest.js (right): https://codereview.chromium.org/2660383002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_idle_render_browsertest.js:17: __proto__: testing.Test.prototype, On 2017/02/07 07:21:33, Dan Beam wrote: > why is this in its own file? instead of being a settings test? I thought it was the right thing to do. There's likely a nuance of the test styles that I'm missing. Let's chat further about how this can be improved and I'll do that in a follow-up CL. Since this has an LGTM and there is a unit test - I'm interpreting this as being 'okay, but could be improved'.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2660383002/#ps100001 (title: "merge with master; changed test name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486497276415360,
"parent_rev": "28c43220bd065a8bd35275886a61764a4add6cd3", "commit_rev":
"f7247aef78ab5a3aa4cc8d61431893bdb6b4e68f"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] idle load basic and advanced pages This CL uses lazy loading templates to preload the advanced page while the basic page is idle. The result is that the advanced page toggles much faster (addressing the 'jank' mentioned in the bug). This CL is replacing CL 2652443002. BUG=681238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] idle load basic and advanced pages This CL uses lazy loading templates to preload the advanced page while the basic page is idle. The result is that the advanced page toggles much faster (addressing the 'jank' mentioned in the bug). This CL is replacing CL 2652443002. BUG=681238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2660383002 Cr-Commit-Position: refs/heads/master@{#448757} Committed: https://chromium.googlesource.com/chromium/src/+/f7247aef78ab5a3aa4cc8d614318... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/f7247aef78ab5a3aa4cc8d614318... |
