Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2333333004:
Track usage of Windows accessibility APIs with a UMA histogram (Closed)
|
Created:
4 years, 3 months ago by dmazzoni Modified:
4 years, 3 months ago CC:
aboxhall+watch_chromium.org, asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack usage of Windows accessibility APIs with a UMA histogram
BUG=647049
Committed: https://crrev.com/73376f5258b07d54c30271631848c75f49bbd392
Cr-Commit-Position: refs/heads/master@{#419314}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Remove one bad enum, add explicit numbering #Patch Set 3 : Add histograms for several more missing methods #
Total comments: 1
Patch Set 4 : Add period #
Messages
Total messages: 23 (12 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + mpearson@chromium.org, nektar@chromium.org
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.
I did not check to see if every enum you added was emitted somewhere. --mark https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... File content/browser/accessibility/browser_accessibility_win.cc (right): https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:35: enum { Please put a comment here, something like // These values are written to logs. Do not renumber or delete existing items; add new entries to the end of the list. https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:35: enum { optional: Consider an anonymous namespace. https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:37: UMA_API_ACC_HIT_TEST, Please explicitly do =1, =2, etc. This will make it obvious later when someone who doesn't read the comment deletes something. Furthermore, it will make an accidental deletion have a smaller effect (no other entries will get renumbered). (We've had these issues with large enum histograms in the past. That's why I'm asking for this.) https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:183: UMA_API_MAX Comment this last entry. // This enum value must be last. possibly (if you wish) add // It's okay to change this value. https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:1105: return E_NOTIMPL; You're logging the above not implemented function but this or the following ones? https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:2873: return E_NOTIMPL; Not logged? https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:3028: return E_NOTIMPL; Here and below? https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:3498: BrowserAccessibilityWin::GetObjectForChild(long child_id, IAccessibleEx** ret) { ditto https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:3516: return E_NOTIMPL; ditto https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:3565: // Nothing at all in this section? https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:4251: WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_GET_STRING_ATTRIBUTE_AS_BSTR); I thought this was the private method section. Why are you logging this? And if so then what about some of the other private methods around here, e.g. GetTargetFromChildID?
WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_IATEXT_GET_ATTRIBUTES) Shouldn't that be WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_IA2_GET_ATTRIBUTES)?
lgtm
On 2016/09/16 at 16:06:28, nektar wrote: > WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_IATEXT_GET_ATTRIBUTES) > Shouldn't that be > WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_IA2_GET_ATTRIBUTES)? Both IAccessible2 and IAccessibleText have functions that are identically named getAttributes, with slightly different signatures. So we have both.
On 9/16/2016 12:09 PM, dmazzoni@chromium.org wrote: > On 2016/09/16 at 16:06:28, nektar wrote: > > WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_IATEXT_GET_ATTRIBUTES) > > Shouldn't that be > > WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_IA2_GET_ATTRIBUTES)? > > Both IAccessible2 and IAccessibleText have functions that are > identically named > getAttributes, with slightly different signatures. So we have both. > Sorry, I didn't see that. LGTM -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Rietveld won't let me respond inline to some of the comments for some reason, but I went and fixed the sections where I was missing some methods. Thanks for the careful review! https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... File content/browser/accessibility/browser_accessibility_win.cc (right): https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:35: enum { On 2016/09/15 at 23:36:56, Mark P wrote: > optional: Consider an anonymous namespace. Done > Please put a comment here, something like Done https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:37: UMA_API_ACC_HIT_TEST, On 2016/09/15 at 23:36:56, Mark P wrote: > Please explicitly do =1, =2, etc. This will make it obvious later when someone who doesn't read the comment deletes something. Furthermore, it will make an accidental deletion have a smaller effect (no other entries will get renumbered). > > (We've had these issues with large enum histograms in the past. That's why I'm asking for this.) Done. That also makes it easier to spot-check that the numbers match the histograms.xml file. https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:183: UMA_API_MAX On 2016/09/15 at 23:36:56, Mark P wrote: > Comment this last entry. > // This enum value must be last. > possibly (if you wish) add > // It's okay to change this value. Done https://codereview.chromium.org/2333333004/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_win.cc:4251: WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_GET_STRING_ATTRIBUTE_AS_BSTR); On 2016/09/15 at 23:36:56, Mark P wrote: > I thought this was the private method section. Why are you logging this? And if so then what about some of the other private methods around here, e.g. GetTargetFromChildID? Oops, good catch. This one was a mistake.
The CQ bit was checked by dmazzoni@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.
lgtm https://codereview.chromium.org/2333333004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2333333004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:267: + <summary>Tracks usage of all public Windows accessibility APIs</summary> nit: period.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2333333004/#ps60001 (title: "Add period")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Track usage of Windows accessibility APIs with a UMA histogram BUG=647049 ========== to ========== Track usage of Windows accessibility APIs with a UMA histogram BUG=647049 Committed: https://crrev.com/73376f5258b07d54c30271631848c75f49bbd392 Cr-Commit-Position: refs/heads/master@{#419314} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/73376f5258b07d54c30271631848c75f49bbd392 Cr-Commit-Position: refs/heads/master@{#419314} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
