Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1477773003:
Use dom-if to hide settings pages and show explicitly. (Closed)
|
Created:
5 years ago by stevenjb Modified:
5 years ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org, esprehn, sorvell, kschaaf_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse dom-if to hide settings pages and show explicitly.
This CL:
* Wraps each settings subpage with a dom-if
* Adds a new SettingsPageVisibility behavior that allows tests
to control page visibility.
* NOTE: This changes the page load order (subpages are all loaded
after the main pages and their dependencies). This has the side
benefit of making Prefs more likely to load before the pages do.
* Fixes an inconsistency with the naming of the OnStartup section
NOTE: This does not impact i18n_template.js since that is loaded after <cr-settings> in settings.html, however if we enable a subpage after loading (e.g. in a test) then i18n_template.js will not be processed and would need to be run explicitly to include translations.
BUG=560432
R=dbeam@chromium.org, michaelpg@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/004444d1a5d12d477446abda2376648d5701551c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Re-upload PS2 #Patch Set 3 : Feedback #
Total comments: 16
Patch Set 4 : Feedback #Patch Set 5 : dom-if settings-sections, use SettingsHideAllPagesForTest #Patch Set 6 : . #
Total comments: 13
Patch Set 7 : Rebase without onStartup change #Patch Set 8 : Feedback #Patch Set 9 : Rebase #Patch Set 10 : restamp #Patch Set 11 : Rebase #Messages
Total messages: 40 (11 generated)
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org, michaelpg@chromium.org, tommycli@chromium.org
Consider this a proposal. With these changes I have been able to write tests that time just the loading of a single subpage by initially leaving each subpage dom-if'd out and adding and removing them one at a time. LMKWYT
On 2015/11/25 19:17:07, stevenjb wrote: > Consider this a proposal. > > With these changes I have been able to write tests that time just the loading of > a single subpage by initially leaving each subpage dom-if'd out and adding and > removing them one at a time. > > LMKWYT So this is just for testing, not for production? (because we can't just hide arbitrary sections on load, other than advanced vs. basic and sub-pages.) How do the <dom-if>s affect page load in "production" (when not testing)? I'm looking into a production version of this, so it's good to get a sense of what you need for tests so we don't duplicate work or have two solutions for the same thing.
Description was changed from ========== Use dom-if to hide settings pages and show explicitly. This CL: * Wraps each settings subpage with a dom-if * Adds a new SettingsPageVisibility behavior that enables pages explicitly. * By default all pages specified will be shown, unless a global controlling visibility is set (e.g. in a test). * This changes the page load order (subpages are all loaded after the main pages and their dependencies) with the benefit of making Prefs more likely to load before the pages do. * Fixes an inconsistency with the naming of the 0nStartup section * Fixes a bug in the settings-checkbox logic revealed with the new page load ordering. BUG=560432 ========== to ========== Use dom-if to hide settings pages and show explicitly. This CL: * Wraps each settings subpage with a dom-if * Adds a new SettingsPageVisibility behavior that enables pages explicitly. * By default all pages specified will be shown, unless a global controlling visibility is set (e.g. in a test). * This changes the page load order (subpages are all loaded after the main pages and their dependencies) with the benefit of making Prefs more likely to load before the pages do. * Fixes an inconsistency with the naming of the 0nStartup section * Fixes a bug in the settings-checkbox logic revealed with the new page load ordering. NOTE: This does not impact i18n_template.js since that is loaded after <cr-settings> in settings.html, however if we enable a subpage after loading (e.g. in a test) then i18n_template.js will not be processed and would need to be run explicitly to include translations. BUG=560432 ==========
On 2015/11/25 19:23:14, michaelpg wrote: > On 2015/11/25 19:17:07, stevenjb wrote: > > Consider this a proposal. > > > > With these changes I have been able to write tests that time just the loading > of > > a single subpage by initially leaving each subpage dom-if'd out and adding and > > removing them one at a time. > > > > LMKWYT > > So this is just for testing, not for production? (because we can't just hide > arbitrary sections on load, other than advanced vs. basic and sub-pages.) > > How do the <dom-if>s affect page load in "production" (when not testing)? > > I'm looking into a production version of this, so it's good to get a sense of > what you need for tests so we don't duplicate work or have two solutions for the > same thing. Currently this is just for testing, but it also gives us an idea of how we might dom-if out sub-sub-pages initially, or even the entire 'advanced' section, and what artifacts there might be. Also: A note was added to the description regarding i18n, which is not a problem with this CL, but will be a problem if we use this mechanism to initially exclude sections and show them after loading. Adding these dom-if's has no measurable impact on the Settings page load times. If you would prefer that we wait for a production version I am happy to hold off on this, but I do find the individual page timings useful.
https://codereview.chromium.org/1477773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/1477773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/basic_page/basic_page.js:44: this.setVisiblePages([ why do we need this? why can't the behavior calculate this from currentRoute/the URL?
https://codereview.chromium.org/1477773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/1477773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/basic_page/basic_page.js:44: this.setVisiblePages([ On 2015/11/30 19:22:43, Dan Beam wrote: > why do we need this? why can't the behavior calculate this from > currentRoute/the URL? Sorry, I don't understand your comment. The intention is: * When chrome://md-settings is initially loaded, no subpages (i.e. sections/cards) are created (dom-if condition is false). * When ready() gets called, we create each subpage, unless a test has already explicitly set which pages are visible. The 'route' at this point is just chrome://md-settings.
Patchset #2 (id:20001) has been deleted
Dan, is this along the lines of what you were suggesting? Keeping but hiding each <settings-section> and iterating through them to create them?
https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_checkbox.js (right): https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_checkbox.js:92: var newvalue = this.getNewValue_(this.checked); newValue https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_checkbox.js:95: this.set('pref.value', newvalue); ^ what's going on here? https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_page_visibility.js (right): https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:48: showDefaultSections() { nit: i'm hesitant to use new ES features without seeing a clear benefit, i.e. showDefaultSections: function() { also works https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:52: var sections = this.shadowRoot.querySelectorAll('settings-section'); in an ideal world, we'd know which is the current section, and the sections would work like a DAG or at least a graph (if there are cycles). meaning you'd have TreeNode - Array<TreeNode> parent - Array<TreeNode> children and we could traverse from the currently active page upward through the ancestry (current = current.parent), asking if its siblings are visible and compute the visible set this way. maybe the DOM itself would be good enough for this, but I'd prefer a format that's agnostic to visual representation. does that make sense? https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:55: pages.push(sections[i].section); arguable nit: webui mainly uses curlies for 1-liner for loops https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:103: section: 'onStartup', what's going on here? https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:19: remove \n
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Dan, huge apologies, I accidentally deleted the patchset with your comments (and my replies). I'll reply inline. On 2015/12/01 04:36:14, Dan Beam wrote: > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/controls/settings_checkbox.js (right): > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/controls/settings_checkbox.js:92: var newvalue > = this.getNewValue_(this.checked); > newValue Done (in separate CL) > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/controls/settings_checkbox.js:95: > this.set('pref.value', newvalue); > ^ what's going on here? I ran into this bug while testing. I've moved it to a separate CL and added comments. > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_page/settings_page_visibility.js > (right): > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/settings_page_visibility.js:48: > showDefaultSections() { > nit: i'm hesitant to use new ES features without seeing a clear benefit, i.e. > > showDefaultSections: function() { > > also works > That was unintentional, just my C++ nature unexpectedly working. Fixed. > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/settings_page_visibility.js:52: > var sections = this.shadowRoot.querySelectorAll('settings-section'); > in an ideal world, we'd know which is the current section, and the sections > would work like a DAG or at least a graph (if there are cycles). meaning you'd > have > > TreeNode > - Array<TreeNode> parent > - Array<TreeNode> children > > and we could traverse from the currently active page upward through the ancestry > (current = current.parent), asking if its siblings are visible and compute the > visible set this way. maybe the DOM itself would be good enough for this, but > I'd prefer a format that's agnostic to visual representation. > > does that make sense? > Kind of? Not really... I'm not sure what your objective is. This CL is just intended to be a starting point for testing individual sections. It is effectively tied to the DOM, since this is explicitly including DOM nodes (the sections). > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/settings_page_visibility.js:55: > pages.push(sections[i].section); > arguable nit: webui mainly uses curlies for 1-liner for loops > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_page/settings_router.js (right): > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/settings_router.js:103: section: > 'onStartup', > what's going on here? The naming of this section was inconsistent with the others (e.g. defaultBrowser) and with its title (which we use for routing). It seemed easier/better to make it consistent than special-case it in the tests. > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/site_settings/site_settings_behavior.js > (right): > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/site_settings/site_settings_behavior.js:19: > remove \n Done.
PTAL
On 2015/12/01 04:36:14, Dan Beam wrote: > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/controls/settings_checkbox.js (right): > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/controls/settings_checkbox.js:92: var newvalue > = this.getNewValue_(this.checked); > newValue > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/controls/settings_checkbox.js:95: > this.set('pref.value', newvalue); > ^ what's going on here? > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_page/settings_page_visibility.js > (right): > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/settings_page_visibility.js:48: > showDefaultSections() { > nit: i'm hesitant to use new ES features without seeing a clear benefit, i.e. > > showDefaultSections: function() { > > also works > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/settings_page_visibility.js:52: > var sections = this.shadowRoot.querySelectorAll('settings-section'); > in an ideal world, we'd know which is the current section, and the sections > would work like a DAG or at least a graph (if there are cycles). meaning you'd > have > > TreeNode > - Array<TreeNode> parent > - Array<TreeNode> children > > and we could traverse from the currently active page upward through the ancestry > (current = current.parent), asking if its siblings are visible and compute the > visible set this way. maybe the DOM itself would be good enough for this, but > I'd prefer a format that's agnostic to visual representation. > > does that make sense? > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/settings_page_visibility.js:55: > pages.push(sections[i].section); > arguable nit: webui mainly uses curlies for 1-liner for loops [citation needed] or more constructively: is the guidance really to mainly use curlies for for loops, and no curlies for 1-liner if/else? > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_page/settings_router.js (right): > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/settings_page/settings_router.js:103: section: > 'onStartup', > what's going on here? > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/site_settings/site_settings_behavior.js > (right): > > https://codereview.chromium.org/1477773003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/site_settings/site_settings_behavior.js:19: > remove \n
https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:25: <template is="dom-if" if="[[pageVisible.dateTime]]"> why can't we include the <settings-section> itself in the dom-if? https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/compiled_resources.gyp (right): https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/compiled_resources.gyp:17: 'settings_pages/compiled_resources.gyp:*', settings_pages -> settings_page https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_page_visibility.js (right): https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:7: * Behavior common to all Settings pages nit: period. and more accurate comment https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:12: * @group Chrome UI Behavior do we ever plan to use @group for anything? https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:24: /** Dictionary of pages to hide. Includes 'basic' and 'advanced'. */ Can you provide a more specific /** @type */? Isn't this more like "dictionary of pages to show"? Lastly... this doesn't include 'basic' or 'advanced'. https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:37: var visible = this.pageVisible || {}; since setVisiblePages([]) is a no-op (it doesn't hide pages), maybe rename this, e.g. setPagesVisible? "set visible pages" --> set the visible pages to these ones "set pages visible"--> make these pages visible https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:41: this.set('pageVisible', visible); this.pageVisible = visible https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:17: value: settings.ContentSettingsTypes.GEOLOCATION No reason to make geolocation the default category for all elements with SiteSettingsBehaviorImpl. The <site-settings-category> in the "location" section should have its category attribute set to geolocation, if not in advanced_page.js, then in advanced_page.html.
PTAL https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:25: <template is="dom-if" if="[[pageVisible.dateTime]]"> On 2015/12/02 19:22:51, michaelpg wrote: > why can't we include the <settings-section> itself in the dom-if? We can, but then we have to explicitly add each section somewhwere, see SettingsPageVisibility.showDefaultSections() https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/compiled_resources.gyp (right): https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/compiled_resources.gyp:17: 'settings_pages/compiled_resources.gyp:*', On 2015/12/02 19:22:52, michaelpg wrote: > settings_pages -> settings_page Oops, done. (Must have missed this with the current closure failure noise...) https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_page_visibility.js (right): https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:7: * Behavior common to all Settings pages On 2015/12/02 19:22:52, michaelpg wrote: > nit: period. and more accurate comment Done. https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:12: * @group Chrome UI Behavior On 2015/12/02 19:22:52, michaelpg wrote: > do we ever plan to use @group for anything? Dunno, but we have it everywhere. https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:24: /** Dictionary of pages to hide. Includes 'basic' and 'advanced'. */ On 2015/12/02 19:22:52, michaelpg wrote: > Can you provide a more specific /** @type */? > > Isn't this more like "dictionary of pages to show"? > > Lastly... this doesn't include 'basic' or 'advanced'. Opps. It did originally but then I decided that was confusing. Fixed. As for type, looks like I can use Object.<string, boolean> - I hadn't run into that before. https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:37: var visible = this.pageVisible || {}; On 2015/12/02 19:22:52, michaelpg wrote: > since setVisiblePages([]) is a no-op (it doesn't hide pages), maybe rename this, > e.g. setPagesVisible? > > "set visible pages" --> set the visible pages to these ones > "set pages visible"--> make these pages visible That kinda sorta almost makes sense to me :) Done. https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:41: this.set('pageVisible', visible); On 2015/12/02 19:22:52, michaelpg wrote: > this.pageVisible = visible Done. https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1477773003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:17: value: settings.ContentSettingsTypes.GEOLOCATION On 2015/12/02 19:22:52, michaelpg wrote: > No reason to make geolocation the default category for all elements with > SiteSettingsBehaviorImpl. The <site-settings-category> in the "location" section > should have its category attribute set to geolocation, if not in > advanced_page.js, then in advanced_page.html. Done.
so in talking with stevenjb@ on chat, there were a few things we realized:
1) I thought we wanted to make a mechanism to tie navigation and dom-if'ing
together for performance benefits *now*. That's not the case. We just want a
way of isolating pages.
2) After I realized we weren't doing 1), I felt this adds a lot of "cruft" for
more-or-less test-only purposes.
3) Steven thought I had an issue with only dom-if around the sections, which is
why he added [hidden] bindings. I don't see a need for both, and it certainly
doesn't seem clearer or necessary to me (and likely slows things down just a
little).
So I think it'd be the best if we could get (from this CL):
* something that serves the original objective (isolation) in the cleanest and
cheapest way possible
* something that could eventually be used to not render invisible pages (i.e. so
this code isn't really "test-only")
It seemed like the easiest way (in our minds) was something along the lines of:
<template is="dom-if" if="pagesVisible.thisSection">
<settings-section id="thisSection">...</settings-section>
</template>
Polymer({
// maybe is: 'settings-main', ?
properties: {
pagesVisible: {
thisSection: !window.hidePagesForTesting,
...
},
},
});
NOTE: as discussed here and in the chat, it's likely that the templates will
still be rendered if pagesVisible.thisSection starts as true by default (i.e.
and was just change to false for testing).
The only way they might not render then hide (the typical dom-if behavior if you
change the conditional after a template has been stamped), is if we can run
JavaScript correctly before templates are asynchronously rendered (but that
depends on implementation details of the templating that maybe change?) or just
start them as false with a global :(. I don't see a better, more scoped,
alternative yet.
Maybe sorvell@ or kschaaf@ know how to do this.
Dan, that looks good to me. I'm not sure if it's true that the templates will be stamped if the "if" is true by default. The templates are stamped asynchronously (at microtask timing), so if the test can negate the test condition synchronously, I would expect that to prevent the templates from being stamped at all. Although, for all I know the test case's code is not called immediately after the page's script executes -- is it called after a timeout or something? On Thu, Dec 3, 2015 at 11:46 AM <dbeam@chromium.org> wrote: > so in talking with stevenjb@ on chat, there were a few things we realized: > > 1) I thought we wanted to make a mechanism to tie navigation and dom-if'ing > together for performance benefits *now*. That's not the case. We just > want a > way of isolating pages. > > 2) After I realized we weren't doing 1), I felt this adds a lot of "cruft" > for > more-or-less test-only purposes. > > 3) Steven thought I had an issue with only dom-if around the sections, > which is > why he added [hidden] bindings. I don't see a need for both, and it > certainly > doesn't seem clearer or necessary to me (and likely slows things down just > a > little). > > So I think it'd be the best if we could get (from this CL): > * something that serves the original objective (isolation) in the cleanest > and > cheapest way possible > * something that could eventually be used to not render invisible pages > (i.e. so > this code isn't really "test-only") > > It seemed like the easiest way (in our minds) was something along the lines > of: > > <template is="dom-if" if="pagesVisible.thisSection"> > <settings-section id="thisSection">...</settings-section> > </template> > > Polymer({ > // maybe is: 'settings-main', ? > properties: { > pagesVisible: { > thisSection: !window.hidePagesForTesting, > ... > }, > }, > }); > > NOTE: as discussed here and in the chat, it's likely that the templates > will > still be rendered if pagesVisible.thisSection starts as true by default > (i.e. > and was just change to false for testing). > > The only way they might not render then hide (the typical dom-if behavior > if you > change the conditional after a template has been stamped), is if we can run > JavaScript correctly before templates are asynchronously rendered (but that > depends on implementation details of the templating that maybe change?) or > just > start them as false with a global :(. I don't see a better, more scoped, > alternative yet. > > Maybe sorvell@ or kschaaf@ know how to do this. > > https://codereview.chromium.org/1477773003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
"chromium-reviews" was me btw.
On 2015/12/03 20:05:57, chromium-reviews wrote:
> Dan, that looks good to me.
>
> I'm not sure if it's true that the templates will be stamped if the "if" is
> true by default. The templates are stamped asynchronously (at microtask
> timing), so if the test can negate the test condition synchronously, I
> would expect that to prevent the templates from being stamped at all.
>
> Although, for all I know the test case's code is not called immediately
> after the page's script executes -- is it called after a timeout or
> something?
>
> On Thu, Dec 3, 2015 at 11:46 AM <mailto:dbeam@chromium.org> wrote:
>
> > so in talking with stevenjb@ on chat, there were a few things we realized:
> >
> > 1) I thought we wanted to make a mechanism to tie navigation and dom-if'ing
> > together for performance benefits *now*. That's not the case. We just
> > want a
> > way of isolating pages.
> >
> > 2) After I realized we weren't doing 1), I felt this adds a lot of "cruft"
> > for
> > more-or-less test-only purposes.
> >
> > 3) Steven thought I had an issue with only dom-if around the sections,
> > which is
> > why he added [hidden] bindings. I don't see a need for both, and it
> > certainly
> > doesn't seem clearer or necessary to me (and likely slows things down just
> > a
> > little).
> >
> > So I think it'd be the best if we could get (from this CL):
> > * something that serves the original objective (isolation) in the cleanest
> > and
> > cheapest way possible
> > * something that could eventually be used to not render invisible pages
> > (i.e. so
> > this code isn't really "test-only")
> >
> > It seemed like the easiest way (in our minds) was something along the lines
> > of:
> >
> > <template is="dom-if" if="pagesVisible.thisSection">
> > <settings-section id="thisSection">...</settings-section>
> > </template>
> >
> > Polymer({
> > // maybe is: 'settings-main', ?
> > properties: {
> > pagesVisible: {
> > thisSection: !window.hidePagesForTesting,
> > ...
> > },
> > },
> > });
> >
> > NOTE: as discussed here and in the chat, it's likely that the templates
> > will
> > still be rendered if pagesVisible.thisSection starts as true by default
> > (i.e.
> > and was just change to false for testing).
> >
> > The only way they might not render then hide (the typical dom-if behavior
> > if you
> > change the conditional after a template has been stamped), is if we can run
> > JavaScript correctly before templates are asynchronously rendered (but that
> > depends on implementation details of the templating that maybe change?) or
> > just
> > start them as false with a global :(. I don't see a better, more scoped,
> > alternative yet.
> >
> > Maybe sorvell@ or kschaaf@ know how to do this.
> >
> > https://codereview.chromium.org/1477773003/
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
On Thu, Dec 3, 2015 at 1:07 PM, <michaelpg@chromium.org> wrote: > "chromium-reviews" was me btw. > > On 2015/12/03 20:05:57, chromium-reviews wrote: > >> Dan, that looks good to me. >> > > I'm not sure if it's true that the templates will be stamped if the "if" is >> true by default. The templates are stamped asynchronously (at microtask >> timing), so if the test can negate the test condition synchronously, I >> would expect that to prevent the templates from being stamped at all. >> > > Although, for all I know the test case's code is not called immediately >> after the page's script executes -- is it called after a timeout or >> something? >> > >> The problem is that we can't use any properties of Polymer objects (e.g. SettingsBasicPage) because they will not be loaded until the DOM is, at which point the pages will already be stamped unless they default to false somehow. I may be able to provide a 'pagesVisibleByDefault' property that defaults to undefined and gets set in ready() unless defined, but that will only work if there is a test hook that occurs after the DOM is loaded and before ready() (or maybe attached()?) gets called. I will experiment. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/07 19:54:22, stevenjb wrote: > On Thu, Dec 3, 2015 at 1:07 PM, <mailto:michaelpg@chromium.org> wrote: > > > "chromium-reviews" was me btw. > > > > On 2015/12/03 20:05:57, chromium-reviews wrote: > > > >> Dan, that looks good to me. > >> > > > > I'm not sure if it's true that the templates will be stamped if the "if" is > >> true by default. The templates are stamped asynchronously (at microtask > >> timing), so if the test can negate the test condition synchronously, I > >> would expect that to prevent the templates from being stamped at all. > >> > > > > Although, for all I know the test case's code is not called immediately > >> after the page's script executes -- is it called after a timeout or > >> something? > >> > > > >> > The problem is that we can't use any properties of Polymer objects (e.g. > SettingsBasicPage) because they will not be loaded until the DOM is, at > which point the pages will already be stamped unless they default to false > somehow. I may be able to provide a 'pagesVisibleByDefault' property that > defaults to undefined and gets set in ready() unless defined, but that will > only work if there is a test hook that occurs after the DOM is loaded and > before ready() (or maybe attached()?) gets called. I will experiment. > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Can you provide a fiddle of this problem? Declared properties are set after the created callback, before the ready callback. I would expect the "if" condition to propagate to whatever is providing the value, before asynchronous tasks like stamping templates get triggered.
Interesting. I'll try some tests. On Mon, Dec 7, 2015 at 1:23 PM, <michaelpg@chromium.org> wrote: > On 2015/12/07 19:54:22, stevenjb wrote: > >> On Thu, Dec 3, 2015 at 1:07 PM, <mailto:michaelpg@chromium.org> wrote: >> > > > "chromium-reviews" was me btw. >> > >> > On 2015/12/03 20:05:57, chromium-reviews wrote: >> > >> >> Dan, that looks good to me. >> >> >> > >> > I'm not sure if it's true that the templates will be stamped if the >> "if" is >> >> true by default. The templates are stamped asynchronously (at microtask >> >> timing), so if the test can negate the test condition synchronously, I >> >> would expect that to prevent the templates from being stamped at all. >> >> >> > >> > Although, for all I know the test case's code is not called immediately >> >> after the page's script executes -- is it called after a timeout or >> >> something? >> >> >> > >> >> >> The problem is that we can't use any properties of Polymer objects (e.g. >> SettingsBasicPage) because they will not be loaded until the DOM is, at >> which point the pages will already be stamped unless they default to false >> somehow. I may be able to provide a 'pagesVisibleByDefault' property that >> defaults to undefined and gets set in ready() unless defined, but that >> will >> only work if there is a test hook that occurs after the DOM is loaded and >> before ready() (or maybe attached()?) gets called. I will experiment. >> > > -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > Can you provide a fiddle of this problem? > > Declared properties are set after the created callback, before the ready > callback. I would expect the "if" condition to propagate to whatever is > providing the value, before asynchronous tasks like stamping templates get > triggered. > > https://codereview.chromium.org/1477773003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, this latest patcheset is closest to PS1, so you may want to ignore the others and compare the latest to that (or start fresh). As Michael discovered, "document.registerElement seems to *immediately* create any elements in the DOM with that tag". Meaning, there is no way execute JS after SettingsBasicPage is created but before any <template dom-if> content is stamped, unless the dom-if defaults to false. So, we are stuck with using a global, but I've made it so there is just one boolean and appended ForTest like we do in C++ to make its purpose clear. LMKWYT
Description was changed from ========== Use dom-if to hide settings pages and show explicitly. This CL: * Wraps each settings subpage with a dom-if * Adds a new SettingsPageVisibility behavior that enables pages explicitly. * By default all pages specified will be shown, unless a global controlling visibility is set (e.g. in a test). * This changes the page load order (subpages are all loaded after the main pages and their dependencies) with the benefit of making Prefs more likely to load before the pages do. * Fixes an inconsistency with the naming of the 0nStartup section * Fixes a bug in the settings-checkbox logic revealed with the new page load ordering. NOTE: This does not impact i18n_template.js since that is loaded after <cr-settings> in settings.html, however if we enable a subpage after loading (e.g. in a test) then i18n_template.js will not be processed and would need to be run explicitly to include translations. BUG=560432 ========== to ========== Use dom-if to hide settings pages and show explicitly. This CL: * Wraps each settings subpage with a dom-if * Adds a new SettingsPageVisibility behavior that allows tests to control page visibility. * NOTE: This changes the page load order (subpages are all loaded after the main pages and their dependencies). This has the side benefit of making Prefs more likely to load before the pages do. * Fixes an inconsistency with the naming of the OnStartup section NOTE: This does not impact i18n_template.js since that is loaded after <cr-settings> in settings.html, however if we enable a subpage after loading (e.g. in a test) then i18n_template.js will not be processed and would need to be run explicitly to include translations. BUG=560432 ==========
On 2015/12/07 23:54:23, stevenjb wrote: > OK, this latest patcheset is closest to PS1, so you may want to ignore the > others and compare the latest to that (or start fresh). > > As Michael discovered, "document.registerElement seems to *immediately* create > any elements in the DOM with that tag". > > Meaning, there is no way execute JS after SettingsBasicPage is created but > before any <template dom-if> content is stamped, unless the dom-if defaults to > false. dom-if is still asynchronous (microtask timing), so this isn't true strictly speaking. The problem is there's some sort of delay between the end of the synchronous execution of the page under test, and the start of the execution of the test fixture's setUp method. I don't know what this is from, whether it's intended, or whether it's a constant amount of time. But it means our tests run asynchronously some or all of the time, so we can't reliably "intercept" the dom-if. > > So, we are stuck with using a global, but I've made it so there is just one > boolean and appended ForTest like we do in C++ to make its purpose clear. > > LMKWYT lgtm
https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_page_visibility.js (right): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:31: * @type {Object<string, boolean>} nit: just Object<boolean>, keys are always strings https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:36: notify: true, notify: true is unnecessary -- no one is two-way binding to this, right?
lgtm https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/on_startup_page/on_startup_page.html (right): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/on_startup_page/on_startup_page.html:16: section="onStartup"> please try to stop writing these: https://en.wikipedia.org/wiki/Omnibus_bill https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/compiled_resources.gyp (left): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/compiled_resources.gyp:11: 'includes': ['../../../../../third_party/closure_compiler/compile_js2.gypi'], btw, prefer *2 versions when possible https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_page_visibility.js (right): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:23: var SettingsHideAllPagesForTest; nit: settingsHideAllPagesForTest https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:31: * @type {Object<string, boolean>} On 2015/12/08 00:45:58, michaelpg wrote: > nit: just Object<boolean>, keys are always strings +1 (I thought we had a presubmit about this?)
https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/on_startup_page/on_startup_page.html (right): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/on_startup_page/on_startup_page.html:16: section="onStartup"> On 2015/12/08 02:55:27, Dan Beam wrote: > please try to stop writing these: https://en.wikipedia.org/wiki/Omnibus_bill https://codereview.chromium.org/1505113005 https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/compiled_resources.gyp (left): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/compiled_resources.gyp:11: 'includes': ['../../../../../third_party/closure_compiler/compile_js2.gypi'], On 2015/12/08 02:55:27, Dan Beam wrote: > btw, prefer *2 versions when possible This is new to me, please send out a PSA (or direct me to it if I missed it). I would also rather migrate all of resources/settings if possible. Thanks. https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_page_visibility.js (right): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:23: var SettingsHideAllPagesForTest; On 2015/12/08 02:55:27, Dan Beam wrote: > nit: settingsHideAllPagesForTest Done. https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:31: * @type {Object<string, boolean>} On 2015/12/08 02:55:27, Dan Beam wrote: > On 2015/12/08 00:45:58, michaelpg wrote: > > nit: just Object<boolean>, keys are always strings > > +1 (I thought we had a presubmit about this?) Done. (And apparently not, we have 128 instances of Object<string, ...> in the Chrome source FWIW). https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_page_visibility.js:36: notify: true, On 2015/12/08 00:45:58, michaelpg wrote: > notify: true is unnecessary -- no one is two-way binding to this, right? Not currently... and probably not ever. Done.
https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/basic_page/basic_page.html:22: <template is="dom-if" if="[[showPage(pageVisibility.people)]]"> Set the restamp attribute on all of these. It won't affect performance in prod, but it will ensure that the section is removed from the DOM after changing the visibility to false, which will provide more independent numbers in your subpage tests. (Otherwise, subpage tests that run later will be slower due to all of the other subpages co-existing in the DOM.) restamp: https://github.com/Polymer/polymer/blob/bab847aaf706e65b44087d3bb342d4ce80ff3...
https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/1477773003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/basic_page/basic_page.html:22: <template is="dom-if" if="[[showPage(pageVisibility.people)]]"> On 2015/12/08 22:27:08, michaelpg wrote: > Set the restamp attribute on all of these. It won't affect performance in prod, > but it will ensure that the section is removed from the DOM after changing the > visibility to false, which will provide more independent numbers in your subpage > tests. (Otherwise, subpage tests that run later will be slower due to all of the > other subpages co-existing in the DOM.) > > restamp: > https://github.com/Polymer/polymer/blob/bab847aaf706e65b44087d3bb342d4ce80ff3... You know, I did that at one point, but lost it I think in the revert. Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1477773003/#ps240001 (title: "restamp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477773003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477773003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Message was sent while issue was closed.
Description was changed from ========== Use dom-if to hide settings pages and show explicitly. This CL: * Wraps each settings subpage with a dom-if * Adds a new SettingsPageVisibility behavior that allows tests to control page visibility. * NOTE: This changes the page load order (subpages are all loaded after the main pages and their dependencies). This has the side benefit of making Prefs more likely to load before the pages do. * Fixes an inconsistency with the naming of the OnStartup section NOTE: This does not impact i18n_template.js since that is loaded after <cr-settings> in settings.html, however if we enable a subpage after loading (e.g. in a test) then i18n_template.js will not be processed and would need to be run explicitly to include translations. BUG=560432 ========== to ========== Use dom-if to hide settings pages and show explicitly. This CL: * Wraps each settings subpage with a dom-if * Adds a new SettingsPageVisibility behavior that allows tests to control page visibility. * NOTE: This changes the page load order (subpages are all loaded after the main pages and their dependencies). This has the side benefit of making Prefs more likely to load before the pages do. * Fixes an inconsistency with the naming of the OnStartup section NOTE: This does not impact i18n_template.js since that is loaded after <cr-settings> in settings.html, however if we enable a subpage after loading (e.g. in a test) then i18n_template.js will not be processed and would need to be run explicitly to include translations. BUG=560432 R=dbeam@chromium.org, michaelpg@chromium.org Committed: https://crrev.com/004444d1a5d12d477446abda2376648d5701551c Cr-Commit-Position: refs/heads/master@{#364470} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/004444d1a5d12d477446abda2376648d5701551c Cr-Commit-Position: refs/heads/master@{#364470}
Message was sent while issue was closed.
Description was changed from ========== Use dom-if to hide settings pages and show explicitly. This CL: * Wraps each settings subpage with a dom-if * Adds a new SettingsPageVisibility behavior that allows tests to control page visibility. * NOTE: This changes the page load order (subpages are all loaded after the main pages and their dependencies). This has the side benefit of making Prefs more likely to load before the pages do. * Fixes an inconsistency with the naming of the OnStartup section NOTE: This does not impact i18n_template.js since that is loaded after <cr-settings> in settings.html, however if we enable a subpage after loading (e.g. in a test) then i18n_template.js will not be processed and would need to be run explicitly to include translations. BUG=560432 R=dbeam@chromium.org, michaelpg@chromium.org Committed: https://crrev.com/004444d1a5d12d477446abda2376648d5701551c Cr-Commit-Position: refs/heads/master@{#364470} ========== to ========== Use dom-if to hide settings pages and show explicitly. This CL: * Wraps each settings subpage with a dom-if * Adds a new SettingsPageVisibility behavior that allows tests to control page visibility. * NOTE: This changes the page load order (subpages are all loaded after the main pages and their dependencies). This has the side benefit of making Prefs more likely to load before the pages do. * Fixes an inconsistency with the naming of the OnStartup section NOTE: This does not impact i18n_template.js since that is loaded after <cr-settings> in settings.html, however if we enable a subpage after loading (e.g. in a test) then i18n_template.js will not be processed and would need to be run explicitly to include translations. BUG=560432 R=dbeam@chromium.org, michaelpg@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/004444d1a5d12d477446abda2376... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001) manually as 004444d1a5d12d477446abda2376648d5701551c (presubmit successful). |
