Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
893453002:
Stop rebuilding all elements in chrome://extensions to preserve focus on refresh (Closed)
|
Created:
5 years, 10 months ago by hcarmona Modified:
5 years, 10 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. |
DescriptionStop rebuilding all elements in chrome://extensions to preserve focus on refresh
Refactored the ExtensionsList.createNode_ method into 2 methods:
ExtensionsList.createNode_ - Will be called once to create a node.
ExtensionsList.updateNode_ - Will be called to update an existing node.
BUG=450818
Committed: https://crrev.com/514b36babdf0e97f32b93eebe3122b17d3cfc2c4
Cr-Commit-Position: refs/heads/master@{#316120}
Patch Set 1 #Patch Set 2 : DO NOT COMMIT - Diff Baseline #Patch Set 3 : DO NOT COMMIT - Diff Changes #
Total comments: 15
Patch Set 4 : DO NOT COMMIT - Applied feedback #
Total comments: 15
Patch Set 5 : DO NOT COMMIT - Most recent feedback #Patch Set 6 : DNC - clean up enabled checkbox code #
Total comments: 9
Patch Set 7 : DNC - Apply feedback before merging back into 1 file #
Total comments: 8
Patch Set 8 : DNC - changes to updateNode_ #Patch Set 9 : Merged to single file #
Total comments: 38
Patch Set 10 : Apply Feedback #
Total comments: 16
Patch Set 11 : Compile JS and apply feedback #
Total comments: 7
Patch Set 12 : seenIds #Patch Set 13 : Closure + Indent #
Total comments: 4
Patch Set 14 : Remove unused line #Messages
Total messages: 57 (9 generated)
hcarmona@chromium.org changed reviewers: + kalman@chromium.org
Hi kalman, Please review this change. It's intended to fix an a11y issue where focus is lost when the extension list updates. With this change the elements in the extension list will be reused instead of recreated. Thanks
Yeah it'd be nice if chrome://extensions were more dynamic rather than rebuilding it each time. We've shied away from it due to complexity :-) QQ before I deep dive into this: (1) is the diff really that large, or was some code moved around and/or rietveld confused? could you split this up somehow or try to make the diff less daunting? (2) would a super quick hack to be to preserve focus before refresh then restore it after? not a nice solution.
On 2015/01/30 00:18:10, kalman wrote: > Yeah it'd be nice if chrome://extensions were more dynamic rather than > rebuilding it each time. We've shied away from it due to complexity :-) > > QQ before I deep dive into this: > > (1) is the diff really that large, or was some code moved around and/or rietveld > confused? could you split this up somehow or try to make the diff less daunting? Yes, the diff is not very useful because there was a kind of zipper effect when I separated the creation code from the update code. The best way to look at the diff would be with a 3-way diff (original createNode_, updated createNode_ and updateNode_). I can upload a patch that has the original createNode_'s code duplicated, this will make it easier to see what's changed in the two functions. > (2) would a super quick hack to be to preserve focus before refresh then restore > it after? not a nice solution. This could work, but the underlying issue would still be there and it might make future changes more fragile. No?
On 2015/01/30 02:20:15, Hector Carmona wrote: > On 2015/01/30 00:18:10, kalman wrote: > > Yeah it'd be nice if chrome://extensions were more dynamic rather than > > rebuilding it each time. We've shied away from it due to complexity :-) > > > > QQ before I deep dive into this: > > > > (1) is the diff really that large, or was some code moved around and/or > rietveld > > confused? could you split this up somehow or try to make the diff less > daunting? > > Yes, the diff is not very useful because there was a kind of zipper effect when > I > separated the creation code from the update code. The best way to look at the > diff > would be with a 3-way diff (original createNode_, updated createNode_ and > updateNode_). > > I can upload a patch that has the original createNode_'s code duplicated, this > will make it easier to see what's changed in the two functions. Something like that would be good. You could also do a no-op change (pulling something into a function, or moving code around, if possible) followed by the real change. > > > (2) would a super quick hack to be to preserve focus before refresh then > restore > > it after? not a nice solution. > > This could work, but the underlying issue would still be there and it might make > future changes more fragile. No? Yes it was a bad idea if the real solution isn't as complicated as a 600+ line diff :)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
On 2015/01/30 20:14:15, kalman wrote: > On 2015/01/30 02:20:15, Hector Carmona wrote: > > On 2015/01/30 00:18:10, kalman wrote: > > > Yeah it'd be nice if chrome://extensions were more dynamic rather than > > > rebuilding it each time. We've shied away from it due to complexity :-) > > > > > > QQ before I deep dive into this: > > > > > > (1) is the diff really that large, or was some code moved around and/or > > rietveld > > > confused? could you split this up somehow or try to make the diff less > > daunting? > > > > Yes, the diff is not very useful because there was a kind of zipper effect > when > > I > > separated the creation code from the update code. The best way to look at the > > diff > > would be with a 3-way diff (original createNode_, updated createNode_ and > > updateNode_). > > > > I can upload a patch that has the original createNode_'s code duplicated, this > > will make it easier to see what's changed in the two functions. > > Something like that would be good. You could also do a no-op change (pulling > something into a function, or moving code around, if possible) followed by the > real change. > > > > > > (2) would a super quick hack to be to preserve focus before refresh then > > restore > > > it after? not a nice solution. > > > > This could work, but the underlying issue would still be there and it might > make > > future changes more fragile. No? > > Yes it was a bad idea if the real solution isn't as complicated as a 600+ line > diff :) OK. This is a bit (a lot) hacky solution to get a better diff. Patch set 2 has a baseline to compare against. Both create_node.js and update_node.js contain just the one function and are identical (except for the function name). Patch set 3 has the same changes from Patch set 1, but split across the 3 files. We can do the code review on the separate files and I'll put them back into one file on the last patch set before committing.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
what's the hold up on this review?
Oops. (a) me forgetting, (b) it being a very hard patch to review, (c) not sending out where I was up to before the weekend. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:183: * Will update an element to show a list of errors. Updates an element... https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:189: // TODO(hcarmona): is it possible to make this non-destructive? What does this TODO mean? https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/update_node.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:27: node.id == this.getIdQueryParam_()); What was wrong with the old class setup code? https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:61: this.toggleEventListener_(showButton, extension.enable_show_button, Any reason why you need to toggle these listeners? Can you just show/hide the elements, the event listeners won't fire if they're hidden.
Review still WIP, that's where my starting got me last week.
*staring
Didn't upload a new CL (comments only) to avoid messing with the diff while the review is in progress. Just wanted to reply to the feedback. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:183: * Will update an element to show a list of errors. On 2015/02/03 17:41:18, kalman wrote: > Updates an element... Done. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:189: // TODO(hcarmona): is it possible to make this non-destructive? On 2015/02/03 17:41:18, kalman wrote: > What does this TODO mean? ExtensionErrorList doesn't have the ability to update. I put this TODO to remind me to look into adding that functionality or leave it as is and accept that errors will refresh. This CL is complicated enough that updating the ExtensionErrorList (if it's necessary) should be done in its own CL. I'll remove the TODO. Any thoughts on ExtensionErrorList? https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/update_node.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:27: node.id == this.getIdQueryParam_()); On 2015/02/03 17:41:18, kalman wrote: > What was wrong with the old class setup code? This code will toggle the classes because the node is being updated, while the previous code would only add to the classes. If we want to reuse the previous code we can remove the classes that can be added and then continue with previous logic that will add the necessary classes. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:61: this.toggleEventListener_(showButton, extension.enable_show_button, On 2015/02/03 17:41:18, kalman wrote: > Any reason why you need to toggle these listeners? Can you just show/hide the > elements, the event listeners won't fire if they're hidden. Previous logic only adds listeners when they're going to be used. I read that having a lot of unused listeners can make the code slow. If there's no performance overhead when the elements are hidden, then we can move all the listeners to the createNode_ function.
https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:189: // TODO(hcarmona): is it possible to make this non-destructive? On 2015/02/03 19:26:10, Hector Carmona wrote: > On 2015/02/03 17:41:18, kalman wrote: > > What does this TODO mean? > > ExtensionErrorList doesn't have the ability to update. I put this TODO to remind > me to look into adding that functionality or leave it as is and accept that > errors will refresh. This CL is complicated enough that updating the > ExtensionErrorList (if it's necessary) should be done in its own CL. > > I'll remove the TODO. Any thoughts on ExtensionErrorList? You don't necessarily need to remove the TODO, just clarify what "this" is. Your reply just now would be a good thing to write here. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/update_node.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:27: node.id == this.getIdQueryParam_()); > If we want to reuse the previous code we can remove the classes that can be > added and then continue with previous logic that will add the necessary classes. Yes I think that would be preferable, at least for now for the sake of reducing the diff in logic. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:61: this.toggleEventListener_(showButton, extension.enable_show_button, On 2015/02/03 19:26:10, Hector Carmona wrote: > On 2015/02/03 17:41:18, kalman wrote: > > Any reason why you need to toggle these listeners? Can you just show/hide the > > elements, the event listeners won't fire if they're hidden. > > Previous logic only adds listeners when they're going to be used. I read that > having a lot of unused listeners can make the code slow. If there's no > performance overhead when the elements are hidden, then we can move all the > listeners to the createNode_ function. Maybe Dan can comment on that, but I feel the complexity added to support this is non trivial, and we're talking about only a small number of listeners really (plus, it's no worse than how it was before, right..?)
https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/update_node.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:61: this.toggleEventListener_(showButton, extension.enable_show_button, On 2015/02/03 19:33:40, kalman wrote: > On 2015/02/03 19:26:10, Hector Carmona wrote: > > On 2015/02/03 17:41:18, kalman wrote: > > > Any reason why you need to toggle these listeners? Can you just show/hide > the > > > elements, the event listeners won't fire if they're hidden. > > > > Previous logic only adds listeners when they're going to be used. I read that > > having a lot of unused listeners can make the code slow. If there's no > > performance overhead when the elements are hidden, then we can move all the > > listeners to the createNode_ function. > > Maybe Dan can comment on that, but I feel the complexity added to support this > is non trivial, and we're talking about only a small number of listeners really > (plus, it's no worse than how it was before, right..?) i think unless there's a metrics-driven or anecdotal speed difference to back up this claim, just write the simplest code possible.
Applied feedback. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:189: // TODO(hcarmona): is it possible to make this non-destructive? On 2015/02/03 19:33:40, kalman wrote: > On 2015/02/03 19:26:10, Hector Carmona wrote: > > On 2015/02/03 17:41:18, kalman wrote: > > > What does this TODO mean? > > > > ExtensionErrorList doesn't have the ability to update. I put this TODO to > remind > > me to look into adding that functionality or leave it as is and accept that > > errors will refresh. This CL is complicated enough that updating the > > ExtensionErrorList (if it's necessary) should be done in its own CL. > > > > I'll remove the TODO. Any thoughts on ExtensionErrorList? > > You don't necessarily need to remove the TODO, just clarify what "this" is. Your > reply just now would be a good thing to write here. Ok, updated TODO with info from this conversation. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/update_node.js (right): https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:27: node.id == this.getIdQueryParam_()); On 2015/02/03 19:33:40, kalman wrote: > > > If we want to reuse the previous code we can remove the classes that can be > > added and then continue with previous logic that will add the necessary > classes. > > Yes I think that would be preferable, at least for now for the sake of reducing > the diff in logic. Done. https://codereview.chromium.org/893453002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/update_node.js:61: this.toggleEventListener_(showButton, extension.enable_show_button, On 2015/02/03 19:35:06, Dan Beam wrote: > On 2015/02/03 19:33:40, kalman wrote: > > On 2015/02/03 19:26:10, Hector Carmona wrote: > > > On 2015/02/03 17:41:18, kalman wrote: > > > > Any reason why you need to toggle these listeners? Can you just show/hide > > the > > > > elements, the event listeners won't fire if they're hidden. > > > > > > Previous logic only adds listeners when they're going to be used. I read > that > > > having a lot of unused listeners can make the code slow. If there's no > > > performance overhead when the elements are hidden, then we can move all the > > > listeners to the createNode_ function. > > > > Maybe Dan can comment on that, but I feel the complexity added to support this > > is non trivial, and we're talking about only a small number of listeners > really > > (plus, it's no worse than how it was before, right..?) > > i think unless there's a metrics-driven or anecdotal speed difference to back up > this claim, just write the simplest code possible. Page feels snappier (compared to stable) when toggling between developer and normal mode when there are several extensions due to not rebuilding the list. Didn't notice any difference between toggling the event listener. Code is less complicated now. :-)
I won't be able to look at this until tomorrow morning - but for then, could you remind me which diff(s) contains the relevant patch? I've lost track.
Never mind that last question.
wdyt? before I pick through all of these lines I'd like to get your thoughts. https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:27: butterBar.hidden = !checked || extension.is_hosted_app; This butterbar visibility logic should be in update_node.js, because there's a bug where: 1. Open up 2 chrome://extensions tabs 2. Click "allow in incognito" in one of them (butterbar shows) 3. Go to the other, deselect "allow in incognito" 4. Go back to the original; the butterbar is still there, even though it's not enabled in incognito anymore. The existing code has this bug too. And, fixing this by moving it into update_node.js will introduce a different bug: apparently the desired behavior is only to show the butterbar the first time you click it, and never after. I'm dubious of whether this is actually a good feature, but let's say it is - a good way to fix this would be to set some flag here like "didEnableIncognito_" to true here, then check that in update_node.js and only show the butterbar if that was true. https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:112: // left unchanged. This (existing) code is weird, I wonder why it doesn't use the |enable| node to determine check value, but instead tries to be smart and use e.target? I.e. seems like you could remove lines 110-116 and change it to chrome.send('extensionSettingsEnable', String(enable.checked)); https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/update_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:60: descriptionHolder.appendChild(description); Any reason why this isn't all just "descriptionHolder.textContent = extension.description", and therefore, why the "Holder" suffix is necessary? https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:63: var showButton = node.querySelector('.show-button'); General function-level comment, which applies to create_node.js as well: (a) it'd be nice to inline some of these "var showButton..." type calls. Saving a reference to the variable is nice when you're doing a bunch with them, but now it's split over 2 functions, so less value. (b) consider creating an alias for node.querySelector or even node.getElementByClassname[0] which is what these basically all are. Like how $ works, but obviously, not $. Hm, though in this class, you could shorten things a bit (not need to continually check the same condition) and generally get a better diff (lacking all of this outdenting) with something like: function update(selector, condition, callback) { var selectedNode = node.querySelector(selector); selectedNode.hidden = condition; if (condition) callback(selectedNode); } then you'd see code like: update('.show-button', extension.enable_show_button); // ok, bad example: update('.incognito-enabled', this.data_.incognitoAvailable, function(incognitoEnabled) { incognitoEnabled.disabled = !extension...; }); // better example, as are most others: update('.error-collection-control', extension.wantsErrorCollection, function(errorCollection) { errorCollection.checked = extension.errorCollectionEnabled; }); An additional advantage of that is you wouldn't be doing unnecessary work if you're not showing the node.
> function update(selector, condition, callback) {
> var selectedNode = node.querySelector(selector);
> selectedNode.hidden = condition;
selectedNode.hidden = !condition;
> if (condition) callback(selectedNode);
> }
Replying to feedback, I'll upload a CL with changes when they're complete. https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:27: butterBar.hidden = !checked || extension.is_hosted_app; On 2015/02/05 22:25:03, kalman wrote: > This butterbar visibility logic should be in update_node.js, because there's a > bug where: > 1. Open up 2 chrome://extensions tabs > 2. Click "allow in incognito" in one of them (butterbar shows) > 3. Go to the other, deselect "allow in incognito" > 4. Go back to the original; the butterbar is still there, even though it's not > enabled in incognito anymore. > > The existing code has this bug too. And, fixing this by moving it into > update_node.js will introduce a different bug: apparently the desired behavior > is only to show the butterbar the first time you click it, and never after. > > I'm dubious of whether this is actually a good feature, but let's say it is - a > good way to fix this would be to set some flag here like "didEnableIncognito_" > to true here, then check that in update_node.js and only show the butterbar if > that was true. There's already code in update_node.js to update the butterbar visibility. We can change it to also make sure that Allow in incognito is set for the extension and use the flag to only change this value once. But this will still show the butterbar if you open the page again and click the incognito checkbox. If it needs to be more permanent, then that should be a different CL. https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:112: // left unchanged. On 2015/02/05 22:25:03, kalman wrote: > This (existing) code is weird, I wonder why it doesn't use the |enable| node to > determine check value, but instead tries to be smart and use e.target? I.e. > seems like you could remove lines 110-116 and change it to > chrome.send('extensionSettingsEnable', String(enable.checked)); enable is a div. if the click event happens on the label it sends !checkbox.checked because the checkbox won't have toggled and if the target is the checkbox then the value of checkbox.checked is used because it's already been toggled. https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/update_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:60: descriptionHolder.appendChild(description); On 2015/02/05 22:25:03, kalman wrote: > Any reason why this isn't all just "descriptionHolder.textContent = > extension.description", and therefore, why the "Holder" suffix is necessary? This was done to avoid changing the structure of the elements. I have no objections to changing it. https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:63: var showButton = node.querySelector('.show-button'); On 2015/02/05 22:25:03, kalman wrote: > General function-level comment, which applies to create_node.js as well: > > (a) it'd be nice to inline some of these "var showButton..." type calls. Saving > a reference to the variable is nice when you're doing a bunch with them, but now > it's split over 2 functions, so less value. > > (b) consider creating an alias for node.querySelector or even > node.getElementByClassname[0] which is what these basically all are. > > Like how $ works, but obviously, not $. > > Hm, though in this class, you could shorten things a bit (not need to > continually check the same condition) and generally get a better diff (lacking > all of this outdenting) with something like: > > function update(selector, condition, callback) { > var selectedNode = node.querySelector(selector); > selectedNode.hidden = condition; > if (condition) callback(selectedNode); > } > > then you'd see code like: > > update('.show-button', extension.enable_show_button); > // ok, bad example: > update('.incognito-enabled', > this.data_.incognitoAvailable, > function(incognitoEnabled) { > incognitoEnabled.disabled = !extension...; > }); > // better example, as are most others: > update('.error-collection-control', > extension.wantsErrorCollection, > function(errorCollection) { > errorCollection.checked = extension.errorCollectionEnabled; > }); > > An additional advantage of that is you wouldn't be doing unnecessary work if > you're not showing the node. Awesome, I'll create an update function.
https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:27: butterBar.hidden = !checked || extension.is_hosted_app; On 2015/02/05 23:27:14, Hector Carmona wrote: > On 2015/02/05 22:25:03, kalman wrote: > > This butterbar visibility logic should be in update_node.js, because there's a > > bug where: > > 1. Open up 2 chrome://extensions tabs > > 2. Click "allow in incognito" in one of them (butterbar shows) > > 3. Go to the other, deselect "allow in incognito" > > 4. Go back to the original; the butterbar is still there, even though it's not > > enabled in incognito anymore. > > > > The existing code has this bug too. And, fixing this by moving it into > > update_node.js will introduce a different bug: apparently the desired behavior > > is only to show the butterbar the first time you click it, and never after. > > > > I'm dubious of whether this is actually a good feature, but let's say it is - > a > > good way to fix this would be to set some flag here like "didEnableIncognito_" > > to true here, then check that in update_node.js and only show the butterbar if > > that was true. > > There's already code in update_node.js to update the butterbar visibility. We > can change it to also make sure that Allow in incognito is set for the extension > and use the flag to only change this value once. But this will still show the > butterbar if you open the page again and click the incognito checkbox. If it > needs to be more permanent, then that should be a different CL. I tried this with your patch, and nothing seems to have changed. Another way to phrase my comment is: this display logic belongs in update_node.js, not here, however you'll need some special code to maintain the behavior where it only shows once. https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:112: // left unchanged. On 2015/02/05 23:27:14, Hector Carmona wrote: > On 2015/02/05 22:25:03, kalman wrote: > > This (existing) code is weird, I wonder why it doesn't use the |enable| node > to > > determine check value, but instead tries to be smart and use e.target? I.e. > > seems like you could remove lines 110-116 and change it to > > chrome.send('extensionSettingsEnable', String(enable.checked)); > > enable is a div. if the click event happens on the label it sends > !checkbox.checked because the checkbox won't have toggled and if the target is > the checkbox then the value of checkbox.checked is used because it's already > been toggled. You've lost me, but I think that if you change the selector to '.enable-checkbox input' then what I said is correct.
> > > This (existing) code is weird, I wonder why it doesn't use the |enable|
node
> > to
> > > determine check value, but instead tries to be smart and use e.target?
I.e.
> > > seems like you could remove lines 110-116 and change it to
> > > chrome.send('extensionSettingsEnable', String(enable.checked));
> >
> > enable is a div. if the click event happens on the label it sends
> > !checkbox.checked because the checkbox won't have toggled and if the target
is
> > the checkbox then the value of checkbox.checked is used because it's already
> > been toggled.
>
> You've lost me, but I think that if you change the selector to
'.enable-checkbox
> input' then what I said is correct.
The HTML code that this is defined in is confusingly formatted:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
FWIW the <label> is hidden away behind the <div> declaration. If it were on the
next line, and correctly formatted, that would be easier to read.
https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:112: // left unchanged. On 2015/02/05 23:34:06, kalman wrote: > On 2015/02/05 23:27:14, Hector Carmona wrote: > > On 2015/02/05 22:25:03, kalman wrote: > > > This (existing) code is weird, I wonder why it doesn't use the |enable| node > > to > > > determine check value, but instead tries to be smart and use e.target? I.e. > > > seems like you could remove lines 110-116 and change it to > > > chrome.send('extensionSettingsEnable', String(enable.checked)); > > > > enable is a div. if the click event happens on the label it sends > > !checkbox.checked because the checkbox won't have toggled and if the target is > > the checkbox then the value of checkbox.checked is used because it's already > > been toggled. > > You've lost me, but I think that if you change the selector to '.enable-checkbox > input' then what I said is correct. What is said is wrong. You should be checking enabled.querySelector('input').checked, as is what is basically already being done, but it's too complicated bringing e.target into it. (and yep - s/you/previous author/)
https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:27: butterBar.hidden = !checked || extension.is_hosted_app; On 2015/02/05 23:34:06, kalman wrote: > On 2015/02/05 23:27:14, Hector Carmona wrote: > > On 2015/02/05 22:25:03, kalman wrote: > > > This butterbar visibility logic should be in update_node.js, because there's > a > > > bug where: > > > 1. Open up 2 chrome://extensions tabs > > > 2. Click "allow in incognito" in one of them (butterbar shows) > > > 3. Go to the other, deselect "allow in incognito" > > > 4. Go back to the original; the butterbar is still there, even though it's > not > > > enabled in incognito anymore. > > > > > > The existing code has this bug too. And, fixing this by moving it into > > > update_node.js will introduce a different bug: apparently the desired > behavior > > > is only to show the butterbar the first time you click it, and never after. > > > > > > I'm dubious of whether this is actually a good feature, but let's say it is > - > > a > > > good way to fix this would be to set some flag here like > "didEnableIncognito_" > > > to true here, then check that in update_node.js and only show the butterbar > if > > > that was true. > > > > There's already code in update_node.js to update the butterbar visibility. We > > can change it to also make sure that Allow in incognito is set for the > extension > > and use the flag to only change this value once. But this will still show the > > butterbar if you open the page again and click the incognito checkbox. If it > > needs to be more permanent, then that should be a different CL. > > I tried this with your patch, and nothing seems to have changed. Another way to > phrase my comment is: this display logic belongs in update_node.js, not here, > however you'll need some special code to maintain the behavior where it only > shows once. This new patch has this working so the butterbar shows once and goes away when updated. https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:112: // left unchanged. On 2015/02/05 23:39:35, kalman wrote: > On 2015/02/05 23:34:06, kalman wrote: > > On 2015/02/05 23:27:14, Hector Carmona wrote: > > > On 2015/02/05 22:25:03, kalman wrote: > > > > This (existing) code is weird, I wonder why it doesn't use the |enable| > node > > > to > > > > determine check value, but instead tries to be smart and use e.target? > I.e. > > > > seems like you could remove lines 110-116 and change it to > > > > chrome.send('extensionSettingsEnable', String(enable.checked)); > > > > > > enable is a div. if the click event happens on the label it sends > > > !checkbox.checked because the checkbox won't have toggled and if the target > is > > > the checkbox then the value of checkbox.checked is used because it's already > > > been toggled. > > > > You've lost me, but I think that if you change the selector to > '.enable-checkbox > > input' then what I said is correct. > > What is said is wrong. You should be checking > enabled.querySelector('input').checked, as is what is basically already being > done, but it's too complicated bringing e.target into it. > > (and yep - s/you/previous author/) It's not possible to rely on the value of the checkbox because it should also be possible to click on the label to make the checkbox toggle. Clicking on the label doesn't toggle the value of the checkbox.
https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:112: // left unchanged. On 2015/02/06 02:25:10, Hector Carmona wrote: > On 2015/02/05 23:39:35, kalman wrote: > > On 2015/02/05 23:34:06, kalman wrote: > > > On 2015/02/05 23:27:14, Hector Carmona wrote: > > > > On 2015/02/05 22:25:03, kalman wrote: > > > > > This (existing) code is weird, I wonder why it doesn't use the |enable| > > node > > > > to > > > > > determine check value, but instead tries to be smart and use e.target? > > I.e. > > > > > seems like you could remove lines 110-116 and change it to > > > > > chrome.send('extensionSettingsEnable', String(enable.checked)); > > > > > > > > enable is a div. if the click event happens on the label it sends > > > > !checkbox.checked because the checkbox won't have toggled and if the > target > > is > > > > the checkbox then the value of checkbox.checked is used because it's > already > > > > been toggled. > > > > > > You've lost me, but I think that if you change the selector to > > '.enable-checkbox > > > input' then what I said is correct. > > > > What is said is wrong. You should be checking > > enabled.querySelector('input').checked, as is what is basically already being > > done, but it's too complicated bringing e.target into it. > > > > (and yep - s/you/previous author/) > > It's not possible to rely on the value of the checkbox because it should also be > possible to click on the label to make the checkbox toggle. Clicking on the > label doesn't toggle the value of the checkbox. I just tried this and clicking on the label toggles the value of the checkbox.
https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:112: // left unchanged. On 2015/02/06 15:48:00, kalman wrote: > On 2015/02/06 02:25:10, Hector Carmona wrote: > > On 2015/02/05 23:39:35, kalman wrote: > > > On 2015/02/05 23:34:06, kalman wrote: > > > > On 2015/02/05 23:27:14, Hector Carmona wrote: > > > > > On 2015/02/05 22:25:03, kalman wrote: > > > > > > This (existing) code is weird, I wonder why it doesn't use the > |enable| > > > node > > > > > to > > > > > > determine check value, but instead tries to be smart and use e.target? > > > I.e. > > > > > > seems like you could remove lines 110-116 and change it to > > > > > > chrome.send('extensionSettingsEnable', String(enable.checked)); > > > > > > > > > > enable is a div. if the click event happens on the label it sends > > > > > !checkbox.checked because the checkbox won't have toggled and if the > > target > > > is > > > > > the checkbox then the value of checkbox.checked is used because it's > > already > > > > > been toggled. > > > > > > > > You've lost me, but I think that if you change the selector to > > > '.enable-checkbox > > > > input' then what I said is correct. > > > > > > What is said is wrong. You should be checking > > > enabled.querySelector('input').checked, as is what is basically already > being > > > done, but it's too complicated bringing e.target into it. > > > > > > (and yep - s/you/previous author/) > > > > It's not possible to rely on the value of the checkbox because it should also > be > > possible to click on the label to make the checkbox toggle. Clicking on the > > label doesn't toggle the value of the checkbox. > > I just tried this and clicking on the label toggles the value of the checkbox. I'm getting a bit confused now. Current functionality allows clicking either the label or checkbox. When I changed this locally, it doesn't toggle on the label. This is my local change, that only works on the checkbox: var checked = node.querySelector('.enable-checkbox input').checked; chrome.send('extensionSettingsEnable', [extension.id, String(checked)]); perhaps I'm not understanding the change you're suggesting
On 2015/02/06 23:41:28, Hector Carmona wrote: > https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... > File chrome/browser/resources/extensions/create_node.js (right): > > https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... > chrome/browser/resources/extensions/create_node.js:112: // left unchanged. > On 2015/02/06 15:48:00, kalman wrote: > > On 2015/02/06 02:25:10, Hector Carmona wrote: > > > On 2015/02/05 23:39:35, kalman wrote: > > > > On 2015/02/05 23:34:06, kalman wrote: > > > > > On 2015/02/05 23:27:14, Hector Carmona wrote: > > > > > > On 2015/02/05 22:25:03, kalman wrote: > > > > > > > This (existing) code is weird, I wonder why it doesn't use the > > |enable| > > > > node > > > > > > to > > > > > > > determine check value, but instead tries to be smart and use > e.target? > > > > I.e. > > > > > > > seems like you could remove lines 110-116 and change it to > > > > > > > chrome.send('extensionSettingsEnable', String(enable.checked)); > > > > > > > > > > > > enable is a div. if the click event happens on the label it sends > > > > > > !checkbox.checked because the checkbox won't have toggled and if the > > > target > > > > is > > > > > > the checkbox then the value of checkbox.checked is used because it's > > > already > > > > > > been toggled. > > > > > > > > > > You've lost me, but I think that if you change the selector to > > > > '.enable-checkbox > > > > > input' then what I said is correct. > > > > > > > > What is said is wrong. You should be checking > > > > enabled.querySelector('input').checked, as is what is basically already > > being > > > > done, but it's too complicated bringing e.target into it. > > > > > > > > (and yep - s/you/previous author/) > > > > > > It's not possible to rely on the value of the checkbox because it should > also > > be > > > possible to click on the label to make the checkbox toggle. Clicking on the > > > label doesn't toggle the value of the checkbox. > > > > I just tried this and clicking on the label toggles the value of the checkbox. > > I'm getting a bit confused now. Current functionality allows clicking either the > label or checkbox. > > When I changed this locally, it doesn't toggle on the label. > This is my local change, that only works on the checkbox: > > var checked = node.querySelector('.enable-checkbox input').checked; > chrome.send('extensionSettingsEnable', [extension.id, String(checked)]); you still need the e.preventDefault() but yes, that. What doesn't work? Generally speaking if you have a label for a checkbox, and you click either the label or the checkbox, the checkbox will toggle. And, within that event, the checkbox will correctly set the 'checked' property (I verified this in a test page). > > perhaps I'm not understanding the change you're suggesting
On 2015/02/06 23:46:06, kalman wrote: > On 2015/02/06 23:41:28, Hector Carmona wrote: > > > https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... > > File chrome/browser/resources/extensions/create_node.js (right): > > > > > https://codereview.chromium.org/893453002/diff/100001/chrome/browser/resource... > > chrome/browser/resources/extensions/create_node.js:112: // left unchanged. > > On 2015/02/06 15:48:00, kalman wrote: > > > On 2015/02/06 02:25:10, Hector Carmona wrote: > > > > On 2015/02/05 23:39:35, kalman wrote: > > > > > On 2015/02/05 23:34:06, kalman wrote: > > > > > > On 2015/02/05 23:27:14, Hector Carmona wrote: > > > > > > > On 2015/02/05 22:25:03, kalman wrote: > > > > > > > > This (existing) code is weird, I wonder why it doesn't use the > > > |enable| > > > > > node > > > > > > > to > > > > > > > > determine check value, but instead tries to be smart and use > > e.target? > > > > > I.e. > > > > > > > > seems like you could remove lines 110-116 and change it to > > > > > > > > chrome.send('extensionSettingsEnable', String(enable.checked)); > > > > > > > > > > > > > > enable is a div. if the click event happens on the label it sends > > > > > > > !checkbox.checked because the checkbox won't have toggled and if the > > > > target > > > > > is > > > > > > > the checkbox then the value of checkbox.checked is used because it's > > > > already > > > > > > > been toggled. > > > > > > > > > > > > You've lost me, but I think that if you change the selector to > > > > > '.enable-checkbox > > > > > > input' then what I said is correct. > > > > > > > > > > What is said is wrong. You should be checking > > > > > enabled.querySelector('input').checked, as is what is basically already > > > being > > > > > done, but it's too complicated bringing e.target into it. > > > > > > > > > > (and yep - s/you/previous author/) > > > > > > > > It's not possible to rely on the value of the checkbox because it should > > also > > > be > > > > possible to click on the label to make the checkbox toggle. Clicking on > the > > > > label doesn't toggle the value of the checkbox. > > > > > > I just tried this and clicking on the label toggles the value of the > checkbox. > > > > I'm getting a bit confused now. Current functionality allows clicking either > the > > label or checkbox. > > > > When I changed this locally, it doesn't toggle on the label. > > This is my local change, that only works on the checkbox: > > > > var checked = node.querySelector('.enable-checkbox input').checked; > > chrome.send('extensionSettingsEnable', [extension.id, String(checked)]); > > you still need the e.preventDefault() but yes, that. What doesn't work? > Generally speaking if you have a label for a checkbox, and you click either the > label or the checkbox, the checkbox will toggle. And, within that event, the > checkbox will correctly set the 'checked' property (I verified this in a test > page). > > > > > perhaps I'm not understanding the change you're suggesting Updated the code that handles the enabled checkbox click. I changed the element that the event was listening to from the div to the checkbox. The problem was that when e.target is the label and preventDefault is called it stops the checkbox from handling the event.
LGTM, let me have one last look at the merged files though. https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:121: butterBarVisibility[extension.id] = false; Is this necessary? I would hope that uninstalling the extension would remove its entire entry in chrome://extensions, including the butterbar. https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:203: * is visible. Mention that the Element it's called with is the item that matched? https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:206: updateElement_: function(node, query, visible, callback) { A more descriptive name would be updateVisibility or similar. https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:207: var item = node.querySelector(query); Can you assert that the element existed?
https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:121: butterBarVisibility[extension.id] = false; On 2015/02/10 22:14:37, kalman wrote: > Is this necessary? I would hope that uninstalling the extension would remove its > entire entry in chrome://extensions, including the butterbar. I guess if you install the extension again you don't want the butterbar showing up again. Ok. I'd go with "delete butterBarVisibility[extension.id]" personally.
New patchsets have been uploaded after l-g-t-m from kalman@chromium.org
Found some issues with the updateNode_ method when merging and rebasing since I was comparing everything to the original. Patch set 8 has the changes I had to make to updateNode_ for easy diff. I commented on each change, please provide feedback if anything doesn't look quite right. Also running the trybots since it's all one file again. https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/create_node.js (right): https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/create_node.js:121: butterBarVisibility[extension.id] = false; On 2015/02/10 22:15:27, kalman wrote: > On 2015/02/10 22:14:37, kalman wrote: > > Is this necessary? I would hope that uninstalling the extension would remove > its > > entire entry in chrome://extensions, including the butterbar. > > I guess if you install the extension again you don't want the butterbar showing > up again. Ok. I'd go with "delete butterBarVisibility[extension.id]" personally. Got rid of butterBarVisibility since it holds the same value as !butterBar.hidden. It's not necessary to keep track of the butterbar for the individual extensions since it should only show once. https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:203: * is visible. On 2015/02/10 22:14:37, kalman wrote: > Mention that the Element it's called with is the item that matched? Done. https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:206: updateElement_: function(node, query, visible, callback) { On 2015/02/10 22:14:37, kalman wrote: > A more descriptive name would be updateVisibility or similar. Done. https://codereview.chromium.org/893453002/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:207: var item = node.querySelector(query); On 2015/02/10 22:14:37, kalman wrote: > Can you assert that the element existed? Done. https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/update_node.js (right): https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:133: item.querySelector('input').checked = isOK && extension.enabled; isOK is redundant. Removed it in merged version. https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:136: // Button for extensions controlled by policy. This was confusing when reviewing changes. https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:160: function() { Changed in merged version: function(item) { https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:167: this.updateVisibility_(node, '.managed-message', isRequired); Changed the condition to prevent from overriding further down: isRequired && !extension.updateRequiredByPolicy https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:184: this.updateVisibility_(node, '.managed-message', This was overriding the hidden value of '.managed-message'. Removed this from merged version and updated above. https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:198: depNode.querySelector('.dep-extension-title').textContent = This fits in 1 line again. https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:251: if (extension.warnings) { if (extension.warnings) will always be true. Removed check. https://codereview.chromium.org/893453002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/update_node.js:271: var installWarningList = installWarningPanel.querySelector('ul'); installWarningPanel -> item since installWarningPanel is not defined anywhere
Just that one last comment. I'd feel more comfortable if Dan also lgtm'd. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].classList.add('to-remove'); It seems odd to be modifying a node's class to mark it for deletion; at least, how about setting it on the JS wrapper itself? I.e. nodes[i].toRemove = true. It looks simpler to me than that though. The only time 'to-remove' is added is here, and the only time it's removed is in updateNode_. You could write it as: var seenIds = []; this.data_.extensions.forEach(function(extension) { var node = $(extension.id); seenIds.push(extension.id); ... }); var nodes = document.querySelectorAll('.extension-list-item-wrapper'); // Note: calling slice() to take a copy of |nodes|, since NodeLists // are live. [].slice.call(nodes).forEach(function(node) { if (seenIds.indexOf(node.getAttribute('id') == 0) node.parentNode.removeChild(node); }); You could even use a Set() for seenIds if you feel inclined. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:144: node.parentElement.removeChild(node); I think this is a bug. A NodeList (which is what querySelectorAll returns) is live, so if you remove an element from the DOM it removes it from the list as well (i.e. if you have 2 adjacent nodes which need to be removed - highly unlikely - only the first will be). This is fixed with the code above.
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:144: node.parentElement.removeChild(node); On 2015/02/11 19:25:35, kalman wrote: > I think this is a bug. A NodeList (which is what querySelectorAll returns) is > live, so if you remove an element from the DOM it removes it from the list as > well (i.e. if you have 2 adjacent nodes which need to be removed - highly > unlikely - only the first will be). This is fixed with the code above. Ok, apparenetly querySelectorAll returns a "static NodeList" in this case, so you're good, and don't need to take the .slice() copy first.
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:140: '.extension-list-item-wrapper.to-remove'); .extension-list-item-wrapper.to-remove[id] https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:143: if (node.id && node.parentElement) document.qSA() doesn't work without a parentElement, if I had to guess https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:233: node.querySelector('.options-link').setAttribute( nit: `.href =` is shorter https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:273: this.addListener_('click', node, '.enable-checkbox input', function(e) { why click and not 'change'? https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:297: this.addListener_('click', node, '.load-path a:nth-of-type(1)', :first-of-type https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:319: node.classList.remove.apply(node.classList, toRemove); node.classList.remove('policy-controlled', 'may-not-modify', 'may-not-remove'); https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:466: item.querySelector('a:nth-of-type(1)').textContent = :first-of-type https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:603: addListener_: function(type, node, query, onclick) { onclick -> handler https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:614: setTextContent_: function(node, query, textContent) { nit: setText_ https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:624: * is visible. |item| is the element specified by |query|. this is kind of an odd API... https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:627: updateVisibility_: function(node, query, visible, callback) { nit: showingCallback or shownCallback or something https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:628: var item = node.querySelector(query); nit: var item = assert(node.querySelector(query)); https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:644: panel.hidden = !errors; is this meant to only hide the pane if |errors| is null or if errors.length == 0 as well? https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:646: if (errors) i think this will work when errors == []. do you want that?
New patchsets have been uploaded after l-g-t-m from kalman@chromium.org
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].classList.add('to-remove'); On 2015/02/11 19:25:35, kalman wrote: > It seems odd to be modifying a node's class to mark it for deletion; at least, > how about setting it on the JS wrapper itself? I.e. nodes[i].toRemove = true. > > It looks simpler to me than that though. The only time 'to-remove' is added is > here, and the only time it's removed is in updateNode_. You could write it as: > > var seenIds = []; > > this.data_.extensions.forEach(function(extension) { > var node = $(extension.id); > seenIds.push(extension.id); > ... > }); > > var nodes = document.querySelectorAll('.extension-list-item-wrapper'); > // Note: calling slice() to take a copy of |nodes|, since NodeLists > // are live. > [].slice.call(nodes).forEach(function(node) { > if (seenIds.indexOf(node.getAttribute('id') == 0) > node.parentNode.removeChild(node); > }); > > You could even use a Set() for seenIds if you feel inclined. Moving to the JS wrapper simplifies the code so we don't need to update |toRemove| in |updateNode_|. It makes more sense to only check the list of pre-existing nodes. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:140: '.extension-list-item-wrapper.to-remove'); On 2015/02/11 20:09:43, Dan Beam wrote: > .extension-list-item-wrapper.to-remove[id] Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:143: if (node.id && node.parentElement) On 2015/02/11 20:09:43, Dan Beam wrote: > document.qSA() doesn't work without a parentElement, if I had to guess Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:144: node.parentElement.removeChild(node); On 2015/02/11 19:29:30, kalman wrote: > On 2015/02/11 19:25:35, kalman wrote: > > I think this is a bug. A NodeList (which is what querySelectorAll returns) is > > live, so if you remove an element from the DOM it removes it from the list as > > well (i.e. if you have 2 adjacent nodes which need to be removed - highly > > unlikely - only the first will be). This is fixed with the code above. > > Ok, apparenetly querySelectorAll returns a "static NodeList" in this case, so > you're good, and don't need to take the .slice() copy first. Acknowledged. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:233: node.querySelector('.options-link').setAttribute( On 2015/02/11 20:09:43, Dan Beam wrote: > nit: `.href =` is shorter Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:273: this.addListener_('click', node, '.enable-checkbox input', function(e) { On 2015/02/11 20:09:43, Dan Beam wrote: > why click and not 'change'? Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:297: this.addListener_('click', node, '.load-path a:nth-of-type(1)', On 2015/02/11 20:09:43, Dan Beam wrote: > :first-of-type Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:319: node.classList.remove.apply(node.classList, toRemove); On 2015/02/11 20:09:43, Dan Beam wrote: > node.classList.remove('policy-controlled', 'may-not-modify', 'may-not-remove'); Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:466: item.querySelector('a:nth-of-type(1)').textContent = On 2015/02/11 20:09:43, Dan Beam wrote: > :first-of-type Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:603: addListener_: function(type, node, query, onclick) { On 2015/02/11 20:09:43, Dan Beam wrote: > onclick -> handler Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:614: setTextContent_: function(node, query, textContent) { On 2015/02/11 20:09:43, Dan Beam wrote: > nit: setText_ Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:624: * is visible. |item| is the element specified by |query|. On 2015/02/11 20:09:43, Dan Beam wrote: > this is kind of an odd API... Should we rename it? https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:627: updateVisibility_: function(node, query, visible, callback) { On 2015/02/11 20:09:43, Dan Beam wrote: > nit: showingCallback or shownCallback or something Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:628: var item = node.querySelector(query); On 2015/02/11 20:09:43, Dan Beam wrote: > nit: var item = assert(node.querySelector(query)); Done. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:644: panel.hidden = !errors; On 2015/02/11 20:09:43, Dan Beam wrote: > is this meant to only hide the pane if |errors| is null or if errors.length == 0 > as well? Acknowledged. https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:646: if (errors) On 2015/02/11 20:09:42, Dan Beam wrote: > i think this will work when errors == []. do you want that? Changed to hide the panels if there's a list of 0 errors as well, since it doesn't make sense to create an ExtensionErrorList to show no errors.
https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].toRemove = true; .extensionStillInstalled_ = false https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:134: node.toRemove = false; .extensionStillInstalled_ = true https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:139: // Remove any node that was not updated. // Remove extensions that are no longer installed. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:142: if (node.toRemove) if (!node.extensionStillInstalled_) https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:452: controlNode.removeChild(indicator); nit: curlies https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:596: * @param {function({Event} e)} handler The function that should be called nit: is this valid syntax? i would think function(Event). maybe try compiling locally https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:621: * @param {function({Element} item)} shownCallback shownCallback if the too much search and replace (second showCallback -> Callback) https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:641: panel.hidden = !errors; [] is truthy, this needs to be !errors || errors.length == 0
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].classList.add('to-remove'); On 2015/02/11 23:36:47, Hector Carmona wrote: > On 2015/02/11 19:25:35, kalman wrote: > > It seems odd to be modifying a node's class to mark it for deletion; at least, > > how about setting it on the JS wrapper itself? I.e. nodes[i].toRemove = true. > > > > It looks simpler to me than that though. The only time 'to-remove' is added is > > here, and the only time it's removed is in updateNode_. You could write it as: > > > > var seenIds = []; > > > > this.data_.extensions.forEach(function(extension) { > > var node = $(extension.id); > > seenIds.push(extension.id); > > ... > > }); > > > > var nodes = document.querySelectorAll('.extension-list-item-wrapper'); > > // Note: calling slice() to take a copy of |nodes|, since NodeLists > > // are live. > > [].slice.call(nodes).forEach(function(node) { > > if (seenIds.indexOf(node.getAttribute('id') == 0) > > node.parentNode.removeChild(node); > > }); > > > > You could even use a Set() for seenIds if you feel inclined. > > Moving to the JS wrapper simplifies the code so we don't need to update > |toRemove| in |updateNode_|. It makes more sense to only check the list of > pre-existing nodes. Alright. Well, I prefer the version which doesn't have this extra variable hanging off the node, regardless of whether it's pure JS or a class. Is it something to do with only checking the list of pre-existing nodes...? There's going to be at most 1 more node than before, so it's hardly any different, and less code the way I suggested. Eh.
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].classList.add('to-remove'); On 2015/02/12 01:04:50, kalman wrote: > On 2015/02/11 23:36:47, Hector Carmona wrote: > > On 2015/02/11 19:25:35, kalman wrote: > > > It seems odd to be modifying a node's class to mark it for deletion; at > least, > > > how about setting it on the JS wrapper itself? I.e. nodes[i].toRemove = > true. > > > > > > It looks simpler to me than that though. The only time 'to-remove' is added > is > > > here, and the only time it's removed is in updateNode_. You could write it > as: > > > > > > var seenIds = []; > > > > > > this.data_.extensions.forEach(function(extension) { > > > var node = $(extension.id); > > > seenIds.push(extension.id); > > > ... > > > }); > > > > > > var nodes = document.querySelectorAll('.extension-list-item-wrapper'); > > > // Note: calling slice() to take a copy of |nodes|, since NodeLists > > > // are live. > > > [].slice.call(nodes).forEach(function(node) { > > > if (seenIds.indexOf(node.getAttribute('id') == 0) > > > node.parentNode.removeChild(node); > > > }); > > > > > > You could even use a Set() for seenIds if you feel inclined. > > > > Moving to the JS wrapper simplifies the code so we don't need to update > > |toRemove| in |updateNode_|. It makes more sense to only check the list of > > pre-existing nodes. > > Alright. Well, I prefer the version which doesn't have this extra variable > hanging off the node, regardless of whether it's pure JS or a class. Is it > something to do with only checking the list of pre-existing nodes...? There's > going to be at most 1 more node than before, so it's hardly any different, and > less code the way I suggested. Eh. what if we just make a superset of all the IDs in the DOM and from the data and iterate them? if we find the ID in the DOM but not data, remove. if we find the ID in the data but not the DOM, create. else update?
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].classList.add('to-remove'); On 2015/02/12 01:08:32, Dan Beam wrote: > On 2015/02/12 01:04:50, kalman wrote: > > On 2015/02/11 23:36:47, Hector Carmona wrote: > > > On 2015/02/11 19:25:35, kalman wrote: > > > > It seems odd to be modifying a node's class to mark it for deletion; at > > least, > > > > how about setting it on the JS wrapper itself? I.e. nodes[i].toRemove = > > true. > > > > > > > > It looks simpler to me than that though. The only time 'to-remove' is > added > > is > > > > here, and the only time it's removed is in updateNode_. You could write it > > as: > > > > > > > > var seenIds = []; > > > > > > > > this.data_.extensions.forEach(function(extension) { > > > > var node = $(extension.id); > > > > seenIds.push(extension.id); > > > > ... > > > > }); > > > > > > > > var nodes = document.querySelectorAll('.extension-list-item-wrapper'); > > > > // Note: calling slice() to take a copy of |nodes|, since NodeLists > > > > // are live. > > > > [].slice.call(nodes).forEach(function(node) { > > > > if (seenIds.indexOf(node.getAttribute('id') == 0) > > > > node.parentNode.removeChild(node); > > > > }); > > > > > > > > You could even use a Set() for seenIds if you feel inclined. > > > > > > Moving to the JS wrapper simplifies the code so we don't need to update > > > |toRemove| in |updateNode_|. It makes more sense to only check the list of > > > pre-existing nodes. > > > > Alright. Well, I prefer the version which doesn't have this extra variable > > hanging off the node, regardless of whether it's pure JS or a class. Is it > > something to do with only checking the list of pre-existing nodes...? There's > > going to be at most 1 more node than before, so it's hardly any different, and > > less code the way I suggested. Eh. > > what if we just make a superset of all the IDs in the DOM and from the data and > iterate them? > > if we find the ID in the DOM but not data, remove. if we find the ID in the > data but not the DOM, create. else update? Since we're only ever adding or removing 1 extension at a time, then why don't we add a class to the extension we're removing and query for it here? This way we don't need to keep track of what's been visited because we know when the trash button was clicked and we know what extension. We just need to verify that the extension is no longer in the data set. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].toRemove = true; On 2015/02/11 23:51:03, Dan Beam wrote: > .extensionStillInstalled_ = false Done. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:134: node.toRemove = false; On 2015/02/11 23:51:03, Dan Beam wrote: > .extensionStillInstalled_ = true Done. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:139: // Remove any node that was not updated. On 2015/02/11 23:51:03, Dan Beam wrote: > // Remove extensions that are no longer installed. Done. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:142: if (node.toRemove) On 2015/02/11 23:51:03, Dan Beam wrote: > if (!node.extensionStillInstalled_) Done. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:452: controlNode.removeChild(indicator); On 2015/02/11 23:51:03, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:596: * @param {function({Event} e)} handler The function that should be called On 2015/02/11 23:51:03, Dan Beam wrote: > nit: is this valid syntax? i would think function(Event). maybe try compiling > locally Compiled now. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:621: * @param {function({Element} item)} shownCallback shownCallback if the On 2015/02/11 23:51:03, Dan Beam wrote: > too much search and replace (second showCallback -> Callback) It's back now. https://codereview.chromium.org/893453002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:641: panel.hidden = !errors; On 2015/02/11 23:51:03, Dan Beam wrote: > [] is truthy, this needs to be !errors || errors.length == 0 Done.
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].classList.add('to-remove'); > Since we're only ever adding or removing 1 extension at a time, then why don't > we add a class to the extension we're removing and query for it here? This way > we don't need to keep track of what's been visited because we know when the > trash button was clicked and we know what extension. We just need to verify that > the extension is no longer in the data set. What do you mean by a "class"? A DOM class (like you had originally) or something populated from the webui data?
On 2015/02/12 18:34:39, kalman wrote: > https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... > File chrome/browser/resources/extensions/extension_list.js (right): > > https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... > chrome/browser/resources/extensions/extension_list.js:125: > nodes[i].classList.add('to-remove'); > > > Since we're only ever adding or removing 1 extension at a time, then why don't > > we add a class to the extension we're removing and query for it here? This way > > we don't need to keep track of what's been visited because we know when the > > trash button was clicked and we know what extension. We just need to verify > that > > the extension is no longer in the data set. > > What do you mean by a "class"? A DOM class (like you had originally) or > something populated from the webui data? After thinking about it some more, I've realized my idea will only work when the delete button is clicked. So if the extension is removed when the dialog is open in 2 windows it will only be removed from the current one. But the idea was that we add a DOM class when the delete button is pushed. That would make it so we don't need to iterate over the nodes when updating because we could query the document and get exactly 1 item to remove. I'm starting to lean towards your idea of keeping track of the seen seenIds and iterating over the nodes afterwards to make sure the id was seen.
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:125: nodes[i].classList.add('to-remove'); On 2015/02/12 01:08:32, Dan Beam wrote: > On 2015/02/12 01:04:50, kalman wrote: > > On 2015/02/11 23:36:47, Hector Carmona wrote: > > > On 2015/02/11 19:25:35, kalman wrote: > > > > It seems odd to be modifying a node's class to mark it for deletion; at > > least, > > > > how about setting it on the JS wrapper itself? I.e. nodes[i].toRemove = > > true. > > > > > > > > It looks simpler to me than that though. The only time 'to-remove' is > added > > is > > > > here, and the only time it's removed is in updateNode_. You could write it > > as: > > > > > > > > var seenIds = []; > > > > > > > > this.data_.extensions.forEach(function(extension) { > > > > var node = $(extension.id); > > > > seenIds.push(extension.id); > > > > ... > > > > }); > > > > > > > > var nodes = document.querySelectorAll('.extension-list-item-wrapper'); > > > > // Note: calling slice() to take a copy of |nodes|, since NodeLists > > > > // are live. > > > > [].slice.call(nodes).forEach(function(node) { > > > > if (seenIds.indexOf(node.getAttribute('id') == 0) > > > > node.parentNode.removeChild(node); > > > > }); > > > > > > > > You could even use a Set() for seenIds if you feel inclined. > > > > > > Moving to the JS wrapper simplifies the code so we don't need to update > > > |toRemove| in |updateNode_|. It makes more sense to only check the list of > > > pre-existing nodes. > > > > Alright. Well, I prefer the version which doesn't have this extra variable > > hanging off the node, regardless of whether it's pure JS or a class. Is it > > something to do with only checking the list of pre-existing nodes...? There's > > going to be at most 1 more node than before, so it's hardly any different, and > > less code the way I suggested. Eh. > > what if we just make a superset of all the IDs in the DOM and from the data and > iterate them? > > if we find the ID in the DOM but not data, remove. if we find the ID in the > data but not the DOM, create. else update? ^ ping https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:564: extension.manifestErrors); indent off https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:623: * @param {function(Element)} [shownCallback] Callback if the element is {function(Element)=} opt_shownCallback https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:638: * @param {Array<RuntimeError>} [errors] The errors to be displayed. = + opt_
actually, whatever, lgtm -- i'll let you guys figure out how to deal with deleted extensions. i'm fine with the current solution as well
New patchsets have been uploaded after l-g-t-m from dbeam@chromium.org
https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:564: extension.manifestErrors); On 2015/02/12 20:46:19, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:623: * @param {function(Element)} [shownCallback] Callback if the element is On 2015/02/12 20:46:19, Dan Beam wrote: > {function(Element)=} opt_shownCallback Done. https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:638: * @param {Array<RuntimeError>} [errors] The errors to be displayed. On 2015/02/12 20:46:20, Dan Beam wrote: > = + opt_ errors should not be optional. Updated the annotation to reflect that it can be undefined, but isn't optional.
lgtm https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/240001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:638: * @param {Array<RuntimeError>} [errors] The errors to be displayed. On 2015/02/12 22:02:28, Hector Carmona wrote: > On 2015/02/12 20:46:20, Dan Beam wrote: > > = + opt_ > > errors should not be optional. Updated the annotation to reflect that it can be > undefined, but isn't optional. yeah, that's fine as well https://codereview.chromium.org/893453002/diff/280001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/280001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:637: * @param {Array.<RuntimeError>|undefined} errors The errors to be nit: leave as Array<, it's just shorter Array.<
lgtm https://codereview.chromium.org/893453002/diff/280001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/280001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:132: node.extensionStillInstalled_ = true; This line doesn't look necessary anymore.
New patchsets have been uploaded after l-g-t-m from dbeam@chromium.org,kalman@chromium.org
Applied last feedback, clicking commit https://codereview.chromium.org/893453002/diff/280001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/893453002/diff/280001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:132: node.extensionStillInstalled_ = true; On 2015/02/12 23:44:49, kalman wrote: > This line doesn't look necessary anymore. Done. https://codereview.chromium.org/893453002/diff/280001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:637: * @param {Array.<RuntimeError>|undefined} errors The errors to be On 2015/02/12 22:39:03, Dan Beam wrote: > nit: leave as Array<, it's just shorter Array.< Done.
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/893453002/300001
Message was sent while issue was closed.
Committed patchset #14 (id:300001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/514b36babdf0e97f32b93eebe3122b17d3cfc2c4 Cr-Commit-Position: refs/heads/master@{#316120} |
