Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1422023012:
MD Settings: Simplify settings-dropdown-menu (Closed)
|
Created:
5 years, 1 month ago by michaelpg Modified:
5 years, 1 month ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, dcheng, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Simplify settings-dropdown-menu and add tests.
Uses the new PrefUtil functionality to test selected elements.
BUG=425627
R=dschuyler@chromium.org,stevenjb@chromium.org
Committed: https://crrev.com/5c22310582dfb1d9a2e50401ce31fa6ec57fc0e1
Cr-Commit-Position: refs/heads/master@{#360421}
Patch Set 1 : #
Total comments: 26
Patch Set 2 : rework with PrefUtil #Patch Set 3 : Fix updateSelected #
Total comments: 8
Patch Set 4 : stevenjb feedback + disabled #Patch Set 5 : rebase #Messages
Total messages: 37 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Dave: I thought it would be easier to show than to tell. I'm not sure if I was right, but PTAL. +stevenjb for general review. If this CL won't work for some reason feel free to adapt it. My main point is that if we follow the model of settings-radio-group and use the built-in features of <paper-menu>, we shouldn't need much custom stuff or extra variables. This is in response to my comments at https://codereview.chromium.org/1436453002/diff/1/chrome/browser/resources/se...
https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_page/clear_browsing_data_page.html:18: menu-options="[[clearFromOptions_]]" disabled> Can we remove 'disabled' here? https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (left): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:41: * @type {?PrefObject} Can we type this (correctly)? https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:49: disabled: Boolean, If we actually need this, let's just document that this reflects the HTML element property. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_radio_group.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_radio_group.js:51: /** @private */ @type selected https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/selectable_pref_behavior.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/selectable_pref_behavior.js:15: toPrefValue: function(value) { nit: Even tho we're using Closure to type values, it's still less obvious that |value| needs to be a string, so I think the code will be more readable if we name this stringToPrefValue. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/selectable_pref_behavior.js:17: return undefined; I have a slight preference for passing 'this.pref' to these, since |pref| is not defined in the behavior, but I don't feel too strongly about that. If we do implement this as-is, we should document that any element using this behavior is expected to have a |pref| member of type PrefObject.
Does this CL address similar issues: https://codereview.chromium.org/1420533011/ A good place to test these changes is the Appearance font size menu and page zoom menus. The font size can be made 'custom' by setting the font size slider to an odd value in Customize Fonts. The custom option should become unavailable as soon as a another selection is made. The Page Zoom is an example of needing the menu disabled with the Loading text, since it can take a moment to become available. Please let me know how those fare.
https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/selectable_pref_behavior.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/selectable_pref_behavior.js:17: return undefined; On 2015/11/11 02:04:22, stevenjb wrote: > I have a slight preference for passing 'this.pref' to these, since |pref| is not > defined in the behavior, but I don't feel too strongly about that. If we do > implement this as-is, we should document that any element using this behavior is > expected to have a |pref| member of type PrefObject. I see your point, but then, should something like this be a global ("static") utility rather than a behavior?
https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/selectable_pref_behavior.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/selectable_pref_behavior.js:17: return undefined; On 2015/11/11 20:29:21, michaelpg wrote: > On 2015/11/11 02:04:22, stevenjb wrote: > > I have a slight preference for passing 'this.pref' to these, since |pref| is > not > > defined in the behavior, but I don't feel too strongly about that. If we do > > implement this as-is, we should document that any element using this behavior > is > > expected to have a |pref| member of type PrefObject. > > I see your point, but then, should something like this be a global ("static") > utility rather than a behavior? It certainly could be. In this case maybe that would be better? 'PrefUtil.StringToPrefValue(this.pref, value)'? Unless we plan to use these within data binding (like e.g. we do with i18n()) that might be simpler.
Dave, I've published some comments on https://codereview.chromium.org/1432103002. Please let me know if you'd like to address those there, or take over this CL, or have me move forward with this CL. I don't mind either way as long as we end up with a robust dropdown menu at the end of the day. On 2015/11/11 18:57:11, dschuyler wrote: > Does this CL address similar issues: https://codereview.chromium.org/1420533011/ No. I specifically left the array structure intact in this CL in order to focus attention on: * extracting the pref <--> string behavior into a common place (either a behavior, or maybe better, a "static" utility) * using attrForSelected to simplify the code, so we don't need all 4 of the itemIndex_, itemValue_, onSelectedChanged_, prefChanged_ functions from https://codereview.chromium.org/1432103002/ > > A good place to test these changes is the Appearance > font size menu and page zoom menus. The font size > can be made 'custom' by setting the font size slider > to an odd value in Customize Fonts. works > The custom option > should become unavailable as soon as a another selection > is made. works > The Page Zoom is an example of needing the > menu disabled with the Loading text, since it can take > a moment to become available. Please let me know how > those fare. This will show a "Loading..." label in the dropdown menu itself until the menu options are set. It won't be disabled, but tapping it won't open a dropdown menu because there are no options. If you'd like to actually disable the dropdown menu until it's ready, that'd be cool, but make sure that doesn't override the original "disabled" property set by hosts.
Awesome, thanks for doing those tests. I've put a few questions here. I'd like to get more information on those and see how well I can follow this code before choosing between the two CLs. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.html (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.html:13: on-iron-select="onSelect_" disabled="[[disabled]]"> If we're not disabling the menus while loading, is the disabled necessary? https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:62: * The current selected value, as a string. Is this really the value? Doesn't this get run through a lookup map to get the value? https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:131: var selected = this.menuMap_[this.selected_]; It is confusing that selected_ is translated into selected. Err, well, I happen to already know it's translating an index number into a preference value string, but it's hard to see that with lhs and rhs called selected{_}. Maybe prefString on the lhs? https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:158: return selected == this.notFoundItem_[0]; Why is this using an array element rather than just using '' directly?
Thanks Dave. To clarify, I'm using menuMap_ as a hash map to convert from the stringified values (for the paper-menu) back to the original values (number, string, Object). But we don't need to do that because we already convert to the Pref's type when setting the pref value. So we only need menuMap_ for O(1) membership testing -- like Object<string, boolean> -- to test whether the pref value is in the option list. ^ if i'm not making sense, let me know and I can quickly change the code to work that way. I think it would be an overall simpler CL. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.html (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.html:13: on-iron-select="onSelect_" disabled="[[disabled]]"> On 2015/11/13 00:11:16, dschuyler wrote: > If we're not disabling the menus while loading, is the disabled necessary? I'm exposing disabled as a property, like we do for settings-input and settings-checkbox. So the host can set a dropdown menu to disabled and that passes thru to the <paper-dropdown-menu>. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:49: disabled: Boolean, On 2015/11/11 02:04:21, stevenjb wrote: > If we actually need this, let's just document that this reflects the HTML > element property. This is just an HTMLElement so it doesn't have that property. (HTMLInputElements have a disabled property.) BTW, we should add reflectToAttribute: true and (if we end up disabling automatically) *maybe* notify: true. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:52: * A reverse lookup from the menu value back to the index in the Crap! I forgot to update this doc. This is a reverse lookup from the menu value (the stringified item[0] of each menuOption) to the original value (the item[0] of each menuOption). Think of it as a hash map from (menuOption[i][0].toString()) to (menuOption[i][0]). The type should be Object<string, (string|number|Object)>. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:62: * The current selected value, as a string. On 2015/11/13 00:11:16, dschuyler wrote: > Is this really the value? Doesn't this get run through a lookup map to get the > value? You're right, but that's why I kept "as a string". The <paper-menu attr-for-selected="data-value"> has a bunch of <paper-items data-value="some string">. So this matches the "selected" property of the paper-menu. It comes from converting the values of the options in menuOptions to strings via data binding to the data-value attribute. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:114: // Create a map from index value [0] to the original value. This was ambiguous. Or just wrong. I should have said "from the stringified value to the original value". https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:131: var selected = this.menuMap_[this.selected_]; On 2015/11/13 00:11:16, dschuyler wrote: > It is confusing that selected_ is translated into > selected. Err, well, I happen to already know it's translating > an index number into a preference value string, but it's > hard to see that with lhs and rhs called selected{_}. > > Maybe prefString on the lhs? No, we're not using index numbers in this CL. We're using the data-value attribute and taking advantage of the paper-menu's attrForSelected property so we don't have to handle the indices manually. The conversion here is done solely to get out the same type that came in via menuOptions, since selected_ is always a String. And now I don't even think we need to do that. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:158: return selected == this.notFoundItem_[0]; On 2015/11/13 00:11:16, dschuyler wrote: > Why is this using an array element rather than just using '' directly? Meh. It seemed more natural to me to make notFoundItem_ the same type as menuOption items, but it could just as easily be a const somewhere if that makes more sense.
https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:62: * The current selected value, as a string. On 2015/11/13 00:33:08, michaelpg wrote: > On 2015/11/13 00:11:16, dschuyler wrote: > > Is this really the value? Doesn't this get run through a lookup map to get > the > > value? > > You're right, but that's why I kept "as a string". > > The <paper-menu attr-for-selected="data-value"> has a bunch of <paper-items > data-value="some string">. So this matches the "selected" property of the > paper-menu. It comes from converting the values of the options in menuOptions to > strings via data binding to the data-value attribute. Ah, thanks for the explanation. That data-value/attr-for-selected is rather nice. I tinkered with it a bit, and it looks like the type that is initially set for |selected_| is ignored. if the data-value is a number, then selected_ becomes a number; if data-value is a string, then selected_ becomes a string. That seems even more handy since we can set the type in the menu-options. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:131: var selected = this.menuMap_[this.selected_]; On 2015/11/13 00:33:08, michaelpg wrote: > On 2015/11/13 00:11:16, dschuyler wrote: > > It is confusing that selected_ is translated into > > selected. Err, well, I happen to already know it's translating > > an index number into a preference value string, but it's > > hard to see that with lhs and rhs called selected{_}. > > > > Maybe prefString on the lhs? > > No, we're not using index numbers in this CL. We're using the data-value > attribute and taking advantage of the paper-menu's attrForSelected property so > we don't have to handle the indices manually. > > The conversion here is done solely to get out the same type that came in via > menuOptions, since selected_ is always a String. And now I don't even think we > need to do that. Now that I have a better understanding of the data-value, it looks like the menuMap_ may be unneeded. Is that what you're saying may no longer be needed - if so, I think you're right. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:158: return selected == this.notFoundItem_[0]; On 2015/11/13 00:33:08, michaelpg wrote: > On 2015/11/13 00:11:16, dschuyler wrote: > > Why is this using an array element rather than just using '' directly? > > Meh. It seemed more natural to me to make notFoundItem_ the same type as > menuOption items, but it could just as easily be a const somewhere if that makes > more sense. Maybe a closure type could be made for menu-option elements and this could be one of those types. I think that would relay the intent.
https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:62: * The current selected value, as a string. On 2015/11/13 19:20:29, dschuyler wrote: > On 2015/11/13 00:33:08, michaelpg wrote: > > On 2015/11/13 00:11:16, dschuyler wrote: > > > Is this really the value? Doesn't this get run through a lookup map to get > > the > > > value? > > > > You're right, but that's why I kept "as a string". > > > > The <paper-menu attr-for-selected="data-value"> has a bunch of <paper-items > > data-value="some string">. So this matches the "selected" property of the > > paper-menu. It comes from converting the values of the options in menuOptions > to > > strings via data binding to the data-value attribute. > > Ah, thanks for the explanation. > That data-value/attr-for-selected is rather nice. > > I tinkered with it a bit, and it looks like the type > that is initially set for |selected_| is ignored. > if the data-value is a number, then selected_ becomes > a number; if data-value is a string, then selected_ > becomes a string. That seems even more handy since > we can set the type in the menu-options. Interesting! And weird and confusing. And apparently inconsistent. I filed https://github.com/PolymerElements/iron-selector/issues/84 to track this. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:131: var selected = this.menuMap_[this.selected_]; On 2015/11/13 19:20:29, dschuyler wrote: > On 2015/11/13 00:33:08, michaelpg wrote: > > On 2015/11/13 00:11:16, dschuyler wrote: > > > It is confusing that selected_ is translated into > > > selected. Err, well, I happen to already know it's translating > > > an index number into a preference value string, but it's > > > hard to see that with lhs and rhs called selected{_}. > > > > > > Maybe prefString on the lhs? > > > > No, we're not using index numbers in this CL. We're using the data-value > > attribute and taking advantage of the paper-menu's attrForSelected property so > > we don't have to handle the indices manually. > > > > The conversion here is done solely to get out the same type that came in via > > menuOptions, since selected_ is always a String. And now I don't even think we > > need to do that. > > Now that I have a better understanding of the data-value, it looks like the > menuMap_ may be unneeded. Is that what you're saying may no longer be needed - > if so, I think you're right. Yep. https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:158: return selected == this.notFoundItem_[0]; On 2015/11/13 19:20:29, dschuyler wrote: > On 2015/11/13 00:33:08, michaelpg wrote: > > On 2015/11/13 00:11:16, dschuyler wrote: > > > Why is this using an array element rather than just using '' directly? > > > > Meh. It seemed more natural to me to make notFoundItem_ the same type as > > menuOption items, but it could just as easily be a const somewhere if that > makes > > more sense. > > Maybe a closure type could be made for menu-option elements and this > could be one of those types. I think that would relay the intent. SGTM
Michael, I really like the possibility that we can remove the menuMap_ and still get the constant time lookup (no lookup, even). Please do make that change and we'll go forward with this version (CL) of the menu (and I'll close the CL for my changes to the menu).
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Simplify settings-dropdown-menu and consolidate with settings-radio-group Moves the "pref -> selected -> pref" logic to SelectablePrefBehavior. Simplifies settings-dropdown-menu a bit. BUG=425627 R=dschuyler@chromium.org,stevenjb@chromium.org ========== to ========== Simplify settings-dropdown-menu and consolidate with settings-radio-group Uses the new PrefUtil functionality to test selected elements. Adds tests. BUG=425627 R=dschuyler@chromium.org,stevenjb@chromium.org ==========
Description was changed from ========== Simplify settings-dropdown-menu and consolidate with settings-radio-group Uses the new PrefUtil functionality to test selected elements. Adds tests. BUG=425627 R=dschuyler@chromium.org,stevenjb@chromium.org ========== to ========== Simplify settings-dropdown-menu and add tests. Uses the new PrefUtil functionality to test selected elements. BUG=425627 R=dschuyler@chromium.org,stevenjb@chromium.org ==========
PTAL. This should be good to go as the finished, committable version (pending any feedback or bugs of course). https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/1422023012/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:49: disabled: Boolean, On 2015/11/13 00:33:08, michaelpg wrote: > On 2015/11/11 02:04:21, stevenjb wrote: > > If we actually need this, let's just document that this reflects the HTML > > element property. > > This is just an HTMLElement so it doesn't have that property. (HTMLInputElements > have a disabled property.) > > BTW, we should add reflectToAttribute: true and (if we end up disabling > automatically) *maybe* notify: true. Done. https://codereview.chromium.org/1422023012/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/settings_dropdown_menu.html (right): https://codereview.chromium.org/1422023012/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/controls/settings_dropdown_menu.html:20: hidden$="[[!showNotFoundItem_]]"> In light of recent performance discussions, maybe <dom-if> is more performant than a hidden item here, but A) it's tricky to get right because the dom-if might not be updated until after the paper-menu-button text has been set, and B) it's just one item in a menu with many items.
lgtm
A few questions, but regardless lgtm https://codereview.chromium.org/1422023012/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/settings_dropdown_menu.html (right): https://codereview.chromium.org/1422023012/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/controls/settings_dropdown_menu.html:20: hidden$="[[!showNotFoundItem_]]"> On 2015/11/17 05:08:56, michaelpg wrote: > In light of recent performance discussions, maybe <dom-if> is more performant > than a hidden item here, but A) it's tricky to get right because the dom-if > might not be updated until after the paper-menu-button text has been set, and B) > it's just one item in a menu with many items. Agreed. We should focus on discluding cards/sections with complex dropdown menus from the main pages instead since they are going to be fairly expensive regardless. https://codereview.chromium.org/1422023012/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/1422023012/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:87: computed: 'isSelectedNotFound_(selected_)', Question: Is this more efficient than just embedding isSelectedNotFound_(selected_) in the HTML? (i.e. should we prefer one over the other?) https://codereview.chromium.org/1422023012/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/dropdown_menu_tests.js (right): https://codereview.chromium.org/1422023012/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/dropdown_menu_tests.js:38: var selectedItem = getSelectable(dropdown).selectedItem; We call getSelectable(dropdown) a lot, any reason we cant just set a 'selectable' (or maybe just 'dropdownMenu') variable in setup()? https://codereview.chromium.org/1422023012/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/dropdown_menu_tests.js:53: var dropdown = document.createElement('settings-dropdown-menu'); We seem to do this in every test, can we do it in setup()?
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/1422023012/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/settings_dropdown_menu.js (right): https://codereview.chromium.org/1422023012/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/controls/settings_dropdown_menu.js:87: computed: 'isSelectedNotFound_(selected_)', On 2015/11/17 18:19:23, stevenjb wrote: > Question: Is this more efficient than just embedding > isSelectedNotFound_(selected_) in the HTML? (i.e. should we prefer one over the > other?) > > Done: Not having data, it's simpler to embed it since we don't use this computed property for anything else. (If anything, my guess is that embedding it might be more efficient than what I had.) https://codereview.chromium.org/1422023012/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/dropdown_menu_tests.js (right): https://codereview.chromium.org/1422023012/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/dropdown_menu_tests.js:38: var selectedItem = getSelectable(dropdown).selectedItem; On 2015/11/17 18:19:23, stevenjb wrote: > We call getSelectable(dropdown) a lot, any reason we cant just set a > 'selectable' (or maybe just 'dropdownMenu') variable in setup()? Not really. I changed setup to create the dropdown menu and set 'selectable'. This also led me to be more precise about the async stuff (using Polymer.dom.flush, we can make the tests synchronous). https://codereview.chromium.org/1422023012/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/dropdown_menu_tests.js:53: var dropdown = document.createElement('settings-dropdown-menu'); On 2015/11/17 18:19:23, stevenjb wrote: > We seem to do this in every test, can we do it in setup()? Done.
Changes lgtm
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/1422023012/#ps100002 (title: "stevenjb feedback + disabled")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422023012/100002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422023012/100002
Patchset #5 (id:150001) has been deleted
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/1422023012/#ps100002 (title: "stevenjb feedback + disabled")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422023012/100002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422023012/100002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Description was changed from ========== Simplify settings-dropdown-menu and add tests. Uses the new PrefUtil functionality to test selected elements. BUG=425627 R=dschuyler@chromium.org,stevenjb@chromium.org ========== to ========== MD Settings: Simplify settings-dropdown-menu and add tests. Uses the new PrefUtil functionality to test selected elements. BUG=425627 R=dschuyler@chromium.org,stevenjb@chromium.org ==========
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1422023012/#ps170001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422023012/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422023012/170001
Message was sent while issue was closed.
Committed patchset #5 (id:170001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5c22310582dfb1d9a2e50401ce31fa6ec57fc0e1 Cr-Commit-Position: refs/heads/master@{#360421} |
