1166462 - Add Telemetry probe for content of "Title bar" setting

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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).

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, would you be opposed to this?

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^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.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Let's add some telemetry!

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (WIP) Add probe

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 6

[:Margaret Leibovic]

(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?

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

I feel silly. :d

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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

Comment 9

[:Margaret Leibovic]

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!

Comment 10

[:Margaret Leibovic]

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.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abc988d2a1fd

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag for FF41. Not sure why this is not tracking for Fennec either.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This landed on m-c prior to the uplift.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 20

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for beta uplift.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (40 branch patch) Add probe for title in titlebar enabled setting

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Here's a branch patch that I have yet to build - I'll test sometime between today and tomorrow and clear the NI.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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).

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/48ef6d7a9434