Closed Bug 728905 Opened 10 years ago Closed 10 years ago

telemetry ZoomText, YouDao Dictionary and Kazaguru

Categories

(Core :: Disability Access APIs, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: surkov, Assigned: capella)

References

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 2 obsolete files)

ZoomText dll is WebFinderRemote.dll

1) Add new constant for this dll (http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/Compatibility.h#103)
1) Check new consumer in Compatibility::Init()
2) Fix TelemetryHistograms.h (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistograms.h#59)

Trevor, would be nice if you can steal mentoring.
OS: All → Windows 7
making Trevor a mentor per irc
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++] → [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
At first glance, it looks like we add / change the following code snips:

compatibility.h:

   /**
    * List of detected consumers of a11y (used for statistics/telemetry)
    */
   enum {
     NVDA = 0,
     JAWS = 1,
     OLDJAWS = 2,
     WE = 3,
     DOLPHIN = 4,
     SEROTEK = 5,
     COBRA = 6,
     ZOOMHOOK = 7
   };

telemetryhistograms.h:

   HISTOGRAM(A11Y_CONSUMERS, 1, 7, 8, LINEAR, "Accessibility client by enum id")

compatibility.cpp

  if (::GetModuleHandleW(L"WebFinderRemote"))
    statistics::A11yConsumers(ZOOMTEXT);


The last part is where my questions are... I'm assuming no special sMode, nor new tab switching code as with JAWS / WE / DOLPHIN. Also assuming that the handle is the name of the DLL.

How is the packaging of the code done, that is, where do the DLL's reside? Local to the client or remote at MOZILLA or DLL provider perhaps?
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Added mentor to the cc list ...
(In reply to Mark Capella [:capella] from comment #2)

> The last part is where my questions are... I'm assuming no special sMode,
> nor new tab switching code as with JAWS / WE / DOLPHIN.

yes

> Also assuming that
> the handle is the name of the DLL.

not sure what you mean
 
> How is the packaging of the code done, that is, where do the DLL's reside?
> Local to the client or remote at MOZILLA or DLL provider perhaps?

locally, this dll is a part of 3d party application - zoom text (http://www.aisquared.com/zoomtext). If the user installed it and running it together with Firefox then this code triggers and we collect this data.
> > Also assuming that
> > the handle is the name of the DLL.
> 
> not sure what you mean

I think he wanted to make sure the dll's name was the thing to pass to GetModuleHandleW() which I believe is correct.
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > > Also assuming that
> > > the handle is the name of the DLL.
> > 
> > not sure what you mean
> 
> I think he wanted to make sure the dll's name was the thing to pass to
> GetModuleHandleW() which I believe is correct.

Ah, yes, that's right. GetModuleHandle takes dll file name.
Attached patch Patch (v1) (obsolete) — Splinter Review
Code changes effect a clean local clobber build. 

How to test soon becomes relevent ...
Attachment #600660 - Flags: review?(trev.saunders)
Comment on attachment 600660 [details] [diff] [review]
Patch (v1)

canceling review for bug 730198 comment 3
Attachment #600660 - Flags: review?(trev.saunders)
Attached patch Patch (v2) (obsolete) — Splinter Review
Rolled patch for bug73198 and bug728905 into this one ...
Attachment #600660 - Attachment is obsolete: true
Attachment #600671 - Flags: review?(trev.saunders)
Sorry ... rolled in bug730198
Attachment #600671 - Flags: review?(trev.saunders)
Attachment #600671 - Flags: review+
Attachment #600671 - Flags: feedback?(surkov.alexander)
Duplicate of this bug: 730198
Summary: telemetry ZoomText → telemetry ZoomText, YouDao Dictionary and Kazaguru
Comment on attachment 600671 [details] [diff] [review]
Patch (v2)

Review of attachment 600671 [details] [diff] [review]:
-----------------------------------------------------------------

f=me with comment addressed

::: accessible/src/msaa/Compatibility.h
@@ +109,5 @@
>      SEROTEK = 5,
> +    COBRA = 6,
> +    ZOOMTEXT = 7,
> +    KAZAHOOK = 8,
> +    TEXTEXTRACTOR = 9

I'd prefer application names, that's better for readability
Attachment #600671 - Flags: feedback?(surkov.alexander) → feedback+
Attached patch Patch (v3)Splinter Review
Attachment #600671 - Attachment is obsolete: true
Attachment #600683 - Flags: feedback?(surkov.alexander)
Comment on attachment 600683 [details] [diff] [review]
Patch (v3)

Review of attachment 600683 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/Compatibility.cpp
@@ +123,5 @@
> +  if (::GetModuleHandleW(L"Kazahook"))
> +    statistics::A11yConsumers(KAZAGURU);
> +
> +  if ((::GetModuleHandleW(L"TextExtractorImpl32")) ||
> +      (::GetModuleHandleW(L"TextExtractorImpl64")))

I'm curious why do you wrap the method call by braces, otherwise is fine, f+, again :)
Attachment #600683 - Flags: feedback?(surkov.alexander) → feedback+
Oh.... hmmm... cut 'n paste overkill  :-(

Do I need to re-ask for review+ to move forward? Since I don't have L1 I usually have to get a review+ then I can AUTOLAND to TRY, when that passes I can CHECKIN-NEEDED ... 

(On our last bug728904 together, you did alot of this for me.)
I'll land it, don't worry. I do that for a11y patches.
https://hg.mozilla.org/mozilla-central/rev/a32cc7faa71a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.