Closed Bug 1219895 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox43 --- verified
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(1 file)

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.
Attachment #8680830 - Flags: review?(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.
Attachment #8680830 - Flags: review?(margaret.leibovic) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/35d014ddd2a4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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
Attachment #8680830 - Flags: approval-mozilla-beta?
Attachment #8680830 - Flags: approval-mozilla-aurora?
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Assignee: nobody → mark.finkle
Comment on attachment 8680830 [details] [diff] [review]
shareevent-cleanup v0.1

Should be in 43 beta 2.
Attachment #8680830 - Flags: approval-mozilla-beta?
Attachment #8680830 - Flags: approval-mozilla-beta+
Attachment #8680830 - Flags: approval-mozilla-aurora?
Attachment #8680830 - Flags: approval-mozilla-aurora+
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$
Flags: needinfo?(mark.finkle)
Verified as fixed on Firefox 43 Beta 4
You need to log in before you can comment on or make changes to this bug.