944147 - Collect UITelemetry on bookmark star usage

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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

* Create a bookmark
* Edit a bookmark

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1 - for non-Australis

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1 - for non-Australis

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2 - for non-Australis

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.3 - for non-Australis

Comment 5

[:Gijs (he/him)]

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

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Yep, we don't double-count those things.

non-Australis patch landed on Holly as https://hg.mozilla.org/projects/holly/rev/058506a5ff78

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Landed in mozilla-beta as https://hg.mozilla.org/releases/mozilla-beta/rev/d7915b764302

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 12

[Mike Conley (:mconley) (:⚙️)]

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

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

D'oh. Yep. Will get it a test before landing.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fab9a65035e