Closed Bug 821777 Opened 7 years ago Closed 7 years ago

Populate flash version in telemetry data

Categories

(Firefox for Android Graveyard :: Plugins, defect)

ARM
Android
defect
Not set

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
Firefox 20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now telemtry sends flash version information on desktop, but not on Android. It would be nice to have this.
Comment on attachment 692342 [details] [diff] [review]
Get some kind of version information into nsPluginTag for Flash on Android

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

::: dom/plugins/base/nsPluginHost.cpp
@@ +2947,5 @@
> +    // Flash on Android does not populate the version field, but it is tacked on to the description.
> +    // For example, "Shockwave Flash 11.1 r115"
> +    if (PL_strncmp("Shockwave Flash ", description, 16) == 0 && description[16]) {
> +      version = &description[16];
> +      description[15] = '\0';

Comment on what you're doing here. Looks like you're cutting the version off of the description field so that it doesn't show up later, but it isn't obvious why you're doing that.

I'd recommend not doing it - doesn't seem like potentially messing with compatibility for our previous behavior is worth a slightly cleaner description field.
Attachment #692342 - Flags: review?(joshmoz) → review+
Attachment #692342 - Attachment is obsolete: true
(In reply to Josh Aas (Mozilla Corporation) from comment #2)
> Comment on attachment 692342 [details] [diff] [review]
> Get some kind of version information into nsPluginTag for Flash on Android
> 
> Review of attachment 692342 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/nsPluginHost.cpp
> @@ +2947,5 @@
> > +    // Flash on Android does not populate the version field, but it is tacked on to the description.
> > +    // For example, "Shockwave Flash 11.1 r115"
> > +    if (PL_strncmp("Shockwave Flash ", description, 16) == 0 && description[16]) {
> > +      version = &description[16];
> > +      description[15] = '\0';
> 
> Comment on what you're doing here. Looks like you're cutting the version off
> of the description field so that it doesn't show up later, but it isn't
> obvious why you're doing that.
> 
> I'd recommend not doing it - doesn't seem like potentially messing with
> compatibility for our previous behavior is worth a slightly cleaner
> description field.

Fair enough. Latest patch only sets the version and leaves the description alone. Carryied the r+ forward.
https://hg.mozilla.org/mozilla-central/rev/9ed12964d424
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Comment on attachment 692931 [details] [diff] [review]
Get some kind of version information into nsPluginTag for Flash on Android

[Approval Request Comment]
I want this in Aurora and Beta pronto. Very low risk, gives very useful information about Flash usage in the wild.
Attachment #692931 - Flags: approval-mozilla-beta?
Attachment #692931 - Flags: approval-mozilla-aurora?
Comment on attachment 692931 [details] [diff] [review]
Get some kind of version information into nsPluginTag for Flash on Android

We like low risk Telemetry fixes, and can back this out before our RC if anything goes awry. Please land on mozilla-beta no later than 12/26 to make it into FF18.
Attachment #692931 - Flags: approval-mozilla-beta?
Attachment #692931 - Flags: approval-mozilla-beta+
Attachment #692931 - Flags: approval-mozilla-aurora?
Attachment #692931 - Flags: approval-mozilla-aurora+
Depends on: 840634
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.