Closed
Bug 1166462
Opened 9 years ago
Closed 9 years ago
Add Telemetry probe for content of "Title bar" setting
Categories
(Firefox for Android Graveyard :: Settings and Preferences, defect)
Tracking
(firefox40 fixed, firefox41+ fixed, fennec40+)
RESOLVED
FIXED
Firefox 41
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(2 files, 1 obsolete file)
40 bytes,
text/x-review-board-request
|
ritu
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details |
2.29 KB,
patch
|
Details | Diff | Splinter Review |
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).
Updated•9 years ago
|
Blocks: menu-reorg
Flags: needinfo?(alam)
Comment 2•9 years ago
|
||
^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.
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
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•9 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?
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #6) > Did you build toolkit also? I feel silly. :d
Assignee | ||
Comment 8•9 years ago
|
||
Bug 1166462 - Add probe for title in titlebar enabled setting. r=margaret
Attachment #8626388 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•9 years ago
|
Attachment #8625796 -
Attachment is obsolete: true
Attachment #8625796 -
Flags: feedback?(margaret.leibovic)
Comment 9•9 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•9 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+
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/abc988d2a1fd
Assignee | ||
Comment 12•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
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?
Assignee | ||
Comment 14•9 years ago
|
||
(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]
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+
Assignee | ||
Updated•9 years ago
|
tracking-fennec: --- → 41+
Comment 18•9 years ago
|
||
This landed on m-c prior to the uplift.
Assignee | ||
Comment 19•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
tracking-fennec: 41+ → 40+
Assignee | ||
Updated•9 years ago
|
Summary: Consider removing "Title bar" setting (i.e. show title or url) → Add Telemetry probe for content of "Title bar" setting
Updated•9 years ago
|
status-firefox40:
--- → affected
Comment 20•9 years ago
|
||
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+
Comment 21•9 years ago
|
||
Needs rebasing for beta uplift.
Flags: needinfo?(michael.l.comella)
Keywords: branch-patch-needed
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Comment 23•9 years ago
|
||
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)
Assignee | ||
Comment 24•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
Target Milestone: --- → Firefox 41
Updated•9 years ago
|
Flags: needinfo?(ryanvm)
Updated•9 years ago
|
Keywords: branch-patch-needed
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•