Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1138973005:
[Extensions Page] Fix focus when an extension is removed (Closed)
|
Created:
5 years, 7 months ago by Devlin Modified:
5 years, 7 months ago Reviewers:
Dan Beam 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[Extensions Page] Fix focus when an extension is removed
When an extension is removed, if an element was focused, the analogous element
on the previous/next extension should be focused.
BUG=479092
Committed: https://crrev.com/0da16bdaa81d0952a73153e74c7889a273b75a3b
Cr-Commit-Position: refs/heads/master@{#330547}
Patch Set 1 #
Total comments: 6
Patch Set 2 : getRequiredElement #Messages
Total messages: 13 (3 generated)
rdevlin.cronin@chromium.org changed reviewers: + dbeam@chromium.org
Dan, please take a look when you can. Cheers! https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:286: this.removeNode_(node); I thought there was something like assertNotNull so that this could be this.removeNode_(assertNotNull($(eventData.item_id))); but I couldn't find it. Did I dream that up?
https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:286: this.removeNode_(node); On 2015/05/15 23:29:43, Devlin wrote: > I thought there was something like assertNotNull so that this could be > this.removeNode_(assertNotNull($(eventData.item_id))); > but I couldn't find it. Did I dream that up? this.removeNode_(getRequiredElement(eventData.item_id)) or assert(node); // top line of removeNode_ or just let removeNode_ explode
lgtm either way, but make the code as simple as possible
https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:286: this.removeNode_(node); On 2015/05/15 23:47:45, Dan Beam wrote: > On 2015/05/15 23:29:43, Devlin wrote: > > I thought there was something like assertNotNull so that this could be > > this.removeNode_(assertNotNull($(eventData.item_id))); > > but I couldn't find it. Did I dream that up? > > this.removeNode_(getRequiredElement(eventData.item_id)) > > or > > assert(node); // top line of removeNode_ > > or just let removeNode_ explode I like getRequiredElement. Gives us compile errors. :)
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1138973005/#ps20001 (title: "getRequiredElement")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138973005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0da16bdaa81d0952a73153e74c7889a273b75a3b Cr-Commit-Position: refs/heads/master@{#330547}
Message was sent while issue was closed.
https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:286: this.removeNode_(node); On 2015/05/19 16:29:50, Devlin wrote: > On 2015/05/15 23:47:45, Dan Beam wrote: > > On 2015/05/15 23:29:43, Devlin wrote: > > > I thought there was something like assertNotNull so that this could be > > > this.removeNode_(assertNotNull($(eventData.item_id))); > > > but I couldn't find it. Did I dream that up? > > > > this.removeNode_(getRequiredElement(eventData.item_id)) > > > > or > > > > assert(node); // top line of removeNode_ > > > > or just let removeNode_ explode > > I like getRequiredElement. Gives us compile errors. :) compile-time type filtering + runtime errors. assert() also does this magic, too, btw: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...
Message was sent while issue was closed.
https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:286: this.removeNode_(node); On 2015/05/19 21:09:33, Dan Beam wrote: > On 2015/05/19 16:29:50, Devlin wrote: > > On 2015/05/15 23:47:45, Dan Beam wrote: > > > On 2015/05/15 23:29:43, Devlin wrote: > > > > I thought there was something like assertNotNull so that this could be > > > > this.removeNode_(assertNotNull($(eventData.item_id))); > > > > but I couldn't find it. Did I dream that up? > > > > > > this.removeNode_(getRequiredElement(eventData.item_id)) > > > > > > or > > > > > > assert(node); // top line of removeNode_ > > > > > > or just let removeNode_ explode > > > > I like getRequiredElement. Gives us compile errors. :) > > compile-time type filtering + runtime errors. assert() also does this magic, > too, btw: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... (For my own edification) Hmm... If I make the param in removeNode an HTMLElement (i.e., remove the '!') and add an assert(node) as the first line, I don't get compile-time warnings for not using getRequiredElement() here, even though it could (for all the compiler knows) be null (that's why I preferred the getRequiredElement approach). Does compile-time type filtering include nullness with assert()?
Message was sent while issue was closed.
https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/1138973005/diff/1/chrome/browser/resources/ex... chrome/browser/resources/extensions/extension_list.js:286: this.removeNode_(node); On 2015/05/19 21:36:39, Devlin wrote: > On 2015/05/19 21:09:33, Dan Beam wrote: > > On 2015/05/19 16:29:50, Devlin wrote: > > > On 2015/05/15 23:47:45, Dan Beam wrote: > > > > On 2015/05/15 23:29:43, Devlin wrote: > > > > > I thought there was something like assertNotNull so that this could be > > > > > this.removeNode_(assertNotNull($(eventData.item_id))); > > > > > but I couldn't find it. Did I dream that up? > > > > > > > > this.removeNode_(getRequiredElement(eventData.item_id)) > > > > > > > > or > > > > > > > > assert(node); // top line of removeNode_ > > > > > > > > or just let removeNode_ explode > > > > > > I like getRequiredElement. Gives us compile errors. :) > > > > compile-time type filtering + runtime errors. assert() also does this magic, > > too, btw: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... > > (For my own edification) > > Hmm... If I make the param in removeNode an HTMLElement (i.e., remove the '!') > and add an assert(node) as the first line, I don't get compile-time warnings for > not using getRequiredElement() here, even though it could (for all the compiler > knows) be null (that's why I preferred the getRequiredElement approach). Does > compile-time type filtering include nullness with assert()? this.removeNode_(assert(node)); |
