Closed
Bug 1100914
Opened 10 years ago
Closed 9 years ago
Switch context menu telemetry to a serialized array of states instead of a single string
Categories
(Firefox :: Menus, defect)
Tracking
()
People
(Reporter: VarCat, Assigned: Gijs)
Details
Attachments
(1 file)
2.00 KB,
patch
|
jaws
:
review+
bwinton
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•10 years ago
|
||
FF 34 Build Id:20141117202603 STR: 1. Start the Firefox promotional video from about:home page. 2. On about:home page select some of the characters from the page. 3. Right click on the video. 4. Press Show Statistics. Issue: A context menu is opened that displays option specifics to both media and selection.
Reporter | ||
Comment 2•10 years ago
|
||
The interaction with the context menu even if it's related to media it will be recorded under selection on about:telemetry simple measurements.
Assignee | ||
Comment 3•10 years ago
|
||
Thanks, Catalin! So, the state of the context menu here is, IMO, correct. There is selected text, and if you then right click, we always display the selection part of the menu (also so you can then right click elsewhere and still have it work, AIUI, and equally I'm not sure how trivial it is to figure out if you did right click within the selection or not, especially if the selection is part of the same text node as the one you clicked). Equally, it would be unexpected if we dropped the video part of the menu if you right-clicked the video... Jared, is that right? Assuming I'm right about the menu itself, what do we want for the telemetry here? It's going to be wrong whatever we end up picking. We could make the "type" string we currently pass concatenate all the states that apply instead, I guess? Would that be less weird? Blake?
Flags: needinfo?(jaws)
Flags: needinfo?(bwinton)
Comment 4•10 years ago
|
||
Instead of a concatenation, I think an array of applicable type-strings would make more sense… (If possible.)
Flags: needinfo?(bwinton)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #4) > Instead of a concatenation, I think an array of applicable type-strings > would make more sense… (If possible.) These are object keys in the JSON that's sent. You can't use arrays as regular JS object keys - those are always strings.
Comment 6•10 years ago
|
||
Could I sell you on a JSON-stringify-ed copy of the array, then? (If not, we can parse whatever format back into an array when we do the analysis, but having it be array-like would simplify things on the back end. ;)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Blake Winton (:bwinton) from comment #6) > Could I sell you on a JSON-stringify-ed copy of the array, then? (If not, > we can parse whatever format back into an array when we do the analysis, but > having it be array-like would simplify things on the back end. ;) ugly, but it would work, I guess. :-( I can see what I can do over the rest of this week (but I keep telling people that, so you may need to wait longer). Also, how do we deal with the changing format? Is that not going to be an issue? :-)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bwinton)
Assignee | ||
Comment 8•10 years ago
|
||
I guess taking for accountability purposes, but I am wary of trying to make this make 34... is that OK?
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 3
Flags: qe-verify+
Flags: needinfo?(gijskruitbosch+bugs)
Flags: firefox-backlog+
Summary: Media context menu interaction recorded as selection context menu interaction. → Switch context menu telemetry to a serialized array of states instead of a single string
Comment 9•10 years ago
|
||
We could check for a leading "["… I suppose if we switched to a more backwards-compatible format, then we could handle older versions as well as newer versions, but I think having the format be obviously different will help us tell whether this is a click on just a selection (new-style), or on a selection possibly with other stuff (old-style)… It would be nice to get it in, but we're all quite busy, so if it doesn't make it, I'm not going to complain. Well, not too loudly… ;) (Thanks for working on it!)
Flags: needinfo?(bwinton)
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Assignee | ||
Comment 10•10 years ago
|
||
This should do the trick, I think? Blake, can you check that you can work with this data? I expect it will take some work to tease out what you want considering that now e.g. hits for 'video' will be spread out between all the things that have video as part of the bools that were true at the point where the user context-clicked...
Attachment #8526069 -
Flags: review?(jaws)
Attachment #8526069 -
Flags: feedback?(bwinton)
Comment 11•10 years ago
|
||
Comment on attachment 8526069 [details] [diff] [review] switch telemetry to use an array of strings to indicate all states, This seems good to me, and I'm going to keep this comment short so that I can mid-air Ilana, who probably also has some thoughts. ;)
Attachment #8526069 -
Flags: feedback?(bwinton) → feedback+
Comment 12•9 years ago
|
||
Comment on attachment 8526069 [details] [diff] [review] switch telemetry to use an array of strings to indicate all states, Review of attachment 8526069 [details] [diff] [review]: ----------------------------------------------------------------- Are there no tests that need to be updated? If not, we should probably add some.
Attachment #8526069 -
Flags: review?(jaws) → review-
Comment 13•9 years ago
|
||
Comment on attachment 8526069 [details] [diff] [review] switch telemetry to use an array of strings to indicate all states, Review of attachment 8526069 [details] [diff] [review]: ----------------------------------------------------------------- Discussed over IRC. We need tests on the server side to make sure that they are parsing and displaying the data correctly. Probably introduce a sort of Talos-style email alert if large variations in the data are seen. Gijs will be filing a bug for this. Changing review status to r+ accordingly.
Attachment #8526069 -
Flags: review- → review+
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/35f07c2d524b Filed bug 1104368 about the telemetry alerts
Flags: needinfo?(jaws)
Updated•9 years ago
|
Iteration: 36.3 → 37.1
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/35f07c2d524b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•9 years ago
|
QA Contact: catalin.varga
Reporter | ||
Comment 16•9 years ago
|
||
Hi Gij I've tested the scenario described in the bug on the latest Nightly and it's not reproducing anymore but I was wondering if that's enough to verify the bug, as this fix seems more complex?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Catalin Varga [QA][:VarCat] from comment #16) > Hi Gij I've tested the scenario described in the bug on the latest Nightly > and it's not reproducing anymore but I was wondering if that's enough to > verify the bug, as this fix seems more complex? It would make sense to verify that the current behaviour is correct, ie the right arrays get logged in telemetry if you click in ways that satisfy more than one of the criteria (e.g. right click on a video while having a selection). If that is not clear enough, maybe check with Blake (:bwinton) about the best way to verify the fix here.
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 18•9 years ago
|
||
Verified as fixed using: FF 36 Build Id: 20150104004008 OS: Win 7 x64.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•