Closed
Bug 1251340
Opened 8 years ago
Closed 8 years ago
Probe to measure if RV item was opened from root Bookmark list or from smart folder
Categories
(Firefox for Android Graveyard :: Metrics, defect)
Firefox for Android Graveyard
Metrics
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: barbara, Assigned: ahunt)
References
Details
Attachments
(1 file)
This will help us understand how useful people will find the smart folder.
Reporter | ||
Updated•8 years ago
|
Blocks: migrate-RL
Comment 1•8 years ago
|
||
We should already be able to do this with existing (or soon to be added) probes. It's a funnel-problem: open-smartfolder -> open reader listitem
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #1) > We should already be able to do this with existing (or soon to be added) > probes. It's a funnel-problem: > open-smartfolder -> open reader listitem If I'm understanding this right, we just need: - Probe to measure entering smartfolder - Probe to measure opening bookmarks, which can distinguish between smartfolder (or root folder, or other folder?) And we can then look for open bookmark events which follow opening the smartfolder (I'm unfamiliar with how the analysis works). Do we also need: - Probe to measure exiting the smartfolder (Otherwise we don't know if a reader view bookmark was opened while the reading-list smartfolder was open, or whether the user returned to the root folder first - assuming my understanding above was correct) Do we want: - Probe to measure opening any folder, and whether it's a top-level subfolder / desktop folder / smartfolder ...? (This would be easy to add now, while I'm in the folder handling code).
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ahunt
Updated•8 years ago
|
Flags: needinfo?(bbermes)
Reporter | ||
Comment 3•8 years ago
|
||
> Do we want:
> - Probe to measure opening any folder, and whether it's a top-level
> subfolder / desktop folder / smartfolder ...? (This would be easy to add
> now, while I'm in the folder handling code).
That's be great, yes. Would help us understand how useful any folder is to users.
Flags: needinfo?(bbermes)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48173/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48173/
Attachment #8743902 -
Flags: review?(margaret.leibovic)
Updated•8 years ago
|
Attachment #8743902 -
Flags: review?(margaret.leibovic)
Comment 5•8 years ago
|
||
Comment on attachment 8743902 [details] MozReview Request: Bug 1251340 - Add probes for entering / exiting bookmark folders r?margaret https://reviewboard.mozilla.org/r/48173/#review44923 ::: mobile/android/base/java/org/mozilla/gecko/TelemetryContract.java:42 (Diff revision 1) > > // Editing an item. > EDIT("edit.1"), > > + // Opening a bookmarks folder. > + FOLDER_OPEN("folder_open.1"), Hm, we're generally pretty reluctant to add new actions... could this be a "show.1" action instead? ::: mobile/android/base/java/org/mozilla/gecko/TelemetryContract.java:45 (Diff revision 1) > > + // Opening a bookmarks folder. > + FOLDER_OPEN("folder_open.1"), > + > + // Closing a bookmarks folder. > + FOLDER_CLOSE("folder_close.1"), I didn't see confirmation from Barbara that we do want this probe to measure leaving the folder, can we ask her if we really need this? And if we do need this data, can we deduce it instead from re-entering parent folders (i.e. other "open" events)? ::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksListView.java:76 (Diff revision 1) > if (adapter.isShowingChildFolder()) { > if (position == 0) { > // If we tap on an opened folder, move back to parent folder. > adapter.moveToParentFolder(); > + > + Telemetry.sendUIEvent(TelemetryContract.Event.FOLDER_CLOSE, TelemetryContract.Method.LIST_ITEM, ""); I'm not sure we need this, although I do see that with this current patch we don't automatically get an event for re-entering the parent folder. Maybe this logic should go directly in `onRefreshFolder`? Although that violates our principe of putting the probe as close to the user action as possible. ::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksListView.java:123 (Diff revision 1) > + } else { > + // The last item in the list corresponds to the bottom of the stack. This will be > + // the fake "mobile bookmarks" folder. The next level up will be the first folder > + // that the user explicitly opened, e.g. desktop bookmarks if they opened that. > + if (parentStack.size() >= 2 && parentStack.get(parentStack.size() - 2).id == Bookmarks.FAKE_DESKTOP_FOLDER_ID) { > + extra = "subfolder-desktop"; Can't desktop folders nest arbitrarily deep? I'm confused about how this logic works for the case where you're many folders down. ::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksListView.java:125 (Diff revision 1) > + // the fake "mobile bookmarks" folder. The next level up will be the first folder > + // that the user explicitly opened, e.g. desktop bookmarks if they opened that. > + if (parentStack.size() >= 2 && parentStack.get(parentStack.size() - 2).id == Bookmarks.FAKE_DESKTOP_FOLDER_ID) { > + extra = "subfolder-desktop"; > + } else { > + extra = "subfolder-mobile"; Nit: For consistency with our other probes, these should use underscores, not dashes.
Assignee | ||
Comment 6•8 years ago
|
||
> mobile/android/base/java/org/mozilla/gecko/home/BookmarksListView.java:123
> (Diff revision 1)
> > + } else {
> > + // The last item in the list corresponds to the bottom of the stack. This will be
> > + // the fake "mobile bookmarks" folder. The next level up will be the first folder
> > + // that the user explicitly opened, e.g. desktop bookmarks if they opened that.
> > + if (parentStack.size() >= 2 && parentStack.get(parentStack.size() - 2).id == Bookmarks.FAKE_DESKTOP_FOLDER_ID) {
> > + extra = "subfolder-desktop";
>
> Can't desktop folders nest arbitrarily deep? I'm confused about how this
> logic works for the case where you're many folders down.
I think we can have arbitrarily deep nesting for both mobile and desktop. My main concern was distinguishing whether we're navigating through desktop vs mobile folders. Maybe we should add the depth to the extra, e.g. subfolder_desktop?depth=1 and subfolder_mobile?depth=2, but I don't know how useful that is, or how analysis is affected.
Assignee | ||
Comment 7•8 years ago
|
||
https://reviewboard.mozilla.org/r/48173/#review44923 > I'm not sure we need this, although I do see that with this current patch we don't automatically get an event for re-entering the parent folder. Maybe this logic should go directly in `onRefreshFolder`? Although that violates our principe of putting the probe as close to the user action as possible. We can check what the parent folder is when exiting a folder, so it's quite easy to just send a SHOW event with the parent folder extra instead of sending FOLDER_CLOSE here.
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8743902 [details] MozReview Request: Bug 1251340 - Add probes for entering / exiting bookmark folders r?margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48173/diff/1-2/
Attachment #8743902 -
Flags: review?(margaret.leibovic)
Comment 9•8 years ago
|
||
(In reply to Andrzej Hunt :ahunt from comment #6) > > mobile/android/base/java/org/mozilla/gecko/home/BookmarksListView.java:123 > > (Diff revision 1) > > > + } else { > > > + // The last item in the list corresponds to the bottom of the stack. This will be > > > + // the fake "mobile bookmarks" folder. The next level up will be the first folder > > > + // that the user explicitly opened, e.g. desktop bookmarks if they opened that. > > > + if (parentStack.size() >= 2 && parentStack.get(parentStack.size() - 2).id == Bookmarks.FAKE_DESKTOP_FOLDER_ID) { > > > + extra = "subfolder-desktop"; > > > > Can't desktop folders nest arbitrarily deep? I'm confused about how this > > logic works for the case where you're many folders down. > > I think we can have arbitrarily deep nesting for both mobile and desktop. My > main concern was distinguishing whether we're navigating through desktop vs > mobile folders. Maybe we should add the depth to the extra, e.g. > subfolder_desktop?depth=1 and subfolder_mobile?depth=2, but I don't know how > useful that is, or how analysis is affected. Ah, I misunderstood the intention here. I don't think depth matters, but I do agree it sounds good to know whether people are using their synced folders.
Comment 10•8 years ago
|
||
Comment on attachment 8743902 [details] MozReview Request: Bug 1251340 - Add probes for entering / exiting bookmark folders r?margaret https://reviewboard.mozilla.org/r/48173/#review47323 ::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksListView.java:95 (Diff revision 2) > + // has a level equal to 1. (Desktop folders will be one level deeper due to the > + // fake desktop folder, hence subtract 2.) > + if (baseFolderID == Bookmarks.FAKE_DESKTOP_FOLDER_ID) { > + return "folder_desktop_subfolder_" + (stackDepth - 2); > + } else { > + return "folder_mobile_subfolder_" + (stackDepth - 1); Given my last comment, I think we could go back to just specifying these as mobile/desktop subfolders. Then we could get rid of this stackDepth parameter and stop my brain from hurting thinking about off-by-one-errors :)
Attachment #8743902 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/de07ed955b6e51bda53802a5440648e0e7b2cc62 Bug 1251340 - Add probes for entering / exiting bookmark folders r=margaret
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/de07ed955b6e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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
•