Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2480843003:
change site-settings -> usb-device to use cr-action-menu instead of paper-menu (Closed)
|
Created:
4 years, 1 month ago by scottchen Modified:
4 years, 1 month ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, vitalyp+closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchange site-settings -> usb-device to use cr-action-menu instead of paper-menu
This CL is part of the effort to move off of paper-item which is known to have performance issues.
Note for future improvement: dpapad@ and I noticed that there's only a remove button in the dropdown menu, so we can probably replace it with just an "X" button.
BUG=603976
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6d273cd75ba1d15a782fd06677560bf54fade3f2
Cr-Commit-Position: refs/heads/master@{#431362}
Patch Set 1 #Patch Set 2 : add tests for patch 1 code #Patch Set 3 : add annotation to fix closure_compiler #
Total comments: 30
Patch Set 4 : fix up usb-device tests as comments recommended on previous patches #
Total comments: 19
Patch Set 5 : update tests as recommended by reviewers #Patch Set 6 : usb_device action menu now closes when tapped #Patch Set 7 : move helper function out of test function scope #
Total comments: 3
Messages
Total messages: 31 (17 generated)
Description was changed from ========== change site-settings -> usb-device to use cr-action-menu instead of paper-menu This CL is part of the effort to move off of paper-item which is known to have performance issues. Note for future improvement: dpapad@ and I noticed that there's only a remove button in the dropdown menu, so we can probably replace it with just an "X" button. BUG=603976 ========== to ========== change site-settings -> usb-device to use cr-action-menu instead of paper-menu This CL is part of the effort to move off of paper-item which is known to have performance issues. Note for future improvement: dpapad@ and I noticed that there's only a remove button in the dropdown menu, so we can probably replace it with just an "X" button. BUG=603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by scottchen@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...
scottchen@chromium.org changed reviewers: + dpapad@chromium.org, dschuyler@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is just a quick pass for closure and style notes. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.js (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:22: /** Please add a blank line before the /**. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:54: }, Please add a blank line here. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:55: showMenu_: function(event) { Please add an @param for |event|. Something like this with the 'example' bits changed to something better: /** * Example description. * @param {example_type} event */ https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:103: * @param !Array<UsbDeviceEntry> list The usb device entry list to set. The @param type should be in {}, like this: * @param {!Array<UsbDeviceEntry>} list The usb device entry list to set. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:231: removeUsbDevice: function(origin, embeddingOrigin, object) { @param for each parameter.
https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.html (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.html:33: class="dropdown-trigger"> Nit: Needs 2 more indent spaces. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.js (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:21: devices: Array, !Array<!UsbDeviceEntry> Also, can we turn this into a private member variable instead? https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:26: actionMenuModel: Object Since this is only used within this file, let's do the following /** * The ... * @private {?Object} */ actionMenuModel_: Object, https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:48: onRemoveTap_: function(event) { |event| is no longer used in this method, let's remove it. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:54: }, Add a blank line between functions. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:55: showMenu_: function(event) { Need @private and @param annotation. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:231: removeUsbDevice: function(origin, embeddingOrigin, object) { Why do you need to have any logic inside here? https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/usb_devices_tests.js (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:8: suite('USBDevices', function() { Nit: UsbDevices https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:62: return initPage(); Can we remove this call? Every test is calling initPage() already, because they need to setup fake data first. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:82: assertTrue(!!devices); This tests that |devices| is populated, but it's not giving us great coverage. |devices| is private state of the element that is tested, where as we should be testing externally observable behavior, meaning the DOM. For example, if someone updates the HTML code and break the dom-if (say items="devicesss"), this test will not break. So how about we change this to query the DOM that the right amount of list-item nodes have been created? (similar for tests below).
https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.html (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.html:33: class="dropdown-trigger"> On 2016/11/08 01:53:49, dpapad wrote: > Nit: Needs 2 more indent spaces. Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.js (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:21: devices: Array, On 2016/11/08 01:53:49, dpapad wrote: > !Array<!UsbDeviceEntry> > > Also, can we turn this into a private member variable instead? Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:22: /** On 2016/11/08 01:48:41, dschuyler wrote: > Please add a blank line before the /**. Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:26: actionMenuModel: Object On 2016/11/08 01:53:49, dpapad wrote: > Since this is only used within this file, let's do the following > > /** > * The ... > * @private {?Object} > */ > actionMenuModel_: Object, Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:48: onRemoveTap_: function(event) { On 2016/11/08 01:53:49, dpapad wrote: > |event| is no longer used in this method, let's remove it. Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:54: }, On 2016/11/08 01:48:40, dschuyler wrote: > Please add a blank line here. Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:54: }, On 2016/11/08 01:53:49, dpapad wrote: > Add a blank line between functions. Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:55: showMenu_: function(event) { On 2016/11/08 01:48:41, dschuyler wrote: > Please add an @param for |event|. > Something like this with the 'example' bits changed > to something better: > > /** > * Example description. > * @param {example_type} event > */ Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:55: showMenu_: function(event) { On 2016/11/08 01:53:49, dpapad wrote: > Need @private and @param annotation. Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:103: * @param !Array<UsbDeviceEntry> list The usb device entry list to set. On 2016/11/08 01:48:41, dschuyler wrote: > The @param type should be in {}, like this: > > * @param {!Array<UsbDeviceEntry>} list The usb device entry list to set. Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:231: removeUsbDevice: function(origin, embeddingOrigin, object) { On 2016/11/08 01:53:49, dpapad wrote: > Why do you need to have any logic inside here? The intention was to mimic the expected remove logic, in order to test that the removeButton is sending the correct parameters from the device that triggered the remove. But now to think of it, there's probably some other way to capture the param it intends to pass and do assert on that instead. I'll look into it. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:231: removeUsbDevice: function(origin, embeddingOrigin, object) { On 2016/11/08 01:48:41, dschuyler wrote: > @param for each parameter. Acknowledged. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/usb_devices_tests.js (right): https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:8: suite('USBDevices', function() { On 2016/11/08 01:53:49, dpapad wrote: > Nit: UsbDevices Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:62: return initPage(); On 2016/11/08 01:53:49, dpapad wrote: > Can we remove this call? Every test is calling initPage() already, because they > need to setup fake data first. Done. https://codereview.chromium.org/2480843003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:82: assertTrue(!!devices); On 2016/11/08 01:53:49, dpapad wrote: > This tests that |devices| is populated, but it's not giving us great coverage. > |devices| is private state of the element that is tested, where as we should be > testing externally observable behavior, meaning the DOM. > For example, if someone updates the HTML code and break the dom-if (say > items="devicesss"), this test will not break. > > So how about we change this to query the DOM that the right amount of list-item > nodes have been created? (similar for tests below). Done.
The CQ bit was checked by scottchen@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...
Patchset #5 (id:80001) has been deleted
Almost there, just a few more comments. https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.html (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.html:40: <button class="dropdown-item" role="option" on-tap="onRemoveTap_" id="remove-button"> Nit: s/remove-button/removeButton This way you can access it without quotes from JS. https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/usb_devices_tests.js (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:75: return browserProxy.whenCalled('fetchUsbDevices'); Polymer.dom.flush() is called after every initPage() call, so let's move it here instead. return browserProxy.whenCalled('fetchUsbDevices').then(function() { Polymer.dom.flush(); }); https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:97: test('non-empty device list creates dialog menu correctly', function() { I don't think this test is adding much value. the cr-action-menu exists on the page regardless of the |devices| array being not empty. If the dialog did not exist at all the next test would fail anyway, so how about we remove this one? https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:102: var dialog = testElement.$$('dialog'); Let's be specific about what kind of dialog we look for (here and elsewhere). $$('dialog[is=cr-action-menu]') https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:124: assertTrue(dialog.hasAttribute('open')); Nit: I think this would also work assertTrue(dialog.open) https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:128: test('try removing items using remove button', function() { Given that we are not actually removing anything during the test (we simply report back arguments from removeUsbDevice), I think testing two removals instead of just one has no benefit. How about we simply test only one removal and merge this test with the one above?
https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.js (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:51: item.origin, item.embeddingOrigin, item.object); How does the dialog get closed after we tap the remove button? Shouldn't we be calling this.$$('dialog[is=cr-action-menu]')).close() here?
https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.html (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.html:40: <button class="dropdown-item" role="option" on-tap="onRemoveTap_" id="remove-button"> On 2016/11/09 20:32:09, dpapad wrote: > Nit: s/remove-button/removeButton > > This way you can access it without quotes from JS. Done. https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/usb_devices_tests.js (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:75: return browserProxy.whenCalled('fetchUsbDevices'); On 2016/11/09 20:32:09, dpapad wrote: > Polymer.dom.flush() is called after every initPage() call, so let's move it here > instead. > > return browserProxy.whenCalled('fetchUsbDevices').then(function() { > Polymer.dom.flush(); > }); Done. https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:97: test('non-empty device list creates dialog menu correctly', function() { On 2016/11/09 20:32:09, dpapad wrote: > I don't think this test is adding much value. the cr-action-menu exists on the > page regardless of the |devices| array being not empty. If the dialog did not > exist at all the next test would fail anyway, so how about we remove this one? Done. https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:102: var dialog = testElement.$$('dialog'); On 2016/11/09 20:32:09, dpapad wrote: > Let's be specific about what kind of dialog we look for (here and elsewhere). > > $$('dialog[is=cr-action-menu]') Done. https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:124: assertTrue(dialog.hasAttribute('open')); On 2016/11/09 20:32:09, dpapad wrote: > Nit: I think this would also work > > assertTrue(dialog.open) Done. https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:128: test('try removing items using remove button', function() { On 2016/11/09 20:32:09, dpapad wrote: > Given that we are not actually removing anything during the test (we simply > report back arguments from removeUsbDevice), I think testing two removals > instead of just one has no benefit. How about we simply test only one removal > and merge this test with the one above? - I think the previous test is to test the menu button's showMenu_ handler works correctly, where as this one is testing the removeButton sending correct params, so if for some reason showMenu_ is broken. By having two separate tests, I think it'll be easier to tell what's wrong just by looking at which test failed. - I tested tapping on the remove button for two different device, because I want to make sure the showMenu_ handler is setting the right actionMenuModel_ and onRemoveTap_ is deleting the right item both times. I could imagine incorrect implementations of the code where onRemoveTap_ is just always trying to delete the first device, or whichever device gets clicked first, etc. Please let me know if those two points make sense.
https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.js (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:51: item.origin, item.embeddingOrigin, item.object); On 2016/11/09 at 21:37:42, dpapad wrote: > How does the dialog get closed after we tap the remove button? Shouldn't we be calling this.$$('dialog[is=cr-action-menu]')).close() here? ^^ Also can we ensure that the test checks that the action menu is closed after a removal? https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/usb_devices_tests.js (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:128: test('try removing items using remove button', function() { On 2016/11/10 at 06:08:21, scottchen wrote: > On 2016/11/09 20:32:09, dpapad wrote: > > Given that we are not actually removing anything during the test (we simply > > report back arguments from removeUsbDevice), I think testing two removals > > instead of just one has no benefit. How about we simply test only one removal > > and merge this test with the one above? > > - I think the previous test is to test the menu button's showMenu_ handler works correctly, where as this one is testing the removeButton sending correct params, so if for some reason showMenu_ is broken. By having two separate tests, I think it'll be easier to tell what's wrong just by looking at which test failed. OK, that makes sense. > > - I tested tapping on the remove button for two different device, because I want to make sure the showMenu_ handler is setting the right actionMenuModel_ and onRemoveTap_ is deleting the right item both times. I could imagine incorrect implementations of the code where onRemoveTap_ is just always trying to delete the first device, or whichever device gets clicked first, etc. Makes sense, except from the fact that the testRemovalFlow() calls initPage(), which means the 1st and 2nd removal are actually happening on different <usb-devices> instances, and effectively we are testing a 1st removal twice. > > Please let me know if those two points make sense. https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:138: function testRemovalFlow(indexToRemove){ Probably more readable if you move this helper function outside of test(function() {...});
https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/usb_devices.js (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:51: item.origin, item.embeddingOrigin, item.object); On 2016/11/09 21:37:42, dpapad wrote: > How does the dialog get closed after we tap the remove button? Shouldn't we be > calling this.$$('dialog[is=cr-action-menu]')).close() here? Done. https://codereview.chromium.org/2480843003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/usb_devices.js:51: item.origin, item.embeddingOrigin, item.object); On 2016/11/10 17:35:09, dpapad wrote: > On 2016/11/09 at 21:37:42, dpapad wrote: > > How does the dialog get closed after we tap the remove button? Shouldn't we be > calling this.$$('dialog[is=cr-action-menu]')).close() here? > > ^^ Also can we ensure that the test checks that the action menu is closed after > a removal? > Done.
The CQ bit was checked by scottchen@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/2480843003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/usb_devices_tests.js (right): https://codereview.chromium.org/2480843003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/usb_devices_tests.js:138: function testRemovalFlow(indexToRemove){ On 2016/11/10 17:35:09, dpapad wrote: > Probably more readable if you move this helper function outside of > test(function() {...}); Done.
LGTM, with some nits. https://codereview.chromium.org/2480843003/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/settings/usb_devices_tests.js (right): https://codereview.chromium.org/2480843003/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/usb_devices_tests.js:101: assertTrue(!!menuButton); Add an assertion here that the dialog is initially closed. https://codereview.chromium.org/2480843003/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/usb_devices_tests.js:119: .querySelectorAll('paper-icon-button')[indexToRemove]; Nit: Per styleguide at https://google.github.io/styleguide/javascriptguide.xml#Code_formatting, the dot operator should be in the previous line in such cases (search for "This includes the dot operator") https://codereview.chromium.org/2480843003/diff/140001/chrome/test/data/webui... chrome/test/data/webui/settings/usb_devices_tests.js:140: var self = this; Don't see any uses of self. Maybe remove?
The CQ bit was checked by scottchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== change site-settings -> usb-device to use cr-action-menu instead of paper-menu This CL is part of the effort to move off of paper-item which is known to have performance issues. Note for future improvement: dpapad@ and I noticed that there's only a remove button in the dropdown menu, so we can probably replace it with just an "X" button. BUG=603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== change site-settings -> usb-device to use cr-action-menu instead of paper-menu This CL is part of the effort to move off of paper-item which is known to have performance issues. Note for future improvement: dpapad@ and I noticed that there's only a remove button in the dropdown menu, so we can probably replace it with just an "X" button. BUG=603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6d273cd75ba1d15a782fd06677560bf54fade3f2 Cr-Commit-Position: refs/heads/master@{#431362} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6d273cd75ba1d15a782fd06677560bf54fade3f2 Cr-Commit-Position: refs/heads/master@{#431362} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
