Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
916243002:
Enable keyboard shortcuts for chrome://extensions (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, oshima+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. |
DescriptionEnable keyboard shortcuts for chrome://extensions
Make the list of extensions into a FocusGrid. This will make the
extensions page consistent with history and downloads.
BUG=427672
Committed: https://crrev.com/02e942ae87ad74bb0076da5cd035f75e06d23d67
Cr-Commit-Position: refs/heads/master@{#318090}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Feedback #Patch Set 3 : Merge with refactor #Patch Set 4 : Fix closure compile #
Total comments: 34
Patch Set 5 : Apply Dan's Feedback #
Total comments: 6
Patch Set 6 : Apply Feedback #
Total comments: 12
Patch Set 7 : Applied Feedback #
Total comments: 22
Patch Set 8 : Apply Feedback #Patch Set 9 : Attempt to fix trybots #
Total comments: 22
Patch Set 10 : Apply Feedback + Fix Trybots #
Total comments: 6
Patch Set 11 : Apply Feedback #Patch Set 12 : Fix license comment and Rebase #
Total comments: 2
Messages
Total messages: 45 (7 generated)
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org, kalman@chromium.org
kalman + dbeam, Since you're both so familiar with the extensions page, I'm sending another CL your way :-) Thanks!
This sounds really cool. I don't know anything about how focus grid works though, which most of the changes require. https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:132: this.addSelected_('.options-button', 'options'); I wonder if this big block can be incorporated more smoothly into the other refactor you've done? E.g. when you create an element you register it as focusable or something, then you iterate over those here. All the code you've added here seems boilerplate-y.
https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:124: * Will rebuild the list of focusable elements. nit: Rebuilds focusable element list. also, why update (method name) vs rebuild (doc comment)? https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:125: */ nit: /** ... */ https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:132: this.addSelected_('.options-button', 'options'); On 2015/02/11 22:42:49, kalman wrote: > I wonder if this big block can be incorporated more smoothly into the other > refactor you've done? E.g. when you create an element you register it as > focusable or something, then you iterate over those here. > > All the code you've added here seems boilerplate-y. how is this "boilerplate-y"? https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:145: if (!this.classList.contains('inactive-extension')) { i'm confused. how can |this| have a classList but also be a FocusRow? something's wrong here. oh crap, why are FocusRows divs? that makes very little sense to me. https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:167: getFirstFocusableByType: function(types) { make private https://codereview.chromium.org/916243002/diff/1/ui/webui/resources/js/cr/ui/... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/916243002/diff/1/ui/webui/resources/js/cr/ui/... ui/webui/resources/js/cr/ui/focus_row.js:32: * @extends {HTMLDivElement} whhhhhhhhhhhhhhhhhhhhhhhat? whhhhhhhhhhhhhhy? did i do that?
Made changes and responded to feedback. https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:124: * Will rebuild the list of focusable elements. On 2015/02/11 23:36:17, Dan Beam wrote: > nit: Rebuilds focusable element list. > > also, why update (method name) vs rebuild (doc comment)? Changed doc comment to "Updates" for consistency. https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:125: */ On 2015/02/11 23:36:17, Dan Beam wrote: > nit: /** ... */ Done. https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:132: this.addSelected_('.options-button', 'options'); On 2015/02/11 23:36:17, Dan Beam wrote: > On 2015/02/11 22:42:49, kalman wrote: > > I wonder if this big block can be incorporated more smoothly into the other > > refactor you've done? E.g. when you create an element you register it as > > focusable or something, then you iterate over those here. > > > > All the code you've added here seems boilerplate-y. > > how is this "boilerplate-y"? Yes, some of this will merge nicely with the other refactor. We would still need to add the focusable elements here in order to maintain the proper focus order. https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:145: if (!this.classList.contains('inactive-extension')) { On 2015/02/11 23:36:17, Dan Beam wrote: > i'm confused. how can |this| have a classList but also be a FocusRow? > something's wrong here. oh crap, why are FocusRows divs? that makes very > little sense to me. Pro: The FocusRow is the div that it represents in the UI Con: Closely couples the FocusRow logic to div Does it make more sense where FocusRow is handling the focus logic and FocusRow.rowElement is where the focusing takes place? Or does it make more sense to have a FocusRow be a UI element with its own focus logic? https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:167: getFirstFocusableByType: function(types) { On 2015/02/11 23:36:17, Dan Beam wrote: > make private Done. https://codereview.chromium.org/916243002/diff/1/ui/webui/resources/js/cr/ui/... File ui/webui/resources/js/cr/ui/focus_row.js (right): https://codereview.chromium.org/916243002/diff/1/ui/webui/resources/js/cr/ui/... ui/webui/resources/js/cr/ui/focus_row.js:32: * @extends {HTMLDivElement} On 2015/02/11 23:36:17, Dan Beam wrote: > whhhhhhhhhhhhhhhhhhhhhhhat? whhhhhhhhhhhhhhy? did i do that? That was me, when we did the big refactoring for downloads. This lets us decorate any div: https://codereview.chromium.org/807593005/diff2/120001:140001/ui/webui/resour...
https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:132: this.addSelected_('.options-button', 'options'); On 2015/02/12 00:24:08, Hector Carmona wrote: > On 2015/02/11 23:36:17, Dan Beam wrote: > > On 2015/02/11 22:42:49, kalman wrote: > > > I wonder if this big block can be incorporated more smoothly into the other > > > refactor you've done? E.g. when you create an element you register it as > > > focusable or something, then you iterate over those here. > > > > > > All the code you've added here seems boilerplate-y. > > > > how is this "boilerplate-y"? Two ways: (1) We already have 2 sections of code which go and add a bunch of controls, then update those controls. Now we also have a 3rd section which describes how focus works in this table. (2) Several of the helper methods below seem quite generic, seems like they should be part of the FocusGrid infrastructure? > > Yes, some of this will merge nicely with the other refactor. We would still need > to add the focusable elements here in order to maintain the proper focus order.
https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:132: this.addSelected_('.options-button', 'options'); On 2015/02/12 01:15:19, kalman wrote: > On 2015/02/12 00:24:08, Hector Carmona wrote: > > On 2015/02/11 23:36:17, Dan Beam wrote: > > > On 2015/02/11 22:42:49, kalman wrote: > > > > I wonder if this big block can be incorporated more smoothly into the > other > > > > refactor you've done? E.g. when you create an element you register it as > > > > focusable or something, then you iterate over those here. > > > > > > > > All the code you've added here seems boilerplate-y. > > > > > > how is this "boilerplate-y"? > > Two ways: > > (1) We already have 2 sections of code which go and add a bunch of controls, > then update those controls. Now we also have a 3rd section which describes how > focus works in this table. > > (2) Several of the helper methods below seem quite generic, seems like they > should be part of the FocusGrid infrastructure? > > > > > Yes, some of this will merge nicely with the other refactor. We would still > need > > to add the focusable elements here in order to maintain the proper focus > order. the reason why the column names exist is so that when moving from one extension to the other the *type* of control focused can stay the same, or if that control doesn't exist in the extension (where it gets interesting), we can make an informed choice in getEquivalentType(). for example: moving between an extension w/ a background page link focused to an extension without a background page at all -- where do you expect the focus to go? without annotating the control types in some [admittedly manual] way, there's really no great way to handle all cases and spatial representations/UIs.
Merged the changes from the refactoring into this and fixed the js compile issues I ran into.
Dan are you going to have a look at this?
On 2015/02/19 00:41:53, kalman wrote: > Dan are you going to have a look at this? (I'd rather you approve first, since it's a framework I'm not at all familiar with)
https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:58: assertInstanceof(this, cr.ui.FocusRow), boundary); why are you using assertInstanceOf() here? have we been bitten by this or is the compiler complaining? https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:127: this.contents_.addEventListener('focus', this.focusContents_.bind(this)); onFocus_ or onFocusContents_ https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:129: this.focusinContents_.bind(this)); onFocusin_ or onFocusinContents_ https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:149: '.extension-error-list-show-more [is="action-link"]'); return this.querySelector( '.extension-error-list-show-more [is="action-link"]:not([hidden])'); https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:156: * @return {Element} The boundary for the FocusGrid. nit: 1-line -> /** ... */ https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:158: getListElement: function() { getFocusGridBoundary? https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:165: * grid in reverse. rewrite doc comment without using "this" as much (no idea what you're talking about without reading whole file) https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:182: if (activeRow) activeRow = activeRow || this.contents_.querySelector('.extension-error-metadata'); activeRow.getEquivalentElement(null).focus(); https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:184: else { conditionals can either have 0 curlies or ALL curlies. add more here https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:185: activeRow = this.contents_.querySelector('.extension-error-metadata'); why doesn't focusgrid have a way of accessing rows? https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:203: querySelector('.extension-error-metadata'); wrong indent https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:103: equivalent = this.getFirstFocusableByType_(actionLinks); why are we potentially overwriting |equivalent| 4 times after it's set? should these all be `else if` more or less? https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:179: return type.indexOf('dev-') == 0; this can fit in one line as: return element.getAttribute('column-type').indexOf('dev-') == 0; OR return /^dev-/.test(element.getAttribute('column-type')); https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:214: /** @private {cr.ui.FocusGrid} */ nit: !cr.ui.FocusGrid https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:285: var nodes = document.querySelectorAll('.extension-list-item-wrapper[id]'); nit: nodes -> rows https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:750: node.updateFocusableElements(); every time I see node#focusRowFunction() my eyes are sad. if this is a focusrow, it should be row, not node (which sounds like Node, which does not have updateFocusableElements). https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:767: element.setAttribute('column-type', columnType); this is an unexpected side effect in "addListener_" IMO. split this into 2 methods or rename
https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:58: assertInstanceof(this, cr.ui.FocusRow), boundary); On 2015/02/19 19:26:27, Dan Beam wrote: > why are you using assertInstanceOf() here? have we been bitten by this or is > the compiler complaining? This was left over from before I updated the @extends and I forgot to remove it. It's not necessary. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:127: this.contents_.addEventListener('focus', this.focusContents_.bind(this)); On 2015/02/19 19:26:27, Dan Beam wrote: > onFocus_ or onFocusContents_ Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:129: this.focusinContents_.bind(this)); On 2015/02/19 19:26:27, Dan Beam wrote: > onFocusin_ or onFocusinContents_ Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:149: '.extension-error-list-show-more [is="action-link"]'); On 2015/02/19 19:26:28, Dan Beam wrote: > return this.querySelector( > '.extension-error-list-show-more [is="action-link"]:not([hidden])'); Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:156: * @return {Element} The boundary for the FocusGrid. On 2015/02/19 19:26:28, Dan Beam wrote: > nit: 1-line -> /** ... */ Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:158: getListElement: function() { On 2015/02/19 19:26:28, Dan Beam wrote: > getFocusGridBoundary? Updated doc. Only this class needs to know it's the boundary of the grid. Classes outside only need to know it's the element with a list of errors. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:165: * grid in reverse. On 2015/02/19 19:26:28, Dan Beam wrote: > rewrite doc comment without using "this" as much (no idea what you're talking > about without reading whole file) Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:182: if (activeRow) On 2015/02/19 19:26:27, Dan Beam wrote: > activeRow = activeRow || > this.contents_.querySelector('.extension-error-metadata'); > activeRow.getEquivalentElement(null).focus(); Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:184: else { On 2015/02/19 19:26:28, Dan Beam wrote: > conditionals can either have 0 curlies or ALL curlies. add more here Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:185: activeRow = this.contents_.querySelector('.extension-error-metadata'); On 2015/02/19 19:26:28, Dan Beam wrote: > why doesn't focusgrid have a way of accessing rows? It does! :D And using that simplifies this code significantly. Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:203: querySelector('.extension-error-metadata'); On 2015/02/19 19:26:28, Dan Beam wrote: > wrong indent Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:103: equivalent = this.getFirstFocusableByType_(actionLinks); On 2015/02/19 19:26:28, Dan Beam wrote: > why are we potentially overwriting |equivalent| 4 times after it's set? should > these all be `else if` more or less? Only one should match. Changed to else if. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:179: return type.indexOf('dev-') == 0; On 2015/02/19 19:26:28, Dan Beam wrote: > this can fit in one line as: > > return element.getAttribute('column-type').indexOf('dev-') == 0; > > OR > > return /^dev-/.test(element.getAttribute('column-type')); Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:214: /** @private {cr.ui.FocusGrid} */ On 2015/02/19 19:26:28, Dan Beam wrote: > nit: !cr.ui.FocusGrid Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:285: var nodes = document.querySelectorAll('.extension-list-item-wrapper[id]'); On 2015/02/19 19:26:28, Dan Beam wrote: > nit: nodes -> rows Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:750: node.updateFocusableElements(); On 2015/02/19 19:26:28, Dan Beam wrote: > every time I see node#focusRowFunction() my eyes are sad. if this is a > focusrow, it should be row, not node (which sounds like Node, which does not > have updateFocusableElements). Done. https://codereview.chromium.org/916243002/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:767: element.setAttribute('column-type', columnType); On 2015/02/19 19:26:28, Dan Beam wrote: > this is an unexpected side effect in "addListener_" IMO. split this into 2 > methods or rename Done.
https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:147: '.extension-error-list-show-more [is="action-link"]:not(hidden)'); :not([hidden]), did you try this code? do we need a test? https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:154: * @param {string} eventType The type of event to listen to. why are we listening to |eventType|? https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:156: * by the event. why is this handler needed?
https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:147: '.extension-error-list-show-more [is="action-link"]:not(hidden)'); On 2015/02/20 00:22:15, Dan Beam wrote: > :not([hidden]), did you try this code? do we need a test? Added a test to verify that this button is part of the focus grid when there are enough errors and not added when there are not enough errors. The test also caught an a11y issue here. The icons for the errors did not have alt text. https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:154: * @param {string} eventType The type of event to listen to. On 2015/02/20 00:22:15, Dan Beam wrote: > why are we listening to |eventType|? Updated comment to be more descriptive. https://codereview.chromium.org/916243002/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:156: * by the event. On 2015/02/20 00:22:15, Dan Beam wrote: > why is this handler needed? Updated comment to be more descriptive.
looking pretty good, like the test https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:98: Profile* profile = this->GetProfile(); remove this-> and inline GetProfile() if it's prettier https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:105: return NULL; where do we care about the return value? wouldn't this just fail silently currently? https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.h (right): https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.h:41: const extensions::Extension* InstallUnpackedExtension( \s\s -> \s https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:70: GEN(' InstallErrorsExtension();'); doesn't care about the return value https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:106: nit: remove \n
https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:98: Profile* profile = this->GetProfile(); On 2015/02/20 23:36:43, Dan Beam wrote: > remove this-> and inline GetProfile() if it's prettier Done. https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:105: return NULL; On 2015/02/20 23:36:43, Dan Beam wrote: > where do we care about the return value? wouldn't this just fail silently > currently? Changed to void. Most tests will still pass if this fails to load, so it's not fatal. https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.h (right): https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.h:41: const extensions::Extension* InstallUnpackedExtension( On 2015/02/20 23:36:43, Dan Beam wrote: > \s\s -> \s Done. Just noticed the function immediately above this one, but that doesn't seem to be defined anywhere. Keeping "InstallUnpackedExtension" since its name more closely matches the existing "InstallExtension". https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js (right): https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:70: GEN(' InstallErrorsExtension();'); On 2015/02/20 23:36:43, Dan Beam wrote: > doesn't care about the return value Acknowledged. https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:106: On 2015/02/20 23:36:43, Dan Beam wrote: > nit: remove \n Done.
https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:105: return NULL; On 2015/02/21 00:16:01, Hector Carmona wrote: > On 2015/02/20 23:36:43, Dan Beam wrote: > > where do we care about the return value? wouldn't this just fail silently > > currently? > > Changed to void. Most tests will still pass if this fails to load, so it's not > fatal. wait, what? why would we ever want to say InstallUnpackedExtension() if it's not necessary that it succeeds...? https://codereview.chromium.org/916243002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:99: extensions::ExtensionSystem::Get(this->GetProfile())->extension_service(); this->GetProfile() should be just GetProfile()
kalman@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Honestly I've been staring at this for a while, and can't get very far - I'm sick this week and don't have access to a build besides, so can't try out your changes. However, if Dan is happy, I am happy, and delegate the lgtm stamp to him. https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; +rdevlin.cronin It looks like this alt should describe the severity level, unless there's some other way the severity level is described to screen readers. You can leave it as a todo for now though, pending what Devlin wants to do about it. https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:132: if (idIsValid(error.extensionId)) { +rdevlin.cronin How can this idIsValid check fail, these errors should be populated from our trusted C++ code? https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:146: getToggleElement: function() { The comment is more useful than the name of the function; should it be getShowMoreElement? Is it a toggle because it says "Show less" sometimes, in which case the comment needs to express that as well? https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:152: getListElement: function() { ListElement makes me think like HTMLUListElement or whatever. Maybe getErrorListElement? https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:157: * The gird should not be focusable once it or an element inside it is grid https://codereview.chromium.org/916243002/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/error_console/runtime_and_manifest_errors/manifest.json (right): https://codereview.chromium.org/916243002/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/error_console/runtime_and_manifest_errors/manifest.json:17: "gnarly", TODO(kalman): Implement the chrome.gnarly API. https://codereview.chromium.org/916243002/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/error_console/runtime_and_manifest_errors/manifest.json:19: "tubular", chrome.tubular sounds like fun as well.
https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; On 2015/02/21 00:47:01, kalman wrote: > +rdevlin.cronin > > It looks like this alt should describe the severity level, unless there's some > other way the severity level is described to screen readers. You can leave it as > a todo for now though, pending what Devlin wants to do about it. doesn't the message indicate severity in some way or is this solely presentational at this point? https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:157: * The gird should not be focusable once it or an element inside it is On 2015/02/21 00:47:01, kalman wrote: > grid #noregerts
https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; On 2015/02/21 00:49:58, Dan Beam wrote: > On 2015/02/21 00:47:01, kalman wrote: > > +rdevlin.cronin > > > > It looks like this alt should describe the severity level, unless there's some > > other way the severity level is described to screen readers. You can leave it > as > > a todo for now though, pending what Devlin wants to do about it. > > doesn't the message indicate severity in some way or is this solely > presentational at this point? It's meant to emulate devtools. If I were to do: console.error('forgot the milk') [x] forgot the milk console.log('forgot the milk') forgot the milk those seem like different things that are worth communicating.
https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; On 2015/02/21 00:54:41, kalman wrote: > On 2015/02/21 00:49:58, Dan Beam wrote: > > On 2015/02/21 00:47:01, kalman wrote: > > > +rdevlin.cronin > > > > > > It looks like this alt should describe the severity level, unless there's > some > > > other way the severity level is described to screen readers. You can leave > it > > as > > > a todo for now though, pending what Devlin wants to do about it. > > > > doesn't the message indicate severity in some way or is this solely > > presentational at this point? > > It's meant to emulate devtools. If I were to do: > > console.error('forgot the milk') > [x] forgot the milk > console.log('forgot the milk') > forgot the milk > > those seem like different things that are worth communicating. so that's a "yes" :)
https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; On 2015/02/21 00:55:53, Dan Beam wrote: > On 2015/02/21 00:54:41, kalman wrote: > > On 2015/02/21 00:49:58, Dan Beam wrote: > > > On 2015/02/21 00:47:01, kalman wrote: > > > > +rdevlin.cronin > > > > > > > > It looks like this alt should describe the severity level, unless there's > > some > > > > other way the severity level is described to screen readers. You can leave > > it > > > as > > > > a todo for now though, pending what Devlin wants to do about it. > > > > > > doesn't the message indicate severity in some way or is this solely > > > presentational at this point? > > > > It's meant to emulate devtools. If I were to do: > > > > console.error('forgot the milk') > > [x] forgot the milk > > console.log('forgot the milk') > > forgot the milk > > > > those seem like different things that are worth communicating. > > so that's a "yes" :) Confused. But, I think my comment has merit.
https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; On 2015/02/21 00:47:01, kalman wrote: > +rdevlin.cronin > > It looks like this alt should describe the severity level, unless there's some > other way the severity level is described to screen readers. You can leave it as > a todo for now though, pending what Devlin wants to do about it. Yeah, for perfect a11y this should describe the severity level ('log', 'warn[ing]', 'error'). The source of the image is set in css based on the class (see lines 61-66), but I don't _think_ there's a way to do that for alt-text, so it might be good to set here (I'm fine with it as a TODO, since it isn't a regression to have it absent). https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:132: if (idIsValid(error.extensionId)) { On 2015/02/21 00:47:01, kalman wrote: > +rdevlin.cronin > > How can this idIsValid check fail, these errors should be populated from our > trusted C++ code? IIRC, this is just us being paranoid. Yes, these should be populated via trusted code, but there's a chance the renderer is compromised, and these errors can (indirectly) cause access to the filesystem. That said, I'm pretty sure we have checks on the C++ side, too. Up to kalman/dbeam if they want to keep the extra check here.
https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:132: if (idIsValid(error.extensionId)) { On 2015/02/23 16:36:28, Devlin wrote: > On 2015/02/21 00:47:01, kalman wrote: > > +rdevlin.cronin > > > > How can this idIsValid check fail, these errors should be populated from our > > trusted C++ code? > > IIRC, this is just us being paranoid. Yes, these should be populated via > trusted code, but there's a chance the renderer is compromised, and these errors > can (indirectly) cause access to the filesystem. That said, I'm pretty sure we > have checks on the C++ side, too. Up to kalman/dbeam if they want to keep the > extra check here. That's true. In this case I'd argue that it'd just hide a bug on the browser side if anything; we should be validating things like extension IDs. Hopefully that isn't a source of a WebUI vulnerability (e.g. triggering a bug, versus "just" triggering a privilege escalation). In any case totally orthogonal, sorry bout that :-)
https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:105: return NULL; On 2015/02/21 00:26:10, Dan Beam wrote: > On 2015/02/21 00:16:01, Hector Carmona wrote: > > On 2015/02/20 23:36:43, Dan Beam wrote: > > > where do we care about the return value? wouldn't this just fail silently > > > currently? > > > > Changed to void. Most tests will still pass if this fails to load, so it's not > > fatal. > > wait, what? why would we ever want to say InstallUnpackedExtension() if it's > not necessary that it succeeds...? Updated so that it will fail the test if we cannot load the extension. https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; On 2015/02/23 16:36:28, Devlin wrote: > On 2015/02/21 00:47:01, kalman wrote: > > +rdevlin.cronin > > > > It looks like this alt should describe the severity level, unless there's some > > other way the severity level is described to screen readers. You can leave it > as > > a todo for now though, pending what Devlin wants to do about it. > > Yeah, for perfect a11y this should describe the severity level ('log', > 'warn[ing]', 'error'). The source of the image is set in css based on the class > (see lines 61-66), but I don't _think_ there's a way to do that for alt-text, so > it might be good to set here (I'm fine with it as a TODO, since it isn't a > regression to have it absent). Do we have 18n strings for these warnings? If so, I can add them. If not, I'll create an issue to add them. https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:132: if (idIsValid(error.extensionId)) { On 2015/02/23 16:53:26, kalman wrote: > On 2015/02/23 16:36:28, Devlin wrote: > > On 2015/02/21 00:47:01, kalman wrote: > > > +rdevlin.cronin > > > > > > How can this idIsValid check fail, these errors should be populated from our > > > trusted C++ code? > > > > IIRC, this is just us being paranoid. Yes, these should be populated via > > trusted code, but there's a chance the renderer is compromised, and these > errors > > can (indirectly) cause access to the filesystem. That said, I'm pretty sure > we > > have checks on the C++ side, too. Up to kalman/dbeam if they want to keep the > > extra check here. > > That's true. In this case I'd argue that it'd just hide a bug on the browser > side if anything; we should be validating things like extension IDs. Hopefully > that isn't a source of a WebUI vulnerability (e.g. triggering a bug, versus > "just" triggering a privilege escalation). In any case totally orthogonal, sorry > bout that :-) What should we do about the check? Keep it or get rid of it? https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:146: getToggleElement: function() { On 2015/02/21 00:47:01, kalman wrote: > The comment is more useful than the name of the function; should it be > getShowMoreElement? Is it a toggle because it says "Show less" sometimes, in > which case the comment needs to express that as well? Updated to comment to reflect that the "Show more" and "Show less" button are the same element. https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:152: getListElement: function() { On 2015/02/21 00:47:01, kalman wrote: > ListElement makes me think like HTMLUListElement or whatever. Maybe > getErrorListElement? Done. https://codereview.chromium.org/916243002/diff/120001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:157: * The gird should not be focusable once it or an element inside it is On 2015/02/21 00:47:01, kalman wrote: > grid Done. https://codereview.chromium.org/916243002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:99: extensions::ExtensionSystem::Get(this->GetProfile())->extension_service(); On 2015/02/21 00:26:10, Dan Beam wrote: > this->GetProfile() should be just GetProfile() Done. Too used to JS
very close to lg https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:49: this.querySelector('.extension-error-view-details'), Element); nit: querySelector only returns Element|null, so just assert() https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; nit: add TODO to fix https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:178: if (!toggleButton || toggleButton.isShowingAll) { i think this if is easier to read reversed as if (toggleButton && !toggleButton.isShowingAll) { is just easier for me to grok (do you agree?) https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:184: var firstVisible = rows.length - ExtensionErrorList.MAX_ERRORS_TO_SHOW_; what if rows.length == 0 or less than MAX_ERRORS_TO_SHOW_? https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:191: } activeRow.getEquivalentElement(null).focus(); is called in each path, so move to here https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:281: node.destroy(); assertInstanceof(node, ExtensionFocusRow).destroy() OR /** @type ExtensionFocusRow */(node).destroy(); or whatever to ensure the compiler knows .destroy is on cr.ui.FocusRow (I'm pretty sure this currently wont compile) https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:301: rows[i].updateFocusableElements(); possibly same casting magic https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:814: if (!panel.hidden) { nit: if (panel.hidden) return; ... rest of code ... https://codereview.chromium.org/916243002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:108: service->set_show_extensions_prompts(false); nit: reverse these ^^ two lines, e.g., ExtensionService* service = extensions::ExtensionSystem::Get(profile)->extension_service(); service->set_show_extensions_prompts(false); extensions::ExtensionRegistry* registry = extensions::ExtensionRegistry::Get(profile); extensions::TestExtensionRegistryObserver observer(registry, id); this is because we want to declare locals (e.g. |registry| or |service|) as closely as possible to where they're used
https://codereview.chromium.org/916243002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:110: extensions::TestExtensionRegistryObserver observer(registry, id); i think this class auto registers itself as an observer: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:49: this.querySelector('.extension-error-view-details'), Element); On 2015/02/24 00:55:37, Dan Beam wrote: > nit: querySelector only returns Element|null, so just assert() Done. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:70: iconNode.alt = ''; On 2015/02/24 00:55:37, Dan Beam wrote: > nit: add TODO to fix Done. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:178: if (!toggleButton || toggleButton.isShowingAll) { On 2015/02/24 00:55:37, Dan Beam wrote: > i think this if is easier to read reversed as > > if (toggleButton && !toggleButton.isShowingAll) { > > is just easier for me to grok (do you agree?) I agree with this change. Done. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:184: var firstVisible = rows.length - ExtensionErrorList.MAX_ERRORS_TO_SHOW_; On 2015/02/24 00:55:37, Dan Beam wrote: > what if rows.length == 0 or less than MAX_ERRORS_TO_SHOW_? toggleButton == null if rows.length < MAX_ERRORS_TO_SHOW_, so this code won't execute. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:191: } On 2015/02/24 00:55:37, Dan Beam wrote: > activeRow.getEquivalentElement(null).focus(); > > is called in each path, so move to here Done. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:281: node.destroy(); On 2015/02/24 00:55:37, Dan Beam wrote: > assertInstanceof(node, ExtensionFocusRow).destroy() > > OR > > /** @type ExtensionFocusRow */(node).destroy(); > > or whatever to ensure the compiler knows .destroy is on cr.ui.FocusRow (I'm > pretty sure this currently wont compile) Compiler didn't complain, but changed to make it more explicit. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:301: rows[i].updateFocusableElements(); On 2015/02/24 00:55:37, Dan Beam wrote: > possibly same casting magic Done. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:814: if (!panel.hidden) { On 2015/02/24 00:55:37, Dan Beam wrote: > nit: if (panel.hidden) return; ... rest of code ... Done. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:108: service->set_show_extensions_prompts(false); On 2015/02/24 00:55:38, Dan Beam wrote: > nit: reverse these ^^ two lines, e.g., > > ExtensionService* service = > extensions::ExtensionSystem::Get(profile)->extension_service(); > service->set_show_extensions_prompts(false); > > extensions::ExtensionRegistry* registry = > extensions::ExtensionRegistry::Get(profile); > extensions::TestExtensionRegistryObserver observer(registry, id); > > this is because we want to declare locals (e.g. |registry| or |service|) as > closely as possible to where they're used Done. https://codereview.chromium.org/916243002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc:110: extensions::TestExtensionRegistryObserver observer(registry, id); On 2015/02/24 02:40:49, Dan Beam wrote: > i think this class auto registers itself as an observer: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... Awesome, thanks! Done.
lgtm https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:184: var firstVisible = rows.length - ExtensionErrorList.MAX_ERRORS_TO_SHOW_; On 2015/02/24 02:49:43, Hector Carmona wrote: > On 2015/02/24 00:55:37, Dan Beam wrote: > > what if rows.length == 0 or less than MAX_ERRORS_TO_SHOW_? > > toggleButton == null if rows.length < MAX_ERRORS_TO_SHOW_, so this code won't > execute. i assume you mean <= https://codereview.chromium.org/916243002/diff/180001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:180: var rows = this.focusGrid_.rows; assert(rows.length > ExtensionErrorList.MAX_ERRORS_TO_SHOW_); https://codereview.chromium.org/916243002/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:185: if (focusedIndex < firstVisible) nit: if (rows.indexOf(activeRow) < firstVisible) https://codereview.chromium.org/916243002/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:188: } else { nit: } else if (!activeRow) { activeRow = this.focusGrid_.rows[0]; }
https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/160001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:184: var firstVisible = rows.length - ExtensionErrorList.MAX_ERRORS_TO_SHOW_; On 2015/02/24 18:16:46, Dan Beam wrote: > On 2015/02/24 02:49:43, Hector Carmona wrote: > > On 2015/02/24 00:55:37, Dan Beam wrote: > > > what if rows.length == 0 or less than MAX_ERRORS_TO_SHOW_? > > > > toggleButton == null if rows.length < MAX_ERRORS_TO_SHOW_, so this code won't > > execute. > > i assume you mean <= Yes https://codereview.chromium.org/916243002/diff/180001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/916243002/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:180: var rows = this.focusGrid_.rows; On 2015/02/24 18:16:47, Dan Beam wrote: > assert(rows.length > ExtensionErrorList.MAX_ERRORS_TO_SHOW_); Done. https://codereview.chromium.org/916243002/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:185: if (focusedIndex < firstVisible) On 2015/02/24 18:16:47, Dan Beam wrote: > nit: if (rows.indexOf(activeRow) < firstVisible) Done. https://codereview.chromium.org/916243002/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_error.js:188: } else { On 2015/02/24 18:16:47, Dan Beam wrote: > nit: > > } else if (!activeRow) { > activeRow = this.focusGrid_.rows[0]; > } Done.
@Devlin, any objections to committing this CL?
On 2015/02/25 17:47:31, Hector Carmona wrote: > @Devlin, any objections to committing this CL? Nope, I trust Ben and Dan.
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/916243002/#ps200001 (title: "Apply Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/916243002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/916243002/#ps220001 (title: "Fix license comment and Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/916243002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/02e942ae87ad74bb0076da5cd035f75e06d23d67 Cr-Commit-Position: refs/heads/master@{#318090}
Message was sent while issue was closed.
https://codereview.chromium.org/916243002/diff/220001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/916243002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:77: function ExtensionFocusRow() {} next time put this inside the cr.define()
Message was sent while issue was closed.
https://codereview.chromium.org/916243002/diff/220001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.css (right): https://codereview.chromium.org/916243002/diff/220001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.css:422: background-color: rgba(0, 0, 0, .05); also, stop making design changes unless: a) it actually looks good b) you ask a designer's opinion this looks bad; removing. |
