Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(182)

Issue 1090763003: Fix scroll regression when specifying an extension id. (Closed)

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.

Description

Fix 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+219 lines, -144 lines) Patch
M chrome/browser/resources/extensions/extension_list.js View 1 2 3 4 5 6 7 8 9 10 3 chunks +41 lines, -8 lines 0 comments Download
M chrome/browser/resources/extensions/extensions.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/extensions/extension_settings_browsertest.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 10 chunks +178 lines, -135 lines 1 comment Download

Messages

Total messages: 42 (7 generated)
hcarmona
This CL was reverted the other day. I've fixed the cause of the failure and ...
5 years, 8 months ago (2015-04-17 19:27:33 UTC) #2
not at google - send to devlin
That's a lot of changes to the test file; what is the gist of it? ...
5 years, 8 months ago (2015-04-17 19:40:59 UTC) #3
hcarmona
Th gist of changes to the tests: Refactored tests so they are all async. One ...
5 years, 8 months ago (2015-04-17 20:57:37 UTC) #4
not at google - send to devlin
You've reordered a bunch of tests, which is the new one? https://codereview.chromium.org/1090763003/diff/120001/chrome/browser/resources/extensions/extension_list.js File chrome/browser/resources/extensions/extension_list.js (right): ...
5 years, 8 months ago (2015-04-17 21:50:11 UTC) #5
hcarmona
https://codereview.chromium.org/1090763003/diff/120001/chrome/browser/resources/extensions/extension_list.js File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/120001/chrome/browser/resources/extensions/extension_list.js#newcode305 chrome/browser/resources/extensions/extension_list.js:305: this.onUpdateFinished_(); On 2015/04/17 21:50:11, kalman wrote: > Whether or ...
5 years, 8 months ago (2015-04-17 22:43:44 UTC) #6
not at google - send to devlin
lg but double check the tests https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (left): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#oldcode187 chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:187: enableDeveloperMode: function(callback) { ...
5 years, 8 months ago (2015-04-17 23:00:45 UTC) #7
hcarmona
Double checked to make sure all tests are there. The only test that is deleted ...
5 years, 8 months ago (2015-04-18 00:00:34 UTC) #8
not at google - send to devlin
lgtm
5 years, 8 months ago (2015-04-18 00:06:25 UTC) #9
Dan Beam
https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resources/extensions/extension_list.js File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resources/extensions/extension_list.js#newcode301 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/resources/extensions/extension_list.js#newcode315 ...
5 years, 8 months ago (2015-04-18 01:28:41 UTC) #10
hcarmona
https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resources/extensions/extension_list.js File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resources/extensions/extension_list.js#newcode301 chrome/browser/resources/extensions/extension_list.js:301: this.extensionsUpdated_.then(function() { On 2015/04/18 01:28:40, Dan Beam wrote: > ...
5 years, 8 months ago (2015-04-20 17:51:08 UTC) #11
Dan Beam
lgtm https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resources/extensions/extension_list.js File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/140001/chrome/browser/resources/extensions/extension_list.js#newcode301 chrome/browser/resources/extensions/extension_list.js:301: this.extensionsUpdated_.then(function() { On 2015/04/20 17:51:08, Hector Carmona wrote: ...
5 years, 8 months ago (2015-04-20 17:56:00 UTC) #12
hcarmona
Applied Dan's feedback https://codereview.chromium.org/1090763003/diff/160001/chrome/browser/resources/extensions/extension_list.js File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1090763003/diff/160001/chrome/browser/resources/extensions/extension_list.js#newcode303 chrome/browser/resources/extensions/extension_list.js:303: // visible in extensions.js. On 2015/04/20 ...
5 years, 8 months ago (2015-04-20 18:46:56 UTC) #13
Devlin
lgtm with one request https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode90 chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:90: TEST_F('ExtensionSettingsWebUITest', 'testDeveloperMode', function() { I ...
5 years, 8 months ago (2015-04-21 18:35:05 UTC) #14
hcarmona
https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode90 chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:90: TEST_F('ExtensionSettingsWebUITest', 'testDeveloperMode', function() { On 2015/04/21 18:35:05, Devlin wrote: ...
5 years, 8 months ago (2015-04-21 18:56:39 UTC) #15
Devlin
https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode90 chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:90: TEST_F('ExtensionSettingsWebUITest', 'testDeveloperMode', function() { On 2015/04/21 18:56:39, Hector Carmona ...
5 years, 8 months ago (2015-04-21 18:57:41 UTC) #16
hcarmona
On 2015/04/21 18:57:41, Devlin wrote: > https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js > File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js > (right): > > https://codereview.chromium.org/1090763003/diff/180001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode90 ...
5 years, 8 months ago (2015-04-21 22:02:32 UTC) #17
Dan Beam
On 2015/04/21 22:02:32, Hector Carmona wrote: > On 2015/04/21 18:57:41, Devlin wrote: > > > ...
5 years, 8 months ago (2015-04-21 22:19:30 UTC) #18
Devlin
not sure if you're waiting on me - if so, lgtm stands :)
5 years, 8 months ago (2015-04-21 22:28:55 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1090763003/200001
5 years, 8 months ago (2015-04-21 22:31:55 UTC) #22
hcarmona
Had to make a change to avoid flaking on a11y. PTAL
5 years, 8 months ago (2015-04-22 17:23:52 UTC) #24
Dan Beam
https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode99 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/webui/extensions/extension_settings_browsertest.js#newcode100 chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:100: this.nextStep(this); this.nextStep(); ...
5 years, 8 months ago (2015-04-22 18:44:55 UTC) #25
Dan Beam
still lgtm overall though
5 years, 8 months ago (2015-04-22 18:45:26 UTC) #26
hcarmona
https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode99 chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:99: window.setTimeout(function() { On 2015/04/22 18:44:55, Dan Beam wrote: > ...
5 years, 8 months ago (2015-04-22 20:44:45 UTC) #27
Dan Beam
https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/220001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode371 chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:371: document.querySelector('extensionoptions').onpreferredsizechanged(size); nit: this is the same number of lines ...
5 years, 8 months ago (2015-04-22 21:09:45 UTC) #28
hcarmona
https://codereview.chromium.org/1090763003/diff/240001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/240001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode109 chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:109: }.bind(this)); On 2015/04/22 21:09:45, Dan Beam wrote: > |this| ...
5 years, 8 months ago (2015-04-22 21:21:52 UTC) #29
Dan Beam
lgtm
5 years, 8 months ago (2015-04-22 21:30:33 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1090763003/260001
5 years, 8 months ago (2015-04-23 17:07:53 UTC) #33
commit-bot: I haz the power
Committed patchset #14 (id:260001)
5 years, 8 months ago (2015-04-23 17:12:51 UTC) #34
commit-bot: I haz the power
Patchset 14 (id:??) landed as https://crrev.com/579b37723600b2d1c8636d6f3fd5ffb0814a67cb Cr-Commit-Position: refs/heads/master@{#326554}
5 years, 8 months ago (2015-04-23 17:13:50 UTC) #35
Nico
https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode203 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 ...
5 years, 8 months ago (2015-04-24 23:04:55 UTC) #37
hcarmona
ARG! Flakyness is bad... Should we revert the CL or disable the test? Failure is ...
5 years, 8 months ago (2015-04-24 23:31:29 UTC) #38
Devlin
On 2015/04/24 23:31:29, Hector Carmona wrote: > ARG! Flakyness is bad... Should we revert the ...
5 years, 8 months ago (2015-04-24 23:37:18 UTC) #39
Nico
On 2015/04/24 23:04:55, Nico wrote: > https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js > File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js > (right): > > https://codereview.chromium.org/1090763003/diff/260001/chrome/browser/ui/webui/extensions/extension_settings_browsertest.js#newcode203 ...
5 years, 8 months ago (2015-04-27 17:21:53 UTC) #40
not at google - send to devlin
Looks fine, so lgtm, but you've tested this by hand right? Another option would be ...
5 years, 7 months ago (2015-05-06 19:16:00 UTC) #41
not at google - send to devlin
5 years, 7 months ago (2015-05-06 19:16:26 UTC) #42
Message was sent while issue was closed.
I meant to make this comment on the other patch. I had them open side-by-side :)

Powered by Google App Engine
This is Rietveld 408576698