|
|
Created:
5 years, 8 months ago by hcarmona Modified:
5 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix scroll regression when specifying an extension id.
Issue was caused because scrolling the page should happen after the
extensions are visible.
Updated tests so that they know when the page has loaded.
Added tests to make sure the list of extensions is hidden or shown.
BUG=473002
Committed: https://crrev.com/579b37723600b2d1c8636d6f3fd5ffb0814a67cb
Cr-Commit-Position: refs/heads/master@{#326554}
Patch Set 1 : Same as reverted CL #Patch Set 2 : Hopefully useful logging #Patch Set 3 : MOAR!!! #Patch Set 4 : Fix #Patch Set 5 : Make tests async #Patch Set 6 : Add tests #
Total comments: 4
Patch Set 7 : bind + comment #
Total comments: 2
Patch Set 8 : Apply Feedback #
Total comments: 9
Patch Set 9 : Fix annotation #
Total comments: 4
Patch Set 10 : Apply feedback #
Total comments: 3
Patch Set 11 : One more test #Patch Set 12 : Disable transitions for a11y #
Total comments: 11
Patch Set 13 : Dan's Feedback #
Total comments: 4
Patch Set 14 : Less binding #
Total comments: 1
Messages
Total messages: 42 (7 generated)
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org, kalman@chromium.org, rdevlin.cronin@chromium.org
This CL was reverted the other day. I've fixed the cause of the failure and added tests so that trybots will more easily catch a similar issue.
That's a lot of changes to the test file; what is the gist of it? https://codereview.chromium.org/1090763003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/100001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:247: }); .bind(this) instead of self? https://codereview.chromium.org/1090763003/diff/100001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:303: // |onUpdateFinished_| shouldn't be called directly. I don't understand the second half of this comment in the context of the CL; it looks like onUpdateFinished_ *is* being called directly.
Th gist of changes to the tests: Refactored tests so they are all async. One set of tests used "steps" for the async parts to make the tests more readable + async. I moved this to the base class and updated the others. I added a test to verify that the list of extensions is visible after loading. Also added a test to verify that the list of extensions is NOT visible if there are no extensions. (The list not being visible caused the failures that got this CL reverted) https://codereview.chromium.org/1090763003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/100001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:247: }); On 2015/04/17 19:40:59, kalman wrote: > .bind(this) instead of self? Done. https://codereview.chromium.org/1090763003/diff/100001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:303: // |onUpdateFinished_| shouldn't be called directly. On 2015/04/17 19:40:59, kalman wrote: > I don't understand the second half of this comment in the context of the CL; it > looks like onUpdateFinished_ *is* being called directly. Updated comment.
You've reordered a bunch of tests, which is the new one? https://codereview.chromium.org/1090763003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/120001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:305: this.onUpdateFinished_(); Whether or not there are >0 extensions, the update has finished, so it's weird not to call onUpdateFinished_ regardless. Perhaps make onUdpateFinished_ make the extensions.length>0 check.
https://codereview.chromium.org/1090763003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/120001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:305: this.onUpdateFinished_(); On 2015/04/17 21:50:11, kalman wrote: > Whether or not there are >0 extensions, the update has finished, so it's weird > not to call onUpdateFinished_ regardless. Perhaps make onUdpateFinished_ make > the extensions.length>0 check. Done. https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:105: TEST_F('ExtensionSettingsWebUITest', 'testEmptyExtensionList', function() { New test 1: empty list https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:209: TEST_F('BasicExtensionSettingsWebUITest', 'testNonEmptyExtensionList', New test 2: non-empty list
lg but double check the tests https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (left): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:187: enableDeveloperMode: function(callback) { It looks like this test has disappeared?
Double checked to make sure all tests are there. The only test that is deleted is testDeveloperModeA11y. https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (left): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:187: enableDeveloperMode: function(callback) { On 2015/04/17 23:00:45, kalman wrote: > It looks like this test has disappeared? Yes, |verifyDeveloperModeWorks| and |enableDeveloperMode| were doing the same thing. I removed the dupe code. |testDeveloperMode| near the top does the same thing as |testDeveloperModeA11y|. The a11y audit is still enabled for all tests.
lgtm
https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:301: this.extensionsUpdated_.then(function() { why not: this.loadFinished_ = this.extensionUpdated_.then(...); ? https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:315: /** Updates elements that need to be visible in order to update properly. */ /** * Updates ... * @private */
https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:301: this.extensionsUpdated_.then(function() { On 2015/04/18 01:28:40, Dan Beam wrote: > why not: > > this.loadFinished_ = this.extensionUpdated_.then(...); > > ? |updateExtensionsData| is called async, so the test wouldn't have a reference to the |loadFinished_| promise. Also, by assigning |loadFinished_| in |initialize| the promise will only resolve once. https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:315: /** Updates elements that need to be visible in order to update properly. */ On 2015/04/18 01:28:41, Dan Beam wrote: > /** > * Updates ... > * @private > */ Done.
lgtm https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:301: this.extensionsUpdated_.then(function() { On 2015/04/20 17:51:08, Hector Carmona wrote: > On 2015/04/18 01:28:40, Dan Beam wrote: > > why not: > > > > this.loadFinished_ = this.extensionUpdated_.then(...); > > > > ? > > |updateExtensionsData| is called async, so the test wouldn't have a > reference to the |loadFinished_| promise. Also, by assigning > |loadFinished_| in |initialize| the promise will only resolve once. ok, makes sense. thanks for explanation. https://codereview.chromium.org/1090763003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/160001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:303: // visible in extensions.js. ^ this is no longer true, is it? probably OK to just remove https://codereview.chromium.org/1090763003/diff/160001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:308: this.onLoadFinish_(); nit: maybe name this resolveLoadFinished_(); and remove comment? always prefer self-documenting var names to less self-documenting var name + comments
Applied Dan's feedback https://codereview.chromium.org/1090763003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/160001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:303: // visible in extensions.js. On 2015/04/20 17:56:00, Dan Beam wrote: > ^ this is no longer true, is it? probably OK to just remove Updated other comment to show that this is intended to be called after elements are visible. https://codereview.chromium.org/1090763003/diff/160001/chrome/browser/resourc... chrome/browser/resources/extensions/extension_list.js:308: this.onLoadFinish_(); On 2015/04/20 17:56:00, Dan Beam wrote: > nit: maybe name this resolveLoadFinished_(); and remove comment? always prefer > self-documenting var names to less self-documenting var name + comments Done.
lgtm with one request https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:90: TEST_F('ExtensionSettingsWebUITest', 'testDeveloperMode', function() { I preferred this when it was a BasicExtensionSettingsWebUITest (which is a confusing name... but that's my fault), because it tests showing all the dev elements for different types of extensions - which is actually a pretty good catch for any rogue errors lingering in weird code paths. I think it'd be better to either a) move this back as a BasicExtensionSettingsWebUITest or b) just install all those extension types in the default ExtensionSettingsWebUITest, and get rid of the BasicExtensionSettingsWebUITest subclass.
https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:90: TEST_F('ExtensionSettingsWebUITest', 'testDeveloperMode', function() { On 2015/04/21 18:35:05, Devlin wrote: > I preferred this when it was a BasicExtensionSettingsWebUITest (which is a > confusing name... but that's my fault), because it tests showing all the dev > elements for different types of extensions - which is actually a pretty good > catch for any rogue errors lingering in weird code paths. I think it'd be > better to either a) move this back as a BasicExtensionSettingsWebUITest or b) > just install all those extension types in the default > ExtensionSettingsWebUITest, and get rid of the BasicExtensionSettingsWebUITest > subclass. Why not both? I can create 2 tests: |testDeveloperModeNoExtensions| and |testDeveloperModeManyExtensions| (i'm open to other names). Both tests would verify the same things.
https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:90: TEST_F('ExtensionSettingsWebUITest', 'testDeveloperMode', function() { On 2015/04/21 18:56:39, Hector Carmona wrote: > On 2015/04/21 18:35:05, Devlin wrote: > > I preferred this when it was a BasicExtensionSettingsWebUITest (which is a > > confusing name... but that's my fault), because it tests showing all the dev > > elements for different types of extensions - which is actually a pretty good > > catch for any rogue errors lingering in weird code paths. I think it'd be > > better to either a) move this back as a BasicExtensionSettingsWebUITest or b) > > just install all those extension types in the default > > ExtensionSettingsWebUITest, and get rid of the BasicExtensionSettingsWebUITest > > subclass. > > Why not both? I can create 2 tests: |testDeveloperModeNoExtensions| and > |testDeveloperModeManyExtensions| (i'm open to other names). Both tests > would verify the same things. To me, it seems redundant, but I don't feel that strongly either way. :)
On 2015/04/21 18:57:41, Devlin wrote: > https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js > (right): > > https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... > chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:90: > TEST_F('ExtensionSettingsWebUITest', 'testDeveloperMode', function() { > On 2015/04/21 18:56:39, Hector Carmona wrote: > > On 2015/04/21 18:35:05, Devlin wrote: > > > I preferred this when it was a BasicExtensionSettingsWebUITest (which is a > > > confusing name... but that's my fault), because it tests showing all the dev > > > elements for different types of extensions - which is actually a pretty good > > > catch for any rogue errors lingering in weird code paths. I think it'd be > > > better to either a) move this back as a BasicExtensionSettingsWebUITest or > b) > > > just install all those extension types in the default > > > ExtensionSettingsWebUITest, and get rid of the > BasicExtensionSettingsWebUITest > > > subclass. > > > > Why not both? I can create 2 tests: |testDeveloperModeNoExtensions| and > > |testDeveloperModeManyExtensions| (i'm open to other names). Both tests > > would verify the same things. > > To me, it seems redundant, but I don't feel that strongly either way. :) Updated with 2 tests. It's better to test both scenarios since the list of extensions is hidden if there are no extensions.
On 2015/04/21 22:02:32, Hector Carmona wrote: > On 2015/04/21 18:57:41, Devlin wrote: > > > https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... > > File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js > > (right): > > > > > https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webu... > > chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:90: > > TEST_F('ExtensionSettingsWebUITest', 'testDeveloperMode', function() { > > On 2015/04/21 18:56:39, Hector Carmona wrote: > > > On 2015/04/21 18:35:05, Devlin wrote: > > > > I preferred this when it was a BasicExtensionSettingsWebUITest (which is a > > > > confusing name... but that's my fault), because it tests showing all the > dev > > > > elements for different types of extensions - which is actually a pretty > good > > > > catch for any rogue errors lingering in weird code paths. I think it'd be > > > > better to either a) move this back as a BasicExtensionSettingsWebUITest or > > b) > > > > just install all those extension types in the default > > > > ExtensionSettingsWebUITest, and get rid of the > > BasicExtensionSettingsWebUITest > > > > subclass. > > > > > > Why not both? I can create 2 tests: |testDeveloperModeNoExtensions| and > > > |testDeveloperModeManyExtensions| (i'm open to other names). Both tests > > > would verify the same things. > > > > To me, it seems redundant, but I don't feel that strongly either way. :) > > Updated with 2 tests. It's better to test both scenarios since the list > of extensions is hidden if there are no extensions. fwiw: I agree with testing both the empty and non-empty cases
not sure if you're waiting on me - if so, lgtm stands :)
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1090763003/#ps200001 (title: "One more test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1090763003/200001
The CQ bit was unchecked by hcarmona@chromium.org
Had to make a change to avoid flaking on a11y. PTAL
https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:99: window.setTimeout(function() { nit: window.setTimeout(this.nextSetup.bind(this), 0); https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:100: this.nextStep(this); this.nextStep(); https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:135: assertTrue(listWrapper.hidden); nit: inline variables only used once, e.g. assertTrue($('extension-list-wrapper').hidden); https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:250: assertGT(list.childNodes.length, 0); same nits re: inline vars https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:353: testDone]; some of these fit in one line, e.g. this.steps = [this.waitForPageLoad, testDone];
still lgtm overall though
https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:99: window.setTimeout(function() { On 2015/04/22 18:44:55, Dan Beam wrote: > nit: > window.setTimeout(this.nextSetup.bind(this), 0); Done. https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:100: this.nextStep(this); On 2015/04/22 18:44:55, Dan Beam wrote: > this.nextStep(); Acknowledged. https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:135: assertTrue(listWrapper.hidden); On 2015/04/22 18:44:54, Dan Beam wrote: > nit: inline variables only used once, e.g. > > assertTrue($('extension-list-wrapper').hidden); Done. https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:250: assertGT(list.childNodes.length, 0); On 2015/04/22 18:44:54, Dan Beam wrote: > same nits re: inline vars Done. https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:353: testDone]; On 2015/04/22 18:44:54, Dan Beam wrote: > some of these fit in one line, e.g. > > this.steps = [this.waitForPageLoad, testDone]; Done.
https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:371: document.querySelector('extensionoptions').onpreferredsizechanged(size); nit: this is the same number of lines but more is more readable, IMO https://codereview.chromium.org/1090763003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:109: }.bind(this)); |this| inside of checkDevModeIsOff might differ from inside of |testDeveloperMode|. might want to revert to what it was before in patch set 12: https://codereview.chromium.org/1090763003/diff2/220001:240001/chrome/browser... https://codereview.chromium.org/1090763003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:195: }.bind(this)); var next = this.nextStep.bind(this); ... next(); is less binding
https://codereview.chromium.org/1090763003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:109: }.bind(this)); On 2015/04/22 21:09:45, Dan Beam wrote: > |this| inside of checkDevModeIsOff might differ from inside of > |testDeveloperMode|. might want to revert to what it was before in patch set > 12: > https://codereview.chromium.org/1090763003/diff2/220001:240001/chrome/browser... Done. https://codereview.chromium.org/1090763003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:195: }.bind(this)); On 2015/04/22 21:09:45, Dan Beam wrote: > var next = this.nextStep.bind(this); > ... > next(); > > is less binding Done.
lgtm
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1090763003/#ps260001 (title: "Less binding")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1090763003/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/579b37723600b2d1c8636d6f3fd5ffb0814a67cb Cr-Commit-Position: refs/heads/master@{#326554}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:203: TEST_F('BasicExtensionSettingsWebUITest', 'testDeveloperModeManyExtensions', This is failing on http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/453 . I haven't checked yet if it's also failing in official builds when using msvs (still linking).
Message was sent while issue was closed.
ARG! Flakyness is bad... Should we revert the CL or disable the test? Failure is in a11y, but it is a false failure caused by transition... (which should take 0ms) >_< There is an issue I've been working on to re-enable another test in this file crbug.com/463245 (flakyness for same reason) On Fri, Apr 24, 2015 at 4:04 PM <thakis@chromium.org> wrote: > > > https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webu... > File > chrome/browser/ui/webui/extensions/extension_settings_browsertest.js > (right): > > > https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webu... > chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:203 > <https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webu...> > : > TEST_F('BasicExtensionSettingsWebUITest', > 'testDeveloperModeManyExtensions', > This is failing on > > http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/453 > . I haven't checked yet if it's also failing in official builds when > using msvs (still linking). > > https://codereview.chromium.org/1090763003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/04/24 23:31:29, Hector Carmona wrote: > ARG! Flakyness is bad... Should we revert the CL or disable the test? > Failure is in a11y, but it is a false failure caused by transition... > (which should take 0ms) >_< There is an issue I've been working on to > re-enable another test in this file crbug.com/463245 (flakyness for same > reason) The a11y error is saying that a focusable element is overlapped, which is kinda true - the dev controls are, the second they're turned visible, overlapped by the other buttons (perhaps even if it's a 0ms transition?). If it was me, I'd see about disabling only the a11y aspect of the test - the rest of it is still valid and useful. Even cooler yet would be if we could disable the a11y checking until after we know the transition has occurred, but that might be more complicated.
Message was sent while issue was closed.
On 2015/04/24 23:04:55, Nico wrote: > https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js > (right): > > https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webu... > chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:203: > TEST_F('BasicExtensionSettingsWebUITest', 'testDeveloperModeManyExtensions', > This is failing on > http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/453 > . I haven't checked yet if it's also failing in official builds when using msvs > (still linking). I have confirmed that this is also failing when doing an official build of browser_tests with Visual Studio.
Message was sent while issue was closed.
Looks fine, so lgtm, but you've tested this by hand right? Another option would be to revert the change that broke this in the first place, potentially safer, have you looked into that?
Message was sent while issue was closed.
I meant to make this comment on the other patch. I had them open side-by-side :) |