Closed Bug 1251340 Opened 4 years ago Closed 4 years ago

Probe to measure if RV item was opened from root Bookmark list or from smart folder

Categories

(Firefox for Android :: Metrics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: barbara, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This will help us understand how useful people will find the smart folder.
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
Depends on: 1257345
(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: nobody → ahunt
Flags: needinfo?(bbermes)
> 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)
Attachment #8743902 - Flags: review?(margaret.leibovic)
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.
> 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.
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.
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)
(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 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+
https://hg.mozilla.org/integration/fx-team/rev/de07ed955b6e51bda53802a5440648e0e7b2cc62
Bug 1251340 - Add probes for entering / exiting bookmark folders r=margaret
https://hg.mozilla.org/mozilla-central/rev/de07ed955b6e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.