Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
608853006:
Reland fix ChromeVox Next compile. (Closed)
|
Created:
6 years, 2 months ago by David Tseng Modified:
6 years, 2 months ago Reviewers:
Peter Lundblad CC:
chromium-reviews, extensions-reviews_chromium.org, oshima+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nkostylev+watch_chromium.org, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Project:
chromium Visibility:
Public. |
DescriptionReland fix ChromeVox Next compile.
Committed: https://crrev.com/1784cc3cd030e41f35e74a6268bcbf8fd50aa4fe
Cr-Commit-Position: refs/heads/master@{#298054}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : Gen enums. #Patch Set 4 : Begin and end comments for auto generated content. #
Total comments: 6
Patch Set 5 : Address nits. #Messages
Total messages: 16 (2 generated)
dtseng@chromium.org changed reviewers: + plundblad@chromium.org
I'll let you have a peek this time :).
Hi, Looks much better now. I feel pretty strongly about the enum issue because I feel it adds technical debt to pay back later. https://codereview.chromium.org/608853006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js (right): https://codereview.chromium.org/608853006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:37: cvox2.AutomationPredicates.makeRolePredicate('link'); We started to discuss this in a different thread. I think you could add enum constants for the small number of enum values that we need right now. I think that's far more maintainable than starting to have string literals that the compiler can't check all over the code. We can then try to add externs generation to the json_schema_compiler to get rid of *all* duplication in the externs. https://codereview.chromium.org/608853006/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js (right): https://codereview.chromium.org/608853006/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:8: * nit: Do you need this extra blank line?
I disagree about the enum issue. This is no different than DOM event APIs. I can make a change in the automation API to throw an exception if an invalid event is registered. On Thu, Oct 2, 2014 at 4:04 AM, <plundblad@chromium.org> wrote: > Hi, > > Looks much better now. I feel pretty strongly about the enum issue because > I feel it adds technical debt to pay back later. > > > > > https://codereview.chromium.org/608853006/diff/1/chrome/ > browser/resources/chromeos/chromevox/cvox2/background/automation_util.js > File > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/automation_util.js > (right): > > https://codereview.chromium.org/608853006/diff/1/chrome/ > browser/resources/chromeos/chromevox/cvox2/background/ > automation_util.js#newcode37 > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/automation_util.js:37: > cvox2.AutomationPredicates.makeRolePredicate('link'); > We started to discuss this in a different thread. I think you could add > enum > constants for the small number of enum values that we need right now. > I think that's far more maintainable than starting to have string > literals that the compiler can't check all over the code. > We can then try to add externs generation to the json_schema_compiler to > get rid of *all* duplication in the externs. > > https://codereview.chromium.org/608853006/diff/1/chrome/ > browser/resources/chromeos/chromevox/cvox2/background/background.js > File > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js > (right): > > https://codereview.chromium.org/608853006/diff/1/chrome/ > browser/resources/chromeos/chromevox/cvox2/background/ > background.js#newcode8 > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/background.js:8: > * > nit: Do you need this extra blank line? > > https://codereview.chromium.org/608853006/ > > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Alternatively, if you insist, I can do (in ChromeVox):
addVerifiedEvent(node, eventName, callback, cpature) {
if (eventName in chrome.automation.EventType)
node.add...
else
throw
}
Haven't checked this will make the compiler happy...but if it does, and I
fix the other nit, does this cl look good?
On Thu, Oct 2, 2014 at 7:36 AM, David Tseng <dtseng@chromium.org> wrote:
> I disagree about the enum issue. This is no different than DOM event APIs.
>
> I can make a change in the automation API to throw an exception if an
> invalid event is registered.
>
>
> On Thu, Oct 2, 2014 at 4:04 AM, <plundblad@chromium.org> wrote:
>
>> Hi,
>>
>> Looks much better now. I feel pretty strongly about the enum issue
>> because
>> I feel it adds technical debt to pay back later.
>>
>>
>>
>>
>> https://codereview.chromium.org/608853006/diff/1/chrome/
>> browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
>> File
>> chrome/browser/resources/chromeos/chromevox/cvox2/
>> background/automation_util.js
>> (right):
>>
>> https://codereview.chromium.org/608853006/diff/1/chrome/
>> browser/resources/chromeos/chromevox/cvox2/background/
>> automation_util.js#newcode37
>> chrome/browser/resources/chromeos/chromevox/cvox2/
>> background/automation_util.js:37:
>> cvox2.AutomationPredicates.makeRolePredicate('link');
>> We started to discuss this in a different thread. I think you could add
>> enum
>> constants for the small number of enum values that we need right now.
>> I think that's far more maintainable than starting to have string
>> literals that the compiler can't check all over the code.
>> We can then try to add externs generation to the json_schema_compiler to
>> get rid of *all* duplication in the externs.
>>
>> https://codereview.chromium.org/608853006/diff/1/chrome/
>> browser/resources/chromeos/chromevox/cvox2/background/background.js
>> File
>> chrome/browser/resources/chromeos/chromevox/cvox2/
>> background/background.js
>> (right):
>>
>> https://codereview.chromium.org/608853006/diff/1/chrome/
>> browser/resources/chromeos/chromevox/cvox2/background/
>> background.js#newcode8
>> chrome/browser/resources/chromeos/chromevox/cvox2/
>> background/background.js:8:
>> *
>> nit: Do you need this extra blank line?
>>
>> https://codereview.chromium.org/608853006/
>>
>>
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to chromium-reviews+unsubscribe@chromium.org.
>>
>
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
Another idea: could we autogenerate an externs.js from the extension api json file? That'd be the best of both worlds, we'd get a compile-time check but we wouldn't have to maintain them. On Thu, Oct 2, 2014 at 7:41 AM, David Tseng <dtseng@chromium.org> wrote: > Alternatively, if you insist, I can do (in ChromeVox): > addVerifiedEvent(node, eventName, callback, cpature) { > if (eventName in chrome.automation.EventType) > node.add... > else > throw > } > > Haven't checked this will make the compiler happy...but if it does, and I > fix the other nit, does this cl look good? > > On Thu, Oct 2, 2014 at 7:36 AM, David Tseng <dtseng@chromium.org> wrote: > >> I disagree about the enum issue. This is no different than DOM event APIs. >> >> I can make a change in the automation API to throw an exception if an >> invalid event is registered. >> >> >> On Thu, Oct 2, 2014 at 4:04 AM, <plundblad@chromium.org> wrote: >> >>> Hi, >>> >>> Looks much better now. I feel pretty strongly about the enum issue >>> because >>> I feel it adds technical debt to pay back later. >>> >>> >>> >>> >>> https://codereview.chromium.org/608853006/diff/1/chrome/ >>> browser/resources/chromeos/chromevox/cvox2/background/automation_util.js >>> File >>> chrome/browser/resources/chromeos/chromevox/cvox2/ >>> background/automation_util.js >>> (right): >>> >>> https://codereview.chromium.org/608853006/diff/1/chrome/ >>> browser/resources/chromeos/chromevox/cvox2/background/ >>> automation_util.js#newcode37 >>> chrome/browser/resources/chromeos/chromevox/cvox2/ >>> background/automation_util.js:37: >>> cvox2.AutomationPredicates.makeRolePredicate('link'); >>> We started to discuss this in a different thread. I think you could add >>> enum >>> constants for the small number of enum values that we need right now. >>> I think that's far more maintainable than starting to have string >>> literals that the compiler can't check all over the code. >>> We can then try to add externs generation to the json_schema_compiler to >>> get rid of *all* duplication in the externs. >>> >>> https://codereview.chromium.org/608853006/diff/1/chrome/ >>> browser/resources/chromeos/chromevox/cvox2/background/background.js >>> File >>> chrome/browser/resources/chromeos/chromevox/cvox2/ >>> background/background.js >>> (right): >>> >>> https://codereview.chromium.org/608853006/diff/1/chrome/ >>> browser/resources/chromeos/chromevox/cvox2/background/ >>> background.js#newcode8 >>> chrome/browser/resources/chromeos/chromevox/cvox2/ >>> background/background.js:8: >>> * >>> nit: Do you need this extra blank line? >>> >>> https://codereview.chromium.org/608853006/ >>> >>> >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to chromium-reviews+unsubscribe@chromium.org. >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yeah; I agree generating externs for the whole of Chrome extension APIs makes sense. There are some tricky details because, as mentioned before, all objects are just dictionaries and not proper js types (so you can't instantiate them for example). Enums are the same. For the automation API, I did something special to get the enum names bound to chrome.automation. Anyway, it sounds like the proper solution will take some time especially with kalman out, so I still think doing the runtime check should be fine, if the bare string is objectionalbe. Listing things in externs centralizes any errors, but doesn't prevent errors for one, and we'd have to maintain and delete them anyway once we came up with a better solution; and there would be a lot more than what's currently used (roles, events, and states). All other cls in CV next depend on this one, so would be great to resolve this soon. On Thu, Oct 2, 2014 at 7:57 AM, Dominic Mazzoni <dmazzoni@google.com> wrote: > Another idea: could we autogenerate an externs.js from the extension api > json file? That'd be the best of both worlds, we'd get a compile-time check > but we wouldn't have to maintain them. > > > On Thu, Oct 2, 2014 at 7:41 AM, David Tseng <dtseng@chromium.org> wrote: > >> Alternatively, if you insist, I can do (in ChromeVox): >> addVerifiedEvent(node, eventName, callback, cpature) { >> if (eventName in chrome.automation.EventType) >> node.add... >> else >> throw >> } >> >> Haven't checked this will make the compiler happy...but if it does, and I >> fix the other nit, does this cl look good? >> >> On Thu, Oct 2, 2014 at 7:36 AM, David Tseng <dtseng@chromium.org> wrote: >> >>> I disagree about the enum issue. This is no different than DOM event >>> APIs. >>> >>> I can make a change in the automation API to throw an exception if an >>> invalid event is registered. >>> >>> >>> On Thu, Oct 2, 2014 at 4:04 AM, <plundblad@chromium.org> wrote: >>> >>>> Hi, >>>> >>>> Looks much better now. I feel pretty strongly about the enum issue >>>> because >>>> I feel it adds technical debt to pay back later. >>>> >>>> >>>> >>>> >>>> https://codereview.chromium.org/608853006/diff/1/chrome/ >>>> browser/resources/chromeos/chromevox/cvox2/background/ >>>> automation_util.js >>>> File >>>> chrome/browser/resources/chromeos/chromevox/cvox2/ >>>> background/automation_util.js >>>> (right): >>>> >>>> https://codereview.chromium.org/608853006/diff/1/chrome/ >>>> browser/resources/chromeos/chromevox/cvox2/background/ >>>> automation_util.js#newcode37 >>>> chrome/browser/resources/chromeos/chromevox/cvox2/ >>>> background/automation_util.js:37: >>>> cvox2.AutomationPredicates.makeRolePredicate('link'); >>>> We started to discuss this in a different thread. I think you could add >>>> enum >>>> constants for the small number of enum values that we need right now. >>>> I think that's far more maintainable than starting to have string >>>> literals that the compiler can't check all over the code. >>>> We can then try to add externs generation to the json_schema_compiler to >>>> get rid of *all* duplication in the externs. >>>> >>>> https://codereview.chromium.org/608853006/diff/1/chrome/ >>>> browser/resources/chromeos/chromevox/cvox2/background/background.js >>>> File >>>> chrome/browser/resources/chromeos/chromevox/cvox2/ >>>> background/background.js >>>> (right): >>>> >>>> https://codereview.chromium.org/608853006/diff/1/chrome/ >>>> browser/resources/chromeos/chromevox/cvox2/background/ >>>> background.js#newcode8 >>>> chrome/browser/resources/chromeos/chromevox/cvox2/ >>>> background/background.js:8: >>>> * >>>> nit: Do you need this extra blank line? >>>> >>>> https://codereview.chromium.org/608853006/ >>>> >>>> >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to chromium-reviews+unsubscribe@chromium.org. >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok; https://codereview.chromium.org/626653002/ generates at *least* enums from the idl/json formatted extension api specifications. I've included the generated output for now in our externs with the idea that we'll eventually use the above cl for all extension apis. Will wait on kalman for further direction / some more time to spend working out all the mappings to annotations.
Neat!!! On Thu, Oct 2, 2014 at 3:10 PM, <dtseng@chromium.org> wrote: > Ok; > https://codereview.chromium.org/626653002/ > generates at *least* enums from the idl/json formatted extension api > specifications. > > I've included the generated output for now in our externs with the idea > that > we'll eventually use the above cl for all extension apis. Will wait on > kalman > for further direction / some more time to spend working out all the > mappings to > annotations. > > > https://codereview.chromium.org/608853006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm Thanks very much for going the extra km for the sake of static checking! BTW, I am happy to work on the extern generation in the schema compiler if you want, just let me know. Minor things below. https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js (right): https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1262: * @param {string} eventType Can this be annotated with the actual enum type? I know tht works if the enum is a number at least. https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1271: * @param {string} eventType Same here, could the enum type be used? https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1308: activedescendantchanged: 'activedescendantchanged', nit: For this we could leave the actual string values empty since they are not used. For now can leave as-is. https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js (right): https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:20: * @param {string} role Can we use the enum type here? https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js (right): https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:148: current = current.role == 'inlineTextBox' ? nit: keep using the exposed enum value. https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:156: current = current.role == 'inlineTextBox' ? nit: use the exposed enum.
On Fri 03 Oct 2014 05:03:52 AM PDT, plundblad@chromium.org wrote: > lgtm > > Thanks very much for going the extra km for the sake of static checking! > BTW, I am happy to work on the extern generation in the schema > compiler if you want, just let me know. Not sure what you're working on next. If you have some time, wouldn't mind handing off the cl to you. Whoever gets to it first. Let me know if you start on it. > Minor things below. > > > > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > File > chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1262: > * @param {string} eventType > Can this be annotated with the actual enum type? I know tht works if the > enum is a number at least. Works (despite defs being below). > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1271: > * @param {string} eventType > Same here, could the enum type be used? See above. > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1308: > activedescendantchanged: 'activedescendantchanged', > nit: For this we could leave the actual string values empty since they > are > not used. For now can leave as-is. > No strong opinions either way. > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > File > chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:20: > * @param {string} role > Can we use the enum type here? > Done. > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > File > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:148: > current = current.role == 'inlineTextBox' ? > nit: keep using the exposed enum value. Done. > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:156: > current = current.role == 'inlineTextBox' ? > nit: use the exposed enum. Done. > > https://codereview.chromium.org/608853006/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri 03 Oct 2014 05:03:52 AM PDT, plundblad@chromium.org wrote: > lgtm > > Thanks very much for going the extra km for the sake of static checking! > BTW, I am happy to work on the extern generation in the schema > compiler if you want, just let me know. Not sure what you're working on next. If you have some time, wouldn't mind handing off the cl to you. Whoever gets to it first. Let me know if you start on it. > Minor things below. > > > > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > File > chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1262: > * @param {string} eventType > Can this be annotated with the actual enum type? I know tht works if the > enum is a number at least. Works (despite defs being below). > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1271: > * @param {string} eventType > Same here, could the enum type be used? See above. > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:1308: > activedescendantchanged: 'activedescendantchanged', > nit: For this we could leave the actual string values empty since they > are > not used. For now can leave as-is. > No strong opinions either way. > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > File > chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:20: > * @param {string} role > Can we use the enum type here? > Done. > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > File > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:148: > current = current.role == 'inlineTextBox' ? > nit: keep using the exposed enum value. Done. > > https://codereview.chromium.org/608853006/diff/60001/chrome/browser/resources... > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:156: > current = current.role == 'inlineTextBox' ? > nit: use the exposed enum. Done. > > https://codereview.chromium.org/608853006/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > On Fri, Oct 3, 2014 at 5:03 AM, <plundblad@chromium.org> wrote: > lgtm > > Thanks very much for going the extra km for the sake of static checking! > BTW, I am happy to work on the extern generation in the schema > compiler if you want, just let me know. > Minor things below. > > > > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/common/ > chrome_extension_externs.js > File > chrome/browser/resources/chromeos/chromevox/common/ > chrome_extension_externs.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/common/ > chrome_extension_externs.js#newcode1262 > chrome/browser/resources/chromeos/chromevox/common/ > chrome_extension_externs.js:1262: > * @param {string} eventType > Can this be annotated with the actual enum type? I know tht works if the > enum is a number at least. > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/common/ > chrome_extension_externs.js#newcode1271 > chrome/browser/resources/chromeos/chromevox/common/ > chrome_extension_externs.js:1271: > * @param {string} eventType > Same here, could the enum type be used? > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/common/ > chrome_extension_externs.js#newcode1308 > chrome/browser/resources/chromeos/chromevox/common/ > chrome_extension_externs.js:1308: > activedescendantchanged: 'activedescendantchanged', > nit: For this we could leave the actual string values empty since they > are > not used. For now can leave as-is. > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/automation_util.js > File > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/automation_util.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/automation_util.js#newcode20 > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/automation_util.js:20: > * @param {string} role > Can we use the enum type here? > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js > File > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js > (right): > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/background.js#newcode148 > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/background.js:148: > current = current.role == 'inlineTextBox' ? > nit: keep using the exposed enum value. > > https://codereview.chromium.org/608853006/diff/60001/ > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/background.js#newcode156 > chrome/browser/resources/chromeos/chromevox/cvox2/ > background/background.js:156: > current = current.role == 'inlineTextBox' ? > nit: use the exposed enum. > > > https://codereview.chromium.org/608853006/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/608853006/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 9af73515e99c0cf2e719fb857cd2798089440df4
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1784cc3cd030e41f35e74a6268bcbf8fd50aa4fe Cr-Commit-Position: refs/heads/master@{#298054} |
