1219895 - Cleanup the 'share.1' telemetry probes by adding extra context

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: shareevent-cleanup v0.1

We have a lot of "share.1 > list" probes without any Extra context. This patch adds some context, making it easier to identify the code paths.

I also removed a use-once method, moving the code to where it was used. This made it easier to add the specific context as well.

Comment 1

[:Margaret Leibovic]

Comment on attachment 8680830 [details] [diff] [review]
shareevent-cleanup v0.1

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

::: mobile/android/base/BrowserApp.java
@@ +3225,5 @@
>              return true;
>          }
>  
>          if (itemId == R.id.share) {
> +            Tab tab = Tabs.getInstance().getSelectedTab();

Does this compile? Looks like `tab` is already declared above.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #1)
> Comment on attachment 8680830 [details] [diff] [review]
> shareevent-cleanup v0.1
> 
> Review of attachment 8680830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +3225,5 @@
> >              return true;
> >          }
> >  
> >          if (itemId == R.id.share) {
> > +            Tab tab = Tabs.getInstance().getSelectedTab();
> 
> Does this compile? Looks like `tab` is already declared above.

It does not. I dropped the | Tab | part. I can't figure out why we ever declared 'tab' and 'intent' at the top of the method.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/35d014ddd2a4

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35d014ddd2a4

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/35d014ddd2a4

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8680830 [details] [diff] [review]
shareevent-cleanup v0.1

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: We can always use better telemetry
[Describe test coverage new/current, TreeHerder]: Working fine on Nightly
[Risks and why]: Low. Just adds some extra hints
[String/UUID change made/needed]: None

Comment 7

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8680830 [details] [diff] [review]
shareevent-cleanup v0.1

Should be in 43 beta 2.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce897c30d505

Comment 10

[Carsten Book [:Tomcat]]

this doesn't apply cleanly to beta:

grafting 313464:ce897c30d505 "Bug 1219895 - Cleanup the 'share.1' telemetry probes by adding extra context. r=margaret, a=sylvestre"
merging mobile/android/base/BrowserApp.java
warning: conflicts during merge.
merging mobile/android/base/BrowserApp.java incomplete! (edit conflicts, then use 'hg resolve --mark')
merging mobile/android/base/GeckoApp.java
merging mobile/android/base/home/HomeFragment.java
merging mobile/android/base/prompts/PromptListAdapter.java
merging mobile/android/base/widget/GeckoActionProvider.java
merging toolkit/components/reader/AboutReader.jsm
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ce897c30d505

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4640472b4c72

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

... and bustage fix:
https://hg.mozilla.org/releases/mozilla-beta/rev/74993763ef07

Comment 14

[u421692]

Verified as fixed on Firefox 43 Beta 4