Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1064573003:
Fix scroll regression when specifying an extension id. (Closed)
|
Created:
5 years, 8 months ago by hcarmona Modified:
5 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org, Devlin 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.
BUG=473002
Committed: https://crrev.com/4eaeca8aa713e9b7b1f214413bfdac87956fd51d
Cr-Commit-Position: refs/heads/master@{#325278}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move visibility check #
Total comments: 9
Patch Set 3 : rebase + feedback #
Total comments: 3
Patch Set 4 : Comments! #
Messages
Total messages: 25 (4 generated)
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org, kalman@chromium.org
please change the CL description to *what* you're doing, not *how* you're doing it. (e.g. "Fix extension highlighting regression.") https://codereview.chromium.org/1064573003/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:298: if (!visible) i guess I see why you're doing this but it's certainly weird to have an updateVisibility() that only cares about showing...
lgtm
https://codereview.chromium.org/1064573003/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:298: if (!visible) On 2015/04/14 20:33:56, Dan Beam wrote: > i guess I see why you're doing this but it's certainly weird to have an > updateVisibility() that only cares about showing... Yeah - it looks odd to me to only be using the parameter to decide whether the function does anything. Seems like, at the moment, it should be handled at the call site... if (hasExtensions) extensionList.updateVisibility().
https://codereview.chromium.org/1064573003/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:298: if (!visible) On 2015/04/14 20:38:50, kalman wrote: > On 2015/04/14 20:33:56, Dan Beam wrote: > > i guess I see why you're doing this but it's certainly weird to have an > > updateVisibility() that only cares about showing... > > Yeah - it looks odd to me to only be using the parameter to decide whether the > function does anything. Seems like, at the moment, it should be handled at the > call site... if (hasExtensions) extensionList.updateVisibility(). Moved the visibility check to the call site and renamed the method to better convey that it should be called when the list is visible.
https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:297: updateVisible: function() { TBH "updateVisible" is still too vague for me. Are these visible elements? nodes? extensions? Seems more like... well, things, but you can't say that. Maybe something along the lines of "refresh" is more appropriate? Also: should callers of updateFocusableElements be using this method instead? The public API of ExtensionList is growing, and as a consumer of its API I don't know when to call updateVisible and when to call updateFocusable. Or updateExtensionsData for that matter.
also this https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:287: resolve(); nit: call this.whateverYouEndUpNamingIt_() here and make private? https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:297: updateVisible: function() { how about: - updateAndHighlight() - onExtensionsShown() - afterUpdate() - onUpdateFinished() ?
https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:287: resolve(); On 2015/04/14 21:44:13, Dan Beam wrote: > nit: call this.whateverYouEndUpNamingIt_() here and make private? I was looking at this place, but the list is still hidden here. What about moving the hide logic into here instead of the extension-list-wrapper? https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:297: updateVisible: function() { We can go with onUpdateFinished_() and make it private if there are no objections to making the list show itself. updateFocusableElements is different than this because this will potentially scroll while the updateFocusableElements just updates the elements that can be focused.
+rdevlin.cronin@ as an FYI https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:287: resolve(); On 2015/04/14 22:06:24, Hector Carmona wrote: > On 2015/04/14 21:44:13, Dan Beam wrote: > > nit: call this.whateverYouEndUpNamingIt_() here and make private? > > I was looking at this place, but the list is still hidden here. What about > moving the hide logic into here instead of the extension-list-wrapper? talked about this IRL: I meant that after resolve() is called all the .then() stuff should already be done (if all subscribers are resolved synchronously) and we can do some private stuff after (and maybe assert(someNodeIsntHidden) to make sure it doesn't break in the future)
rdevlin.cronin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:295: * @param {boolean} visible Whether the extension list is visible. Drive-by: What param?
https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:297: updateVisible: function() { On 2015/04/14 22:06:24, Hector Carmona wrote: > We can go with onUpdateFinished_() and make it private if there are no > objections to making the list show itself. > > updateFocusableElements is different than this because this will potentially > scroll while the updateFocusableElements just updates the elements that can be > focused. Gotcha. Maybe this doesn't make sense, but could there be a single method which takes a parameter whether to scroll or not? "no objections to making the list show itself" - in what way is the list showing itself, and what might the objections be?
https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/20001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:297: updateVisible: function() { On 2015/04/14 22:39:25, kalman wrote: > On 2015/04/14 22:06:24, Hector Carmona wrote: > > We can go with onUpdateFinished_() and make it private if there are no > > objections to making the list show itself. > > > > updateFocusableElements is different than this because this will potentially > > scroll while the updateFocusableElements just updates the elements that can be > > focused. > > Gotcha. Maybe this doesn't make sense, but could there be a single method which > takes a parameter whether to scroll or not? Scrolling should happen when the list is loaded, if we can keep it encapsulated in the ExtensionList it'll make for a cleaner API > "no objections to making the list show itself" - in what way is the list showing > itself, and what might the objections be? List currently becomes visible after loading via a promise. However, if we use the same promise, it's not necessary to expose this method at all (see updated code).
https://codereview.chromium.org/1064573003/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:288: this.extensionsUpdated_.then(this.onUpdateFinished_.bind(this)); Since the promise has already resolved, this should be equivalent (and not as mind-bending) as just calling this.onUpdateFinished_() directly?
https://codereview.chromium.org/1064573003/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:288: this.extensionsUpdated_.then(this.onUpdateFinished_.bind(this)); On 2015/04/14 23:35:25, kalman wrote: > Since the promise has already resolved, this should be equivalent (and not as > mind-bending) as just calling this.onUpdateFinished_() directly? |resolve| is async and it's important that |onUpdateFinished_| be called after the |then| in extensions.js because that's where the list is unhidden.
lgtm (unfortunate that it's async, though...)
lgtm https://codereview.chromium.org/1064573003/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1064573003/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:288: this.extensionsUpdated_.then(this.onUpdateFinished_.bind(this)); On 2015/04/14 23:49:06, Hector Carmona wrote: > On 2015/04/14 23:35:25, kalman wrote: > > Since the promise has already resolved, this should be equivalent (and not as > > mind-bending) as just calling this.onUpdateFinished_() directly? > > |resolve| is async and it's important that |onUpdateFinished_| be called after > the |then| in extensions.js because that's where the list is unhidden. Mind... blown. Maybe add a comment. Are you sure that the resolve order is stable?
On 2015/04/14 23:56:22, kalman wrote: > lgtm > > https://codereview.chromium.org/1064573003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/extensions/extension_list.js (right): > > https://codereview.chromium.org/1064573003/diff/40001/chrome/browser/resource... > chrome/browser/resources/extensions/extension_list.js:288: > this.extensionsUpdated_.then(this.onUpdateFinished_.bind(this)); > On 2015/04/14 23:49:06, Hector Carmona wrote: > > On 2015/04/14 23:35:25, kalman wrote: > > > Since the promise has already resolved, this should be equivalent (and not > as > > > mind-bending) as just calling this.onUpdateFinished_() directly? > > > > |resolve| is async and it's important that |onUpdateFinished_| be called after > > the |then| in extensions.js because that's where the list is unhidden. > > Mind... blown. Maybe add a comment. Are you sure that the resolve order is > stable? Added comments + verified that the resolve order is the same as the order that |then| is called. Big thanks to Dan for pointing me at the spec! In case anyone feels like reading: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise-objects Specific for the order of resolving promises: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-triggerpromisereactions
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1064573003/#ps60001 (title: "Comments!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1064573003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4eaeca8aa713e9b7b1f214413bfdac87956fd51d Cr-Commit-Position: refs/heads/master@{#325278}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1088183004/ by hanxi@chromium.org. The reason for reverting is: It might cause the failures of: -ExtensionSettingsCommandsConfigWebUITest.testChromeSendHandler -ExtensionSettingsWebUITest.testChromeSendHandled http://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/1149. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
