Collect UITelemetry on bookmark star usage

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 29
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 fixed, firefox28 fixed)

Details

(Whiteboard: [Australis:P-][good first verify])

Attachments

(2 attachments, 5 obsolete attachments)

We're interested in collecting a count of how many times the bookmark star is clicked to:

* Create a bookmark
* Edit a bookmark
Created attachment 8339584 [details] [diff] [review]
Patch v1 - for non-Australis
Created attachment 8340526 [details] [diff] [review]
Patch v1.1 - for non-Australis
Attachment #8340526 - Attachment is patch: true
Attachment #8339584 - Attachment is obsolete: true
No longer blocks: 944049
Depends on: 944049
No longer depends on: 944145
Created attachment 8341889 [details] [diff] [review]
Patch v1.2 - for non-Australis
Attachment #8340526 - Attachment is obsolete: true
Created attachment 8343109 [details] [diff] [review]
Patch v1.3 - for non-Australis
Attachment #8341889 - Attachment is obsolete: true
Attachment #8343109 - Flags: review?(gijskruitbosch+bugs)

Comment 5

4 years ago
Comment on attachment 8343109 [details] [diff] [review]
Patch v1.3 - for non-Australis

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

r+, but make sure you don't double-count stuff when you do the general counting and take the urlbar-container into account. :-)
Attachment #8343109 - Flags: review?(gijskruitbosch+bugs) → review+
Yep, we don't double-count those things.

non-Australis patch landed on Holly as https://hg.mozilla.org/projects/holly/rev/058506a5ff78
status-firefox28: --- → fixed
Comment on attachment 8343109 [details] [diff] [review]
Patch v1.3 - for non-Australis

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

None. This just gives BrowserUITelemetry the capability of counting clicks on the bookmark-star in the URL bar.


User impact if declined: 

None.


Testing completed (on m-c, etc.): 

Lots of manual testing on Holly, which has since merged to Aurora.


Risk to taking this patch (and alternatives if risky): 

None.


String or IDL/UUID changes made by this patch:

None.
Attachment #8343109 - Flags: approval-mozilla-beta?
Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.
Whiteboard: [Australis:P1]
Whiteboard: [Australis:P-]
Attachment #8343109 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Landed in mozilla-beta as https://hg.mozilla.org/releases/mozilla-beta/rev/d7915b764302
status-firefox27: --- → fixed
Whiteboard: [Australis:P-] → [Australis:P-][good first verify]
Created attachment 8349058 [details] [diff] [review]
Patch v1 - for Australis

A direct port was not possible due to the fact that the star-button has been superseded by the bookmarks-menu-button. I special-case it in the built-in item handler.

What do you think of this, jaws?
Attachment #8349058 - Flags: review?(jaws)
Comment on attachment 8349058 [details] [diff] [review]
Patch v1 - for Australis

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

::: browser/modules/BrowserUITelemetry.jsm
@@ +269,5 @@
> +      return null;
> +    }
> +  }
> +
> +  return aNode.id + ":child";

Why is the :child here necessary? It seems that none of the other code here cares that :child is appended. This patch could work just fine if nothing was appended to the ID (and the code that uses this function was updated).
Attachment #8349058 - Flags: review?(jaws) → review-
Created attachment 8349181 [details] [diff] [review]
Patch v1.2 - for Australis

Thanks - removed the :child suffix. That was a carry-over from the non-Australis patch. It *might* need to come back when I do the built-in toolbar item patch, but we'll see when we get there.
Attachment #8349058 - Attachment is obsolete: true
Attachment #8349181 - Flags: review?(jaws)
Comment on attachment 8349181 [details] [diff] [review]
Patch v1.2 - for Australis

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

::: browser/modules/BrowserUITelemetry.jsm
@@ +214,5 @@
> +
> +    // We special-case the bookmarks-menu-button, since we want to
> +    // monitor more than just clicks on it.
> +    if (item.id == "bookmarks-menu-button" ||
> +        getIDBasedOnFirstIDedAncestor(item) == "bookmarks-menu-button:child") {

This shouldn't have the :child on the string anymore.
Attachment #8349181 - Flags: review?(jaws) → review+
D'oh. Yep. Will get it a test before landing.
Created attachment 8349184 [details] [diff] [review]
Patch v1.3 - for Australis (r+'d by jaws)

Yes, this works. Thanks jaws!

Landed on fx-team as: https://hg.mozilla.org/integration/fx-team/rev/3fab9a65035e
Attachment #8349181 - Attachment is obsolete: true
Attachment #8349184 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3fab9a65035e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.