Add Telemetry probe for content of "Title bar" setting

RESOLVED FIXED in Firefox 40

Status

()

Firefox for Android
Settings and Preferences
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 41
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, firefox41+ fixed, fennec40+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

Showing the page title for a page adds some complication in updating the toolbar state - we could simplify it (and the settings!) but not providing the option to show the page title and just showing the url, as we do by default.

We should get some numbers on how often this feature is used and thus if we can remove it. Currently, we can get the number of times this item was clicked, given the number of times Settings was accessed. Ideally, we find out how many users have "show title" enabled (see bug 1166461).
Anthony, would you be opposed to this?
Flags: needinfo?(alam)

Updated

3 years ago
Blocks: 1069035
Flags: needinfo?(alam)
^whoops

I'm open to it. But I'd like to think about it some more. I can see a lot of pros and cons right now and some telemetry might also help.

Keep in mind, depending on what the default we ship with is and what the user current has selected, just "how many users have "show title" enabled" wont really tell us much about the bigger picture.
Let's add some telemetry!
Assignee: nobody → michael.l.comella
Created attachment 8625796 [details] [diff] [review]
(WIP) Add probe
Comment on attachment 8625796 [details] [diff] [review]
(WIP) Add probe

Margaret, this seems to fail with:

NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsITelemetry.getHistogramById]

Any ideas?
Attachment #8625796 - Flags: feedback?(margaret.leibovic)

Comment 6

2 years ago
(In reply to Michael Comella (:mcomella) from comment #5)
> Comment on attachment 8625796 [details] [diff] [review]
> (WIP) Add probe
> 
> Margaret, this seems to fail with:
> 
> NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsITelemetry.getHistogramById]
> 
> Any ideas?

Did you build toolkit also?
(In reply to :Margaret Leibovic from comment #6)
> Did you build toolkit also?

I feel silly. :d
Created attachment 8626388 [details]
MozReview Request: Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret

Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret
Attachment #8626388 - Flags: review?(margaret.leibovic)
Attachment #8625796 - Attachment is obsolete: true
Attachment #8625796 - Flags: feedback?(margaret.leibovic)

Comment 9

2 years ago
Comment on attachment 8626388 [details]
MozReview Request: Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret

https://reviewboard.mozilla.org/r/12087/#review10489

Ship It!
Attachment #8626388 - Flags: review?(margaret.leibovic) → review+

Comment 10

2 years ago
Comment on attachment 8626388 [details]
MozReview Request: Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret

https://reviewboard.mozilla.org/r/12087/#review10491

::: toolkit/components/telemetry/Histograms.json:8265
(Diff revision 1)
> +    "expires_in_version": "43",

If you land this today, you could actually make this expire in 42. We should also try uplifting this.
Attachment #8626388 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/abc988d2a1fd
(In reply to :Margaret Leibovic from comment #10)
> If you land this today, you could actually make this expire in 42. We should
> also try uplifting this.

Changed.
Comment on attachment 8626388 [details]
MozReview Request: Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret

Approval Request Comment
[Feature/regressing bug #]: bug 1166462.
[User impact if declined]:
  We'd have to wait longer to determine whether or not we can remove the title bar preference feature.

[Describe test coverage new/current, TreeHerder]:
  Tested locally w/ added print statements.

[Risks and why]: 
  Low - TRACKING_PROTECTION_ENABLED follows a similar paradigm so I just copied that.

[String/UUID change made/needed]: None
Attachment #8626388 - Flags: approval-mozilla-aurora?
(In reply to Michael Comella (:mcomella) from comment #12)
> (In reply to :Margaret Leibovic from comment #10)
> > If you land this today, you could actually make this expire in 42. We should
> > also try uplifting this.
> 
> Changed.

...but I forgot to land it. I'm just going to leave it 43.
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/abc988d2a1fd
Adding a tracking flag for FF41. Not sure why this is not tracking for Fennec either.
status-firefox41: --- → affected
tracking-firefox41: --- → +
Comment on attachment 8626388 [details]
MozReview Request: Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret

Approving uplift to Aurora. This patch seems non-invasive and has been in m-c for a while now and we have not heard of any negative impact.
Attachment #8626388 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: --- → 41+
This landed on m-c prior to the uplift.
status-firefox41: affected → fixed
Comment on attachment 8626388 [details]
MozReview Request: Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret

Oh, actually, this was intended to be uplifted to 40.

Approval Request Comment 13
Attachment #8626388 - Flags: approval-mozilla-beta?
tracking-fennec: 41+ → 40+
Summary: Consider removing "Title bar" setting (i.e. show title or url) → Add Telemetry probe for content of "Title bar" setting
Blocks: 1181755
status-firefox40: --- → affected
Comment on attachment 8626388 [details]
MozReview Request: Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret

Low risk and helps the team, taking it.
Attachment #8626388 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for beta uplift.
Flags: needinfo?(michael.l.comella)
Keywords: branch-patch-needed
Created attachment 8633006 [details] [diff] [review]
(40 branch patch) Add probe for title in titlebar enabled setting
Here's a branch patch that I have yet to build - I'll test sometime between today and tomorrow and clear the NI.
Flags: needinfo?(michael.l.comella)
Branch patch seems to work:
  * does not crash on startup :)
  * can load pages (I believe JS threw an error when I didn't add the histogram when initially developing this and prevented this)
  * I locally added a print statement after the Telemetry.addData method and it prints out, meaning the method call is a success

NI Ryan to land (because I don't know if this would be seen otherwise).
Flags: needinfo?(ryanvm)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
Target Milestone: --- → Firefox 41
Flags: needinfo?(ryanvm)
Keywords: branch-patch-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/48ef6d7a9434
status-firefox40: affected → fixed
Blocks: 1198955
You need to log in before you can comment on or make changes to this bug.