Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2200893003:
DevTools: Add autocomplete for ARIA (Closed)
|
Created:
4 years, 4 months ago by aboxhall Modified:
4 years, 3 months ago CC:
chromium-reviews, caseq+blink_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, lushnikov+blink_chromium.org, yuzo+watch_chromium.org, pfeldman+blink_chromium.org, nektarios, dmazzoni, apavlov+blink_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, devtools-reviews_chromium.org, blink-reviews, je_julie, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add autocomplete for ARIA.
BUG=616950
Committed: https://crrev.com/515495b524fa67e475f0068809f42f814370866d
Cr-Commit-Position: refs/heads/master@{#415094}
Patch Set 1 : now with all files #Patch Set 2 : Test working #Patch Set 3 : Remove done TODO #
Total comments: 21
Patch Set 4 : lushinkov comments #Patch Set 5 : Delete ARIA.js, add comment to aria_attributes.py #
Total comments: 28
Patch Set 6 : lushinkov review comments #
Total comments: 10
Patch Set 7 : lushnikov review comments #Patch Set 8 : remove test #
Total comments: 5
Patch Set 9 : reinstate test #Patch Set 10 : Inline roles #Patch Set 11 : rebase #
Total comments: 2
Patch Set 12 : skip_compilation #Messages
Total messages: 48 (26 generated)
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
aboxhall@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/aria_attributes.py (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/aria_attributes.py:78: def main(_): Not sure whether to commit this file or not.
dgozman@chromium.org changed reviewers: + lushnikov@chromium.org
Andrey, could you please take a look? This looks pretty similar to CSS autocomplete.
Description was changed from ========== WIP ARIA autocomplete BUG=616950 ========== to ========== Add autocomplete for ARIA. BUG=616950 ==========
@lushnikov Friendly ping?
https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIA.js (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIA.js:1: WebInspector.ARIAMetadata.initializeWithConfig({ why do you need this file? https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:11: this._attributes = {}; can we use sets/maps here? also, let's jsdoc https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:12: this._roles = {}; ditto https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:15: return; let's not do fast-returns in constructors. Instead, let's introduce an _initialize method and call it in case of existing config https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:35: // Do one pass to flip the inheritance hierarchy and turn arrays into sets nit: comments in blink should end with period. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:58: function _inheritAttributes(parentRole, childRole) nit: we only start private class methods and members start with "_". Let's call this "inheritAttributes" https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:80: WebInspector.ARIAMetadata.initializeWithConfig = function(config) please add jsdoc https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:49: this._ariaSubPane.setNode(node); why this change? https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/aria_attributes.py (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/aria_attributes.py:30: response = urllib2.urlopen("http://www.w3.org/WAI/ARIA/schemata/aria-1.rdf", timeout=5) do you intend to run this script as a part of buildsystem? This timeout wouldn't allow you to.
https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIA.js (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIA.js:1: WebInspector.ARIAMetadata.initializeWithConfig({ On 2016/08/23 17:23:51, lushnikov wrote: > why do you need this file? I don't quite follow the question. This provides the allowed values for each attribute, and for role. It does include more information than that, which isn't strictly necessary for this change, but will be useful in future changes. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:11: this._attributes = {}; On 2016/08/23 17:23:51, lushnikov wrote: > can we use sets/maps here? also, let's jsdoc Done. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:12: this._roles = {}; On 2016/08/23 17:23:51, lushnikov wrote: > ditto Actually this is unnecessary for this change (as is much of the logic below) so I removed it. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:15: return; On 2016/08/23 17:23:51, lushnikov wrote: > let's not do fast-returns in constructors. > Instead, let's introduce an _initialize method and call it in case of existing > config Done. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:35: // Do one pass to flip the inheritance hierarchy and turn arrays into sets On 2016/08/23 17:23:51, lushnikov wrote: > nit: comments in blink should end with period. Done. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:58: function _inheritAttributes(parentRole, childRole) On 2016/08/23 17:23:51, lushnikov wrote: > nit: we only start private class methods and members start with "_". Let's call > this "inheritAttributes" Done. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:80: WebInspector.ARIAMetadata.initializeWithConfig = function(config) On 2016/08/23 17:23:51, lushnikov wrote: > please add jsdoc Done. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:49: this._ariaSubPane.setNode(node); On 2016/08/23 17:23:51, lushnikov wrote: > why this change? Fixes a bug where the ARIA sub-pane would get out of sync when changing the inspected node. I can submit a separate CL for it if you like. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/aria_attributes.py (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/aria_attributes.py:30: response = urllib2.urlopen("http://www.w3.org/WAI/ARIA/schemata/aria-1.rdf", timeout=5) On 2016/08/23 17:23:51, lushnikov wrote: > do you intend to run this script as a part of buildsystem? This timeout wouldn't > allow you to. No, if anything it'd be manually run occasionally by a developer.
Any more feedback here?
The accessibility module is getting big and solid; let's make this a remote module so that we don't drastically increase devtools.grd payload. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIA.js (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIA.js:1: WebInspector.ARIAMetadata.initializeWithConfig({ On 2016/08/23 21:06:16, aboxhall wrote: > On 2016/08/23 17:23:51, lushnikov wrote: > > why do you need this file? > > I don't quite follow the question. This provides the allowed values for each > attribute, and for role. It does include more information than that, which isn't > strictly necessary for this change, but will be useful in future changes. This file seems to be identical to ARIAConfig - looks like this is unintended. https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/aria_attributes.py (right): https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/aria_attributes.py:30: response = urllib2.urlopen("http://www.w3.org/WAI/ARIA/schemata/aria-1.rdf", timeout=5) On 2016/08/23 21:06:17, aboxhall wrote: > On 2016/08/23 17:23:51, lushnikov wrote: > > do you intend to run this script as a part of buildsystem? This timeout > wouldn't > > allow you to. > > No, if anything it'd be manually run occasionally by a developer. I see; can we put a clarifying comment in the beginning of generated file?
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
On 2016/08/24 23:05:10, lushnikov wrote: > The accessibility module is getting big and solid; let's make this a remote > module so that we don't drastically increase devtools.grd payload. Sure, but will we need to reverse that once this comes out of experimental? > https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/accessibility/ARIA.js (right): > > https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIA.js:1: > WebInspector.ARIAMetadata.initializeWithConfig({ > On 2016/08/23 21:06:16, aboxhall wrote: > > On 2016/08/23 17:23:51, lushnikov wrote: > > > why do you need this file? > > > > I don't quite follow the question. This provides the allowed values for each > > attribute, and for role. It does include more information than that, which > isn't > > strictly necessary for this change, but will be useful in future changes. > > This file seems to be identical to ARIAConfig - looks like this is unintended. Oh right, I guess I changed the name and lost track of it. ARIAConfig seems like a better name, I'll remove this file. > https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/scripts/aria_attributes.py (right): > > https://codereview.chromium.org/2200893003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/scripts/aria_attributes.py:30: response = > urllib2.urlopen("http://www.w3.org/WAI/ARIA/schemata/aria-1.rdf", timeout=5) > On 2016/08/23 21:06:17, aboxhall wrote: > > On 2016/08/23 17:23:51, lushnikov wrote: > > > do you intend to run this script as a part of buildsystem? This timeout > > wouldn't > > > allow you to. > > > > No, if anything it'd be manually run occasionally by a developer. > > I see; can we put a clarifying comment in the beginning of generated file? Absolutely.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
>> The accessibility module is getting big and solid; let's make this a remote >> module so that we don't drastically increase devtools.grd payload. > > Sure, but will we need to reverse that once this comes out of experimental? Not really, we currently have a few models which are remote and they behave just good; we even consider moving more functionality into a remote modules! Also, nit: please, prefix the patch title with "DevTools:" label. It helps us greatly to navigate commit log. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: WebInspector.ARIAMetadata.initializeWithConfig({ Can we put a clarifying comment in the beginning here, which says something like "this file is generated with scripts/aria_attributes.py"? This way, people hopefully will not try to edit this file. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:89: "alert": { "nameFrom": ["author"], "superclasses": ["region"] }, it appears to me that role values are never used; can we avoid generating them in this case? https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js (right): https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:12: * @type {!Map<string, !WebInspector.ARIAMetadata.Attribute>} nit: we usually do one-line typedefs /** @type {!Map<string, !WebInspector.ARIAMetadata.Attribute>} */ https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:25: WebInspector.ARIAMetadata.instance = new WebInspector.ARIAMetadata(config); let's design it like WI.CSSMetadata object? This will make our code consistent WebInspector.ariaMetadata = function() { if (!WebInspector.ARIAMetadata._instance) WebInspector.ARIAMetadata._instance = new WebInspector.ARIAMetadata(WebInspector.ARIAMetadata._generatedConfig || null); return WebInspector.ARIAMetadata._instance; } https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:49: WebInspector.ARIAMetadata.Roles = new WebInspector.ARIAMetadata.ValueSet(Object.keys(roles)); why store this as a global object? Can we just make this._roles = Object.keys(roles); https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:70: * @param {?Array<string>} values do we ever create it with a null? https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:72: WebInspector.ARIAMetadata.ValueSet = function(values) Let's get rid of this class altogether - it doesn't do much. We would be able to filter things ad-hoc in ARIAAttributesView.js:240 https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:96: this._name = name; is this ever used? https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:105: this._enum = null; it looks like you never actually expect these to be null - can we enforce this through constructor non-nullable types? https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:118: type: function() this seems to be unused as well https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:126: defaultValue: function() where do you use this? https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:134: enum: function() are these values? we have a defaultValue, so this would be natural to call values: as well https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:136: return this._enum nit: missing ; https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:141: WebInspector.ARIAMetadata.AttributeTypes = { are these ever used? I didn't find any occurrence.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: WebInspector.ARIAMetadata.initializeWithConfig({ On 2016/08/25 18:46:45, lushnikov wrote: > Can we put a clarifying comment in the beginning here, which says something like > "this file is generated with scripts/aria_attributes.py"? > > This way, people hopefully will not try to edit this file. That's actually not strictly true, and in any case we're probably going to end up needing to edit this file. It might be simpler just to not commit the python script at all for now. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:89: "alert": { "nameFrom": ["author"], "superclasses": ["region"] }, On 2016/08/25 18:46:45, lushnikov wrote: > it appears to me that role values are never used; can we avoid generating them > in this case? The role names are used; the role metadata isn't used in this change but will definitely be used later on so I'd rather leave it in. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js (right): https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:12: * @type {!Map<string, !WebInspector.ARIAMetadata.Attribute>} On 2016/08/25 18:46:45, lushnikov wrote: > nit: we usually do one-line typedefs > > /** @type {!Map<string, !WebInspector.ARIAMetadata.Attribute>} */ Oops, done, thanks. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:25: WebInspector.ARIAMetadata.instance = new WebInspector.ARIAMetadata(config); On 2016/08/25 18:46:45, lushnikov wrote: > let's design it like WI.CSSMetadata object? This will make our code consistent > > WebInspector.ariaMetadata = function() > { > if (!WebInspector.ARIAMetadata._instance) > WebInspector.ARIAMetadata._instance = new > WebInspector.ARIAMetadata(WebInspector.ARIAMetadata._generatedConfig || null); > return WebInspector.ARIAMetadata._instance; > } Done. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:49: WebInspector.ARIAMetadata.Roles = new WebInspector.ARIAMetadata.ValueSet(Object.keys(roles)); On 2016/08/25 18:46:46, lushnikov wrote: > why store this as a global object? Can we just make this._roles = > Object.keys(roles); A future change will add back this._roles as the set of richer roles metadata (based on the data in ARIAConfig), whereas this is just the set of role names. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:70: * @param {?Array<string>} values On 2016/08/25 18:46:46, lushnikov wrote: > do we ever create it with a null? n/a https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:72: WebInspector.ARIAMetadata.ValueSet = function(values) On 2016/08/25 18:46:46, lushnikov wrote: > Let's get rid of this class altogether - it doesn't do much. > We would be able to filter things ad-hoc in ARIAAttributesView.js:240 Done. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:96: this._name = name; On 2016/08/25 18:46:46, lushnikov wrote: > is this ever used? No. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:105: this._enum = null; On 2016/08/25 18:46:46, lushnikov wrote: > it looks like you never actually expect these to be null - can we enforce this > through constructor non-nullable types? Done. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:118: type: function() On 2016/08/25 18:46:46, lushnikov wrote: > this seems to be unused as well Done. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:126: defaultValue: function() On 2016/08/25 18:46:46, lushnikov wrote: > where do you use this? Not yet used. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:134: enum: function() On 2016/08/25 18:46:46, lushnikov wrote: > are these values? we have a defaultValue, so this would be natural to call > values: as well I'd rather keep it as enum, as it's only relevant for certain types of attributes. (i.e. the value of the attribute may be one of the values of `enum`, iff the type of the attribute is token or boolean) https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:136: return this._enum On 2016/08/25 18:46:45, lushnikov wrote: > nit: missing ; Done. https://codereview.chromium.org/2200893003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:141: WebInspector.ARIAMetadata.AttributeTypes = { On 2016/08/25 18:46:45, lushnikov wrote: > are these ever used? I didn't find any occurrence. Not yet.
On 2016/08/25 18:46:46, lushnikov wrote: > >> The accessibility module is getting big and solid; let's make this a remote > >> module so that we don't drastically increase devtools.grd payload. > > > > Sure, but will we need to reverse that once this comes out of experimental? > > Not really, we currently have a few models which are remote and they behave just > good; we even consider moving more functionality into a remote modules! Oh great. Can you point me at how to do this, and I'll do it in the next CL. > Also, nit: please, prefix the patch title with "DevTools:" label. It helps us > greatly to navigate commit log. [about to be] Done.
Description was changed from ========== Add autocomplete for ARIA. BUG=616950 ========== to ========== DevTools: Add autocomplete for ARIA. BUG=616950 ==========
> Oh great. Can you point me at how to do this, and I'll do it in the next CL. Sure, it should be a one-line change in inspector.json. And probably there are a bit of changes in GYP and GN files. You can use cm_modes module as an example. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html (right): https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html:8: window.debugTest = true; this is a leftover https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js (right): https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:44: if (!("roles" in config)) why do we check for "roles" existence? AFAIU it comes from ARIAConfig where we always have it. https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:50: WebInspector.ARIAMetadata.Roles = Object.keys(roles); May we have this._roleNames for now? As far as we've committed to having WebInspector.ariaMetadata() instance, it would be good to have all the information under this object. https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:57: valueSetForProperty: function(property) There's no such thing as a valueSet anymore; let's rename this. propertyEnum? https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:76: this._default = null; this seems to be a leftover
https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html (right): https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html:8: window.debugTest = true; On 2016/08/26 23:00:16, lushnikov wrote: > this is a leftover This whole test is a leftover D: removed https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js (right): https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:44: if (!("roles" in config)) On 2016/08/26 23:00:16, lushnikov wrote: > why do we check for "roles" existence? AFAIU it comes from ARIAConfig where we > always have it. Done. https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:50: WebInspector.ARIAMetadata.Roles = Object.keys(roles); On 2016/08/26 23:00:16, lushnikov wrote: > May we have this._roleNames for now? As far as we've committed to having > WebInspector.ariaMetadata() instance, it would be good to have all the > information under this object. Done. https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:57: valueSetForProperty: function(property) On 2016/08/26 23:00:16, lushnikov wrote: > There's no such thing as a valueSet anymore; let's rename this. > > propertyEnum? Done - valuesForProperty? https://codereview.chromium.org/2200893003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:76: this._default = null; On 2016/08/26 23:00:16, lushnikov wrote: > this seems to be a leftover Done.
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html (left): https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html:8: function test() You would also want to remove -expected.txt for this test. So this test doesn't actually test anything? https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js (right): https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:44: var roles = config["roles"]; nit: inlining this would be nice https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:54: valuesForProperty: function(property) Feels a little weird that we call it values here and enum in the attribute itself. But works for me!
https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html (left): https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html:8: function test() On 2016/08/26 23:32:20, lushnikov wrote: > You would also want to remove -expected.txt for this test. > > So this test doesn't actually test anything? > Oh, facepalm. This test is needed, but didn't need to be changed. Friday afternoon, man. https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js (right): https://codereview.chromium.org/2200893003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:54: valuesForProperty: function(property) On 2016/08/26 23:32:20, lushnikov wrote: > Feels a little weird that we call it values here and enum in the attribute > itself. But works for me! I can see that. It's kind of an implementation detail.
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Any further changes needed here? Do you want me to do the remote module thing in this change?
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
The CQ bit was unchecked by aboxhall@chromium.org
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Do you want me to do the remote module thing in this change? Let's make it separately; this CL is already big. Thank you, lgtm https://codereview.chromium.org/2200893003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2200893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: WebInspector.ARIAMetadata.config = { can we make this WebInspector.ARIAMetadata._config to signal to possible clients that this should not be used directly?
https://codereview.chromium.org/2200893003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2200893003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: WebInspector.ARIAMetadata.config = { On 2016/08/29 21:51:39, lushnikov wrote: > can we make this WebInspector.ARIAMetadata._config to signal to possible clients > that this should not be used directly? Unfortunately jscompiler complains about that: /usr/local/google/code/cr/src/third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:24: ERROR - Access to private property _config of WebInspector.ARIAMetadata not allowed here. WebInspector.ARIAMetadata._instance = new WebInspector.ARIAMetadata(WebInspector.ARIAMetadata._config); ^
The CQ bit was unchecked by aboxhall@chromium.org
On 2016/08/29 23:08:09, aboxhall wrote: > https://codereview.chromium.org/2200893003/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js > (right): > > https://codereview.chromium.org/2200893003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: > WebInspector.ARIAMetadata.config = { > On 2016/08/29 21:51:39, lushnikov wrote: > > can we make this WebInspector.ARIAMetadata._config to signal to possible > clients > > that this should not be used directly? > > Unfortunately jscompiler complains about that: > /usr/local/google/code/cr/src/third_party/WebKit/Source/devtools/front_end/accessibility/ARIAMetadata.js:24: > ERROR - Access to private property _config of WebInspector.ARIAMetadata not > allowed here. > WebInspector.ARIAMetadata._instance = new > WebInspector.ARIAMetadata(WebInspector.ARIAMetadata._config); > ^ Fixed as per lushnikov's suggestion elsewhere.
The CQ bit was checked by aboxhall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2200893003/#ps220001 (title: "skip_compilation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add autocomplete for ARIA. BUG=616950 ========== to ========== DevTools: Add autocomplete for ARIA. BUG=616950 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add autocomplete for ARIA. BUG=616950 ========== to ========== DevTools: Add autocomplete for ARIA. BUG=616950 Committed: https://crrev.com/515495b524fa67e475f0068809f42f814370866d Cr-Commit-Position: refs/heads/master@{#415094} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/515495b524fa67e475f0068809f42f814370866d Cr-Commit-Position: refs/heads/master@{#415094} |
