Closed Bug 977196 Opened 10 years ago Closed 10 years ago

UI Telemetry for opening urls

Categories

(Firefox for Android Graveyard :: General, defect, P1)

ARM
Android
defect

Tracking

(firefox31 verified, firefox32 verified)

VERIFIED FIXED
Firefox 32
Tracking Status
firefox31 --- verified
firefox32 --- verified

People

(Reporter: liuche, Assigned: oogunsakin)

References

Details

Attachments

(6 files, 25 obsolete files)

3.56 KB, patch
Details | Diff | Splinter Review
12.90 KB, patch
liuche
: review+
Details | Diff | Splinter Review
4.82 KB, patch
liuche
: review+
Details | Diff | Splinter Review
7.85 KB, patch
liuche
: review+
Details | Diff | Splinter Review
3.97 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
1.71 KB, patch
liuche
: review+
Details | Diff | Splinter Review
We'd like to know how users open new urls:
* Externally, through an intent
* From a context menu (e.g., Open in new tab), including the location of context menu (about:home page, link from another page)
* "+" from the dropdown menu and entering a url
* "+" from the tabs tray and entering a url
* Typing into the urlbar of an existing tab

This might inform if we want to make opening a new tab more easily accessible; if we want to emphasize/streamline different entry points
We might also want to consider opening a link from a search shortcut, though that might be a different class of question, or a subset of "Typing into the urlbar *"
Attached patch Loading URL from Intent (obsolete) — Splinter Review
Attachment #8383875 - Flags: feedback?(liuche)
Attached patch WIP: Home Panels Session (obsolete) — Splinter Review
Attachment #8383912 - Flags: feedback?(liuche)
Attached patch WIP: Home Panels Session (obsolete) — Splinter Review
Attachment #8383912 - Attachment is obsolete: true
Attachment #8383912 - Flags: feedback?(liuche)
Attachment #8383915 - Flags: feedback?(liuche)
Comment on attachment 8383875 [details] [diff] [review]
Loading URL from Intent

       mBrowserToolbar = (BrowserToolbar) findViewById(R.id.browser_toolbar);
>         if (Intent.ACTION_VIEW.equals(intent.getAction())) {
>+            Telemetry.sendUIEvent("app:loadUrlFromIntent", "BrowserApp.onCreate");
>             // Show the target URL immediately in the toolbar
>             mBrowserToolbar.setTitle(intent.getDataString());

I think we are putting too much in the event name and the event method. The event method is not supposed to be a copy of the Java method from where the event is created. It's supposed to be a conceptual subtype to the event name. In this case I think we could get by with:

Telemetry.sendUIEvent("loadurl", "intent");

other potential uses are:
Telemetry.sendUIEvent("loadurl", "typed");
Telemetry.sendUIEvent("loadurl", "thumbnail");
Telemetry.sendUIEvent("loadurl", "list");
Telemetry.sendUIEvent("loadurl", "menu");


When using these in conjunction with telemetry sessions, we can get a lot of data. Sessions could include:
"startup" - during startup
"home" - while the home screen is up
  "topsites" - a sub-session for when the Top Sites page is showing
  "bookmarks"
  "history"
  "readinglist"
"tabtray" - while the tab tray is up
"mainmenu" - while the main menu is showing
"contextmenu" - while a context menu is showing

We can have more than one session active at a time. It will allow us to post-analyze the data to dig for "deeper meaning" while still getting high level data easier. As you can see, I am also not a fan of camel casing tags/names.
Comment on attachment 8383915 [details] [diff] [review]
WIP: Home Panels Session

>         // Refresh toolbar height to possibly restore the toolbar padding
>         refreshToolbarHeight();
>+        Telemetry.stopUISession(Telemetry.Session.HOME_PANELS,
>+                                "Navigation to about:home URL");

I think we might want to use tags for the "reason" code too. Using free-form sentences will make analysis harder.
Lastly, these ideas are how I have been thinking about UI telemetry for a while, but I am interested in hearing other ideas too.
Comment on attachment 8383875 [details] [diff] [review]
Loading URL from Intent

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

I like the action/methods mfinkle suggested; we should try to make our tags general but also provide useful information.

Nit: tabstray instead of tabtray.
Attachment #8383875 - Flags: feedback?(liuche)
Comment on attachment 8383915 [details] [diff] [review]
WIP: Home Panels Session

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

I like the idea of having Event and Session names documented, so that there's a place that we can see all the various events. However, I have second thoughts about keeping them all in code, and they should really be on a wiki page. Maybe a section on Telemetry on https://wiki.mozilla.org/Mobile/Fennec/Android that links to another "Mobile Telemetry" page? If we do decide to use a wiki page for documentation, it should be referenced comments, and updating the page with new probes should be required for r+ (since the danger of wikis is so often things getting out of sync).

+1 on using tags instead of sentences - think about how one would need to filter telemetry results.
Attachment #8383915 - Flags: feedback?(liuche)
(In reply to Mark Finkle (:mfinkle) from comment #5)
> I think we are putting too much in the event name and the event method. The
> event method is not supposed to be a copy of the Java method from where the
> event is created. It's supposed to be a conceptual subtype to the event
> name. In this case I think we could get by with:
> 
ah I see. Thanks!
(In reply to Chenxia Liu [:liuche] from comment #9)
> Comment on attachment 8383915 [details] [diff] [review]
> WIP: Home Panels Session
> 
> Review of attachment 8383915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the idea of having Event and Session names documented, so that
> there's a place that we can see all the various events. However, I have
> second thoughts about keeping them all in code, and they should really be on
> a wiki page. Maybe a section on Telemetry on
> https://wiki.mozilla.org/Mobile/Fennec/Android that links to another "Mobile
> Telemetry" page? If we do decide to use a wiki page for documentation, it
> should be referenced comments, and updating the page with new probes should
> be required for r+ (since the danger of wikis is so often things getting out
> of sync).
> 
> +1 on using tags instead of sentences - think about how one would need to
> filter telemetry results.

sounds good. Thanks!
Assignee: nobody → oogunsakin
No longer depends on: mobile-ui-telemetry
Updating the list from comment 1 to be more specific about how URLs are navigated to:
* Typing an entire url
* Typing a little and then:
  * Autocompleted URL
  * Awesomebar recommendation (History or Bookmark)
  * Search suggestion by search engine
  * Search shortcut (including provider)
* Home panels
  * Top Sites - thumbnail or list, and position
  * (other panels)
  * Context menu
* External app
Priority: -- → P1
Attachment #8383875 - Attachment is obsolete: true
Attachment #8400410 - Flags: feedback?(liuche)
Attachment #8383915 - Attachment is obsolete: true
Attachment #8400411 - Flags: feedback?(liuche)
Attachment #8400414 - Flags: feedback?(liuche)
Attachment #8400424 - Flags: feedback?(liuche)
Comment on attachment 8400410 [details] [diff] [review]
Part 1: Loading URL from an intent

Seems good to me. "loadurl.1" and "intent.1" fit my mental model well.
Attachment #8400410 - Flags: feedback+
Comment on attachment 8400411 [details] [diff] [review]
Part 2: Loading URLs in about:home

Thoughts:
1. "newtab.1" and "privatetab.1" don't fit my mental model. That seems like Session information. Looking at where I think you are using those, I think we should be using "contextmenu.1" and the Session will be "topsites" or whatever. At best, I'd pass the "private" as extra (nothing means "normal").
2. You add "listitem.1" but don't use it for several of the lists, like history and reading list.
Comment on attachment 8400414 [details] [diff] [review]
Part 3: Loading URLs by opening a new tab

Feedback:
1. I don't think we should add a "loadurl" to TabsTray. We are not loading a URL, only adding a Tab. Not the same.
2. If we did need to track in TabsTray, we would likely not use "tabstray.1" as a Method, since we would already have a TabsTray Session.

I don't think this patch is needed.
Comment on attachment 8400424 [details] [diff] [review]
Part 4: Loading URLs from the url bar

Feedback:
1. I think we want a URL editing Session. Maybe start it in enterEditingMode(url) and end it in commitEditingMode (and any other ways to close the editing mode). 
2. I think the Method we want is "userentered.1" or "typed.1" (I lean to "typed.1" for brevity).
(In reply to Chenxia Liu [:liuche] from comment #12)
> Updating the list from comment 1 to be more specific about how URLs are
> navigated to:
> * Typing an entire url
> * Typing a little and then:
>   * Autocompleted URL
>   * Awesomebar recommendation (History or Bookmark)
>   * Search suggestion by search engine
>   * Search shortcut (including provider)
> * Home panels
>   * Top Sites - thumbnail or list, and position
>   * (other panels)
>   * Context menu
> * External app

As always, I suggest keeping it simple. Using Sessions to build context. Make sure we really need to analyze all the things. We can always add more later. Get the basics done first.
(In reply to Mark Finkle (:mfinkle) from comment #18)
> 1. "newtab.1" and "privatetab.1" don't fit my mental model. That seems like
> Session information. Looking at where I think you are using those, I think
> we should be using "contextmenu.1" and the Session will be "topsites" or
> whatever. At best, I'd pass the "private" as extra (nothing means "normal").

Currently there isn't a very good way of detecting from a fragment, when a contextmenu is closed. We would need that to have a contextmenu session. Also I'm not sure contextmenu session would tell us anything new; the actions/events in that session cannot be performed any other way (at least not in about:home). 

Method "newtab.1" could maybe be interpreted as "user opened a new that via a new tab". Same with "privatetab.1".

> 2. You add "listitem.1" but don't use it for several of the lists, like
> history and reading list.
 I left them out because they from the Bookmarks, ReadingList and History panels because it doesn't tell us anything new. The only type of item in those panels is a list.
(In reply to Sola Ogunsakin [:sola] from comment #22)

> Currently there isn't a very good way of detecting from a fragment, when a
> contextmenu is closed. We would need that to have a contextmenu session.

That seems like a solvable problem, no?

Why not have the context menu itself create and finish its own sessions? That seems preferable regardless.

> Also I'm not sure contextmenu session would tell us anything new; the
> actions/events in that session cannot be performed any other way (at least
> not in about:home). 

It would tell us how many users use context menus, for one thing, how long they spend trying to use them, and whether they're successful.
(In reply to Sola Ogunsakin [:sola] from comment #22)
> (In reply to Mark Finkle (:mfinkle) from comment #18)
> > 1. "newtab.1" and "privatetab.1" don't fit my mental model. That seems like
> > Session information. Looking at where I think you are using those, I think
> > we should be using "contextmenu.1" and the Session will be "topsites" or
> > whatever. At best, I'd pass the "private" as extra (nothing means "normal").
> 
> Currently there isn't a very good way of detecting from a fragment, when a
> contextmenu is closed. We would need that to have a contextmenu session.
> Also I'm not sure contextmenu session would tell us anything new; the
> actions/events in that session cannot be performed any other way (at least
> not in about:home). 
> 
> Method "newtab.1" could maybe be interpreted as "user opened a new that via
> a new tab". Same with "privatetab.1".

I think I see what you are saying, but I still think the Method for both of these should be "contextmenu.1" and someday we will add Sessions (?) or Events (?) for "normal" and "private" tabs being created.

We don't need to worry about a contextmenu Session yet.

This bug is about tracking loadurls. From about:home we can loadurls from contextmenus, grids and lists.


> 
> > 2. You add "listitem.1" but don't use it for several of the lists, like
> > history and reading list.
>  I left them out because they from the Bookmarks, ReadingList and History
> panels because it doesn't tell us anything new. The only type of item in
> those panels is a list.

I might be able to buy this rationale.
(In reply to Mark Finkle (:mfinkle) from comment #24)
> I think I see what you are saying, but I still think the Method for both of
> these should be "contextmenu.1" and someday we will add Sessions (?) or
> Events (?) for "normal" and "private" tabs being created.
> 
> We don't need to worry about a contextmenu Session yet.
> 
Sounds good. Will update the patch.
-replace "privatetab.1" and "newtab.1" methods with "contextmenu.1"
Attachment #8400411 - Attachment is obsolete: true
Attachment #8400411 - Flags: feedback?(liuche)
Attachment #8400886 - Flags: feedback?(liuche)
Attachment #8400414 - Attachment is obsolete: true
Attachment #8400414 - Flags: feedback?(liuche)
-create a session for url bar
Attachment #8400424 - Attachment is obsolete: true
Attachment #8400424 - Flags: feedback?(liuche)
Attachment #8400888 - Flags: feedback?(liuche)
Attachment #8400410 - Flags: feedback?(liuche) → review?(liuche)
Comment on attachment 8400888 [details] [diff] [review]
Part 3: Loading URLs from the url bar

>         // If the URL doesn't look like a search query, just load it.
>         if (!StringUtils.isSearchQuery(url, true)) {
>             Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);
>+
>+            // Record UI Telemetry event for url load.
>+            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);

Does this handle URLs tapped in the awesomebarlist (BrowserSearch) ?
(In reply to Mark Finkle (:mfinkle) from comment #28)
> Does this handle URLs tapped in the awesomebarlist (BrowserSearch) ?

No, I'll upload a patch for that
Attachment #8400888 - Attachment is obsolete: true
Attachment #8400888 - Flags: feedback?(liuche)
Attachment #8401555 - Flags: feedback?(liuche)
probes for loading urls from browser search. 

Overview:
session is "browsersearch.1", the events are:
  - History list items: "loadurl.1" method="listitem.1"
  - Search suggestions: "loadurl.1" method="searchsuggestion.1" extras=<Search Engine>
Attachment #8401559 - Flags: feedback?(liuche)
Comment on attachment 8401559 [details] [diff] [review]
Part 4: Loading URL from browser search

Since we already have FHR for search engine info, I wonder if we could drop the extra: <search engine> part. What analysis are we planning using this extra part?
Comment on attachment 8400410 [details] [diff] [review]
Part 1: Loading URL from an intent

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

::: mobile/android/base/BrowserApp.java
@@ +458,5 @@
>          mBrowserToolbar.setProgressBar(mProgressView);
>          if (Intent.ACTION_VIEW.equals(intent.getAction())) {
>              // Show the target URL immediately in the toolbar.
>              mBrowserToolbar.setTitle(intent.getDataString());
> +            // Post UI Telemetry event.

Don't need this comment. Same reason as before, for a single line Telemetry event, don't bother adding a comment, because the method call and arguments are pretty clear.

::: mobile/android/base/TelemetryContract.java
@@ +14,5 @@
>       * Holds event names. Intended for use with
>       * Telemetry.sendUIEvent() as the "action" parameter.
>       */
> +    public interface Event {
> +        // Fired when a user loads a URL from outside the geckoview.

I'm feeling hesitant about this prefixing everything with "Fired when" - can we just do "URL loaded..."?

Also, I don't think this is the right comment for the event - we'll want to reuse this event, right? It's not just for loading a url from outside of Firefox, it should be for loading any url, and the session or method will give the appropriate context.
Attachment #8400410 - Flags: review?(liuche)
-edit comments
Attachment #8400410 - Attachment is obsolete: true
Attachment #8402068 - Flags: review?(liuche)
-remove telemetry comments
Attachment #8400886 - Attachment is obsolete: true
Attachment #8400886 - Flags: feedback?(liuche)
Attachment #8402075 - Flags: review?(liuche)
Attached patch bug-977196-urlbar.patch.patch (obsolete) — Splinter Review
-remove telemetry comments
Attachment #8401555 - Attachment is obsolete: true
Attachment #8402078 - Attachment is obsolete: true
Attachment #8401555 - Flags: feedback?(liuche)
Attachment #8402079 - Flags: review?(liuche)
Attachment #8401559 - Attachment is obsolete: true
Attachment #8401559 - Flags: feedback?(liuche)
Attachment #8402084 - Flags: review?(liuche)
(In reply to Sola Ogunsakin [:sola] from comment #38)
> Created attachment 8402084 [details] [diff] [review]
> Part 4: Loading URL from browser search

-removed telemetry comments
Comment on attachment 8402068 [details] [diff] [review]
Part 1: Loading URL from an intent

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

::: mobile/android/base/BrowserApp.java
@@ +459,5 @@
>          if (Intent.ACTION_VIEW.equals(intent.getAction())) {
>              // Show the target URL immediately in the toolbar.
>              mBrowserToolbar.setTitle(intent.getDataString());
> +
> +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.INTENT);

Did you test this? :( It doesn't work for me. I don't think that we only want to put this in onCreate - what if you have FF in the background and open a link from another app? Look at onNewIntent, but pay attention to the other places within this app where we start an intent with ACTION_VIEW, and make sure that we are not counting those.
Attachment #8402068 - Flags: review?(liuche) → review-
(In reply to Chenxia Liu [:liuche] from comment #40)
> Did you test this? :( It doesn't work for me. I don't think that we only
> want to put this in onCreate - what if you have FF in the background and
> open a link from another app? Look at onNewIntent, but pay attention to the
> other places within this app where we start an intent with ACTION_VIEW, and
> make sure that we are not counting those.

Works for me but I guess that's with fennec closed. I'll add a probe to onNewIntent.
Comment on attachment 8402075 [details] [diff] [review]
Part 2: Loading URLs in about:home

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

For opening a url not from a context menu, notice that that's always after a call to onUrlOpen, so take a look at where that goes. I think it would work well with with what you were doing in Patch 1 because onUrlOpen also calls into BrowserApp.onNewIntent, which was the case you weren't handling in your first patch. Feel free to combine these Patch 1 and 2 if it makes it easier.

::: mobile/android/base/home/LastTabsPanel.java
@@ +114,5 @@
>  
>                  final String url = c.getString(c.getColumnIndexOrThrow(Combined.URL));
>                  mNewTabsListener.onNewTabs(new String[] { url });
> +
> +                Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);

Is there a session for Last Tabs vs Recent Tabs (aka History)? We should add one.
Attachment #8402075 - Flags: review?(liuche) → feedback+
(In reply to Chenxia Liu [:liuche] from comment #42)
> For opening a url not from a context menu, notice that that's always after a
> call to onUrlOpen, so take a look at where that goes. I think it would work
> well with with what you were doing in Patch 1 because onUrlOpen also calls
> into BrowserApp.onNewIntent, which was the case you weren't handling in your
> first patch. Feel free to combine these Patch 1 and 2 if it makes it easier.

For all the cases in this patch onUrlOpen is called without the OPEN_WITH_INTENT flag which causes it to go through BrowserApp.openUrlAndStopEditing() instead of BrowserApp.onNewIntent().
(In reply to Sola Ogunsakin [:sola] from comment #43)
> (In reply to Chenxia Liu [:liuche] from comment #42)
> > For opening a url not from a context menu, notice that that's always after a
> > call to onUrlOpen, so take a look at where that goes. I think it would work
> > well with with what you were doing in Patch 1 because onUrlOpen also calls
> > into BrowserApp.onNewIntent, which was the case you weren't handling in your
> > first patch. Feel free to combine these Patch 1 and 2 if it makes it easier.
> 
> For all the cases in this patch onUrlOpen is called without the
> OPEN_WITH_INTENT flag which causes it to go through
> BrowserApp.openUrlAndStopEditing() instead of BrowserApp.onNewIntent().

My main concern here is that while this technically works and is correct, it's best if we have the telemetry code somewhere central, because then it's less likely to bitrot when someone adds something else.

Keep in mind that after correctness, code maintainability is (almost certainly) the next most-important thing. Adding probes now is good, but also try to look forward, and realize that if we add the probe in a shared method, it's less likely to get accidentally omitted in the future if/when we add new code.
(In reply to Chenxia Liu [:liuche] from comment #42)
> Is there a session for Last Tabs vs Recent Tabs (aka History)? We should add
> one.

ViewPager makes this messy. We can't rely on the usual onStart() and onStop() methods in the LastTabsPanel and MostRecentPanel fragments. ViewPager starts the fragments and makes them visible even when they are technically out of view. Any ideas? Or else we could put this info in the method of the event.
(In reply to Chenxia Liu [:liuche] from comment #44)
> (In reply to Sola Ogunsakin [:sola] from comment #43)
> > (In reply to Chenxia Liu [:liuche] from comment #42)
> > > For opening a url not from a context menu, notice that that's always after a
> > > call to onUrlOpen, so take a look at where that goes. I think it would work
> > > well with with what you were doing in Patch 1 because onUrlOpen also calls
> > > into BrowserApp.onNewIntent, which was the case you weren't handling in your
> > > first patch. Feel free to combine these Patch 1 and 2 if it makes it easier.
> > 
> > For all the cases in this patch onUrlOpen is called without the
> > OPEN_WITH_INTENT flag which causes it to go through
> > BrowserApp.openUrlAndStopEditing() instead of BrowserApp.onNewIntent().
> 
> My main concern here is that while this technically works and is correct,
> it's best if we have the telemetry code somewhere central, because then it's
> less likely to bitrot when someone adds something else.
> 
> Keep in mind that after correctness, code maintainability is (almost
> certainly) the next most-important thing. Adding probes now is good, but
> also try to look forward, and realize that if we add the probe in a shared
> method, it's less likely to get accidentally omitted in the future if/when
> we add new code.

I'll move the probes to BrowserApp#onUrlOpen.
(In reply to Sola Ogunsakin [:sola] from comment #46)
> (In reply to Chenxia Liu [:liuche] from comment #44)
> > (In reply to Sola Ogunsakin [:sola] from comment #43)
> > > (In reply to Chenxia Liu [:liuche] from comment #42)
> > > > For opening a url not from a context menu, notice that that's always after a
> > > > call to onUrlOpen, so take a look at where that goes. I think it would work
> > > > well with with what you were doing in Patch 1 because onUrlOpen also calls
> > > > into BrowserApp.onNewIntent, which was the case you weren't handling in your
> > > > first patch. Feel free to combine these Patch 1 and 2 if it makes it easier.
> > > 
> > > For all the cases in this patch onUrlOpen is called without the
> > > OPEN_WITH_INTENT flag which causes it to go through
> > > BrowserApp.openUrlAndStopEditing() instead of BrowserApp.onNewIntent().
> > 
> > My main concern here is that while this technically works and is correct,
> > it's best if we have the telemetry code somewhere central, because then it's
> > less likely to bitrot when someone adds something else.
> > 
> > Keep in mind that after correctness, code maintainability is (almost
> > certainly) the next most-important thing. Adding probes now is good, but
> > also try to look forward, and realize that if we add the probe in a shared
> > method, it's less likely to get accidentally omitted in the future if/when
> > we add new code.
> 
> I'll move the probes to BrowserApp#onUrlOpen.

Actually on second look, if I centralize the loadurl telemetry logic, we will loose info about the context that would have gone in the event's method. For e.g in top sites, the loadurl events for the grid view and list view use the method field.
Comment on attachment 8402079 [details] [diff] [review]
Part 3: Loading URLs from the url bar

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

Almost there!

::: mobile/android/base/BrowserApp.java
@@ +502,5 @@
>          mBrowserToolbar.setOnCommitListener(new BrowserToolbar.OnCommitListener() {
>              public void onCommit() {
>                  commitEditingMode();
> +
> +                Telemetry.stopUISession(TelemetryContract.Session.URL_BAR);

This is unnecessary.

@@ +524,5 @@
>              public void onFocusChange(View v, boolean hasFocus) {
>                  if (isHomePagerVisible()) {
>                      mHomePager.onToolbarFocusChange(hasFocus);
> +
> +                    if (hasFocus) {

This shouldn't be inside the isHomePagerVisible check.

::: mobile/android/base/TelemetryContract.java
@@ +45,5 @@
>          // Started when a user enters a given home panel.
>          // Session name is dynamic, encoded as "homepanel.1:<panel_id>"
>          public static final String HOME_PANEL = "homepanel.1:";
> +
> +        // Started when a user taps the URL bar.

How about "URL bar focused" and URLBAR_FOCUSED for the variable name? I think this is more useful, because it describes the actual session.
Attachment #8402079 - Flags: review?(liuche) → review-
(In reply to Chenxia Liu [:liuche] from comment #48)
> @@ +502,5 @@
> >          mBrowserToolbar.setOnCommitListener(new BrowserToolbar.OnCommitListener() {
> >              public void onCommit() {
> >                  commitEditingMode();
> > +
> > +                Telemetry.stopUISession(TelemetryContract.Session.URL_BAR);
> 
> This is unnecessary.
> 
It's used for an edge case. The OnFocusChangeListener doesn't get called when the user hits "enter" with browser search open.
> @@ +524,5 @@
> >              public void onFocusChange(View v, boolean hasFocus) {
> >                  if (isHomePagerVisible()) {
> >                      mHomePager.onToolbarFocusChange(hasFocus);
> > +
> > +                    if (hasFocus) {
> 
> This shouldn't be inside the isHomePagerVisible check.
Good catch! Thanks, I'll move that.
-add probe to onNewIntent
Attachment #8402068 - Attachment is obsolete: true
Attachment #8403630 - Flags: review?(liuche)
Attachment #8402075 - Attachment is obsolete: true
Attachment #8403631 - Flags: review?(liuche)
-rename URLBAR
-fix issue in onFocusChange
Attachment #8402079 - Attachment is obsolete: true
Attachment #8403635 - Flags: review?(liuche)
-edit comments
Attachment #8402084 - Attachment is obsolete: true
Attachment #8402084 - Flags: review?(liuche)
Attachment #8403636 - Flags: review?(liuche)
(In reply to Sola Ogunsakin [:sola] from comment #49)
> (In reply to Chenxia Liu [:liuche] from comment #48)
> > @@ +502,5 @@
> > >          mBrowserToolbar.setOnCommitListener(new BrowserToolbar.OnCommitListener() {
> > >              public void onCommit() {
> > >                  commitEditingMode();
> > > +
> > > +                Telemetry.stopUISession(TelemetryContract.Session.URL_BAR);
> > 
> > This is unnecessary.
> > 
> It's used for an edge case. The OnFocusChangeListener doesn't get called
> when the user hits "enter" with browser search open.

Hm, why do you think this is an edge case?
(In reply to Chenxia Liu [:liuche] from comment #54)
> (In reply to Sola Ogunsakin [:sola] from comment #49)
> > (In reply to Chenxia Liu [:liuche] from comment #48)
> > > @@ +502,5 @@
> > > >          mBrowserToolbar.setOnCommitListener(new BrowserToolbar.OnCommitListener() {
> > > >              public void onCommit() {
> > > >                  commitEditingMode();
> > > > +
> > > > +                Telemetry.stopUISession(TelemetryContract.Session.URL_BAR);
> > > 
> > > This is unnecessary.
> > > 
> > It's used for an edge case. The OnFocusChangeListener doesn't get called
> > when the user hits "enter" with browser search open.
> 
> Hm, why do you think this is an edge case?

Perhaps I misspoke, all I meant was that its there to handle a special case. As for why the OnFocusChangeListener isn't called in that case, I digged through the code but I'm not sure.
Attachment #8403630 - Flags: review?(liuche) → review+
-close urlbar session when user minimizes/closes app
Attachment #8403635 - Attachment is obsolete: true
Attachment #8403635 - Flags: review?(liuche)
Attachment #8404264 - Flags: review?(liuche)
-close browsersearch session when user minimizes/closes the app
Attachment #8403636 - Attachment is obsolete: true
Attachment #8403636 - Flags: review?(liuche)
Attachment #8404266 - Flags: review?(liuche)
Comment on attachment 8403631 [details] [diff] [review]
Part 2: Loading URLs in about:home

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

> > 
> > I'll move the probes to BrowserApp#onUrlOpen.
> 
> Actually on second look, if I centralize the loadurl telemetry logic, we
> will loose info about the context that would have gone in the event's
> method. For e.g in top sites, the loadurl events for the grid view and list
> view use the method field.

Mmm that's a good point - I guess it's okay to have a probe for each page, because it doesn't seem like we can differentiate whether a Top Sites thumbnail or list item was called.

> > Is there a session for Last Tabs vs Recent Tabs (aka History)? We should add
> > one.
> 
> ViewPager makes this messy. We can't rely on the usual onStart() and
> onStop() methods in the LastTabsPanel and MostRecentPanel fragments.
> ViewPager starts the fragments and makes them visible even when they are
> technically out of view. Any ideas? Or else we could put this info in the
> method of the event.

I'm sure you can figure something out :) File a bug for it, and check to see what Android Fragment/TransactionManager APIs there are - I'd be very, very surprised if there wasn't a way to tell which Fragment was being shown.

As for putting it in the method - that kind of defeats the purpose of sessions, so I don't think we should do that.

::: mobile/android/base/BrowserApp.java
@@ +2595,5 @@
>              Intent intent = new Intent(Intent.ACTION_VIEW);
>              intent.setData(Uri.parse(url));
>              startActivity(intent);
>          } else if (!maybeSwitchToTab(url, flags)) {
> +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.GRID_ITEM);

Why grid item?

::: mobile/android/base/home/BookmarksListView.java
@@ +92,5 @@
>          } else {
>              // Otherwise, just open the URL
>              final String url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
>  
> +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);

I think these would make more sense right after the onUrlOpen call - what do you think?

::: mobile/android/base/home/HomeFragment.java
@@ +156,5 @@
>              final String url = (info.isInReadingList() ? ReaderModeUtils.getAboutReaderForUrl(info.url) : info.url);
>              Tabs.getInstance().loadUrl(url, flags);
>              Toast.makeText(context, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
> +
> +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.CONTEXT_MENU);

Do you think we should differentiate between private/normal tab?
Attachment #8403631 - Flags: review?(liuche) → feedback+
Comment on attachment 8404264 [details] [diff] [review]
Part 3: Loading URLs from the url bar

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

A little more work needed.

::: mobile/android/base/BrowserApp.java
@@ +502,5 @@
>          mBrowserToolbar.setOnCommitListener(new BrowserToolbar.OnCommitListener() {
>              public void onCommit() {
>                  commitEditingMode();
> +
> +                Telemetry.stopUISession(TelemetryContract.Session.URLBAR_FOCUSED);

Did you find out why this is necessary? You mentioned you didn't find any code paths that made this a special case.

@@ +640,5 @@
>      public void onResume() {
>          super.onResume();
>          unregisterEventListener("Prompt:ShowTop");
> +
> +        if (mBrowserToolbar.isEditing()) {

This approach doesn't feel right to me - why do we want to explicitly handle starting/pausing every session that we add? Can you think of a better way to do this?
Attachment #8404264 - Flags: review?(liuche) → review-
Comment on attachment 8404266 [details] [diff] [review]
Part 4: Loading URL from browser search

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

I don't really like the session name Browser Search - it seems pretty vague, and could be any number of things. I think Search Suggestions might be better for the session name, and Search Shortcut might be a better event name. What do you think?

What cases did you consider for this? I noticed there's also a onUrlOpen call in SearchEngineRow - is that something we should pay attention to?

In general, for adding telemetry probes, you should do a bunch of manual testing to make sure probes are being fired when you expect them to be - try edge cases, try to find all the places you might be able to access this feature as a user; also make sure you do method searches through the code base so you don't miss anything, because if we forget to add a probe somewhere, our telemetry data is going to be bad.

::: mobile/android/base/BrowserApp.java
@@ +2627,5 @@
>          openUrlAndStopEditing(text, engine.name);
> +
> +        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL,
> +                              TelemetryContract.Method.SEARCH_SUGGESTION,
> +                              engine.name);

I'd check with mfinkle if we want to track the engine name. I think this is on the edge of the line of UI and Telemetry - from a UI perspective, it doesn't really matter what search engine was used. On the other hand, it might be interesting to know how many different engines a user uses.

::: mobile/android/base/TelemetryContract.java
@@ +49,5 @@
>  
>          // Url bar has focused.
>          public static final String URLBAR_FOCUSED = "urlbar.1:";
> +
> +        // Browser search appears.

Is this a useful description of the session? It only describes when it starts, but doesn't say anything about the session itself.

::: mobile/android/base/home/BrowserSearch.java
@@ +221,5 @@
>      @Override
> +    public void onResume() {
> +        super.onResume();
> +
> +        Telemetry.startUISession(TelemetryContract.Session.BROWSER_SEARCH);

Again, I'm not so sure we want to do this, because of scalability, and code maintenance. See comment on previous patch.

@@ +275,5 @@
>                  position -= getSuggestEngineCount();
>                  final Cursor c = mAdapter.getCursor(position);
>                  final String url = c.getString(c.getColumnIndexOrThrow(URLColumns.URL));
>  
> +                Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);

Do we need to know specifically that this is a list item?
Attachment #8404266 - Flags: review?(liuche) → feedback+
(In reply to Chenxia Liu [:liuche] from comment #60)

> feature as a user; also make sure you do method searches through the code
> base so you don't miss anything, because if we forget to add a probe
> somewhere, our telemetry data is going to be bad.

N.B., Eclipse allows you to do this quite trivially. It's invaluable.


> I'd check with mfinkle if we want to track the engine name.

We report engine names and counts as part of the FHR payload for all channels, including release. I wouldn't expect telemetry data to be any less detailed.
(In reply to Richard Newman [:rnewman] from comment #61)

> > I'd check with mfinkle if we want to track the engine name.
> 
> We report engine names and counts as part of the FHR payload for all
> channels, including release. I wouldn't expect telemetry data to be any less
> detailed.

Except Telemetry and FHR are held to different standards when dealing with privacy. Does sending the engine name infer a different level of privacy? I don't know, but if we already get engine name counts from FHR, we need to think of a reason to duplicate the information in Telemetry. What's the value?
(In reply to Mark Finkle (:mfinkle) from comment #62)

> Except Telemetry and FHR are held to different standards when dealing with
> privacy. Does sending the engine name infer a different level of privacy?

That was my point -- FHR's standards are much higher than telemetry, so if it's acceptable for FHR, it should be so for telemetry. That's true unless the combination of the telemetry events is unnecessarily fingerprintable.

Which leaves the real question...


> I don't know, but if we already get engine name counts from FHR, we need to
> think of a reason to duplicate the information in Telemetry. What's the
> value?

I could make a case for it, for sure: UI telemetry would provide more context to interpret user actions. The FHR data is pretty much "the user searched Google three times today from the URL bar", whereas UI telemetry is "after a minute of scrubbing through bookmarks, the user gave up and searched Google".

Furthermore, unless we have a good short-term story for correlating FHR data with UI telemetry data, we won't be able to do any analysis that crosses the two -- e.g., do users who use context menus have different search behavior?

That's a question we need to address more broadly, though, and I'm sure there are folks elsewhere at Mozilla thinking about this problem.
(In reply to Richard Newman [:rnewman] from comment #63)

> > I don't know, but if we already get engine name counts from FHR, we need to
> > think of a reason to duplicate the information in Telemetry. What's the
> > value?
> 
> I could make a case for it, for sure: UI telemetry would provide more
> context to interpret user actions. The FHR data is pretty much "the user
> searched Google three times today from the URL bar", whereas UI telemetry is
> "after a minute of scrubbing through bookmarks, the user gave up and
> searched Google".

I lean to the "don't send it unless we have a specific need" side. I also worry about payload size and search engine names that might be crazy like "Finkle's super search for Beanie Babies".
Comment on attachment 8403630 [details] [diff] [review]
Part 1: Loading URL from an intent

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

::: mobile/android/base/TelemetryContract.java
@@ +24,5 @@
>       * Telemetry.sendUIEvent() as the "method" parameter.
>       */
> +    public interface Method {
> +        // Event occurred via an intent.
> +        public static final String INTENT = "intent.1";

I realized while reviewing another patch that we don't need to version methods, because they'll be tied to an Event, which will carry the versioning. Also, since methods can be shared, versioning the methods could possibly lead to fracturing and complexity server-side. What do you think?
(In reply to Chenxia Liu [:liuche] from comment #65)

> I realized while reviewing another patch that we don't need to version
> methods, because they'll be tied to an Event, which will carry the
> versioning. Also, since methods can be shared, versioning the methods could
> possibly lead to fracturing and complexity server-side. What do you think?

If we already have landed versioned methods, let's just keep rolling. Unless we feel the need to rollback. It is still early days.

But after we merge to Aurora, let's be consistent.
No, we don't actually any landed methods or reasons with versioning, so we can decide when either this bug lands, or bug 998000 does.
So long as you always remember to bump versions on events, and never to do analysis only on methods without scoping to an event, rip it out now!

(Add notes to this effect to the Sphinx docs, please. Write the handbook as we go.)
Blocks: 999483
No longer blocks: 999483
Blocks: 997768
-remove versioning in method
Attachment #8403630 - Attachment is obsolete: true
(In reply to Chenxia Liu [:liuche] from comment #58)
> ::: mobile/android/base/BrowserApp.java
> @@ +2595,5 @@
> >              Intent intent = new Intent(Intent.ACTION_VIEW);
> >              intent.setData(Uri.parse(url));
> >              startActivity(intent);
> >          } else if (!maybeSwitchToTab(url, flags)) {
> > +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.GRID_ITEM);
> 
> Why grid item?

not sure why this is there :/ taking it out

> ::: mobile/android/base/home/BookmarksListView.java
> @@ +92,5 @@
> >          } else {
> >              // Otherwise, just open the URL
> >              final String url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
> >  
> > +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);
> 
> I think these would make more sense right after the onUrlOpen call - what do
> you think?

if the page loads really fast, onUrlOpen can cause the homepanel session to close before the event is recorded.

> ::: mobile/android/base/home/HomeFragment.java
> @@ +156,5 @@
> >              final String url = (info.isInReadingList() ? ReaderModeUtils.getAboutReaderForUrl(info.url) : info.url);
> >              Tabs.getInstance().loadUrl(url, flags);
> >              Toast.makeText(context, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
> > +
> > +            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.CONTEXT_MENU);
> 
> Do you think we should differentiate between private/normal tab?

haven't heard anyone bring it up as a question we'd want to answer.
-remove unneeded probe
-remove versioning in methods
Attachment #8403631 - Attachment is obsolete: true
Attachment #8411411 - Flags: review?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #59)
> ::: mobile/android/base/BrowserApp.java
> @@ +502,5 @@
> >          mBrowserToolbar.setOnCommitListener(new BrowserToolbar.OnCommitListener() {
> >              public void onCommit() {
> >                  commitEditingMode();
> > +
> > +                Telemetry.stopUISession(TelemetryContract.Session.URLBAR_FOCUSED);
> 
> Did you find out why this is necessary? You mentioned you didn't find any
> code paths that made this a special case.

yeah its been fixed locally. i'll upload the latest patch

> @@ +640,5 @@
> >      public void onResume() {
> >          super.onResume();
> >          unregisterEventListener("Prompt:ShowTop");
> > +
> > +        if (mBrowserToolbar.isEditing()) {
> 
> This approach doesn't feel right to me - why do we want to explicitly handle
> starting/pausing every session that we add? Can you think of a better way to
> do this?

bug 994273 resolves this. urlbar would be an activity-bound session, that way it doesn't have to explicitly handle BrowserApp pausing and restarting.
-remove unnecessary probe
Attachment #8404264 - Attachment is obsolete: true
Attachment #8411412 - Flags: review?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #60)
> ::: mobile/android/base/home/BrowserSearch.java
> @@ +221,5 @@
> >      @Override
> > +    public void onResume() {
> > +        super.onResume();
> > +
> > +        Telemetry.startUISession(TelemetryContract.Session.BROWSER_SEARCH);
> 
> Again, I'm not so sure we want to do this, because of scalability, and code
> maintenance. See comment on previous patch.

This call is needed for starting a browser search session. I think your objection was to making calls like this one in BrowserApp#onResume/onPause but this is BrowserSearch#onResume.
-rename browsersearch session
-remove version from methods
-remove search engine from loadurl extras
Attachment #8404266 - Attachment is obsolete: true
Attachment #8411413 - Flags: review?(liuche)
Comment on attachment 8411409 [details] [diff] [review]
Part 1: Loading URL from an intent

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

::: mobile/android/base/TelemetryContract.java
@@ +29,5 @@
>  
>          // Set default panel.
>          public static final String PANEL_SET_DEFAULT = "setdefault.1";
> +
> +        // Loading a URL.

Nit: URL loaded, to be more consistent with the other comments.

@@ +38,5 @@
>       * Holds event methods. Intended for use in
>       * Telemetry.sendUIEvent() as the "method" parameter.
>       */
> +    public interface Method {
> +        // Event occurred via an intent.

I don't like the "Event occurred..." because it's going to get repetitive. I don't actually think this needs a comment.
Comment on attachment 8411413 [details] [diff] [review]
Part 4: Loading URL from browser search

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

Looking good.

However, you're missing telemetry for the case where you enter a non-url into the urlbar, and hit enter - I don't see a telemetry event being fired, but we're doing a search. Take a look at BrowserSearch? Make sure you're covering all the cases!

(Also, do a mxr search for the urlopen method, and make sure that you are checking each of those cases, and assessing whether or not it's a valid place for telemetry. In general, you'll want to use both manual testing *as well as* code paths, because you can't rely on just hitting all the cases that you can think of to manually test. This is especially important for telemetry, because we'll get skewed results if we never instrument a particular code path.)

::: mobile/android/base/TelemetryContract.java
@@ +46,5 @@
>          public static final String CONTEXT_MENU = "contextmenu";
>  
>          public static final String GRID_ITEM = "griditem";
>          public static final String LIST_ITEM = "listitem";
> +        public static final String SEARCH_SUGGESTION = "searchsuggestion";

Hm, I think we should change the naming of the search session and methods. Reading through this now, searchsuggestion should actually be a method, like the Google Instant suggestions you get if you turn on "search suggestions". This is different from search shortcut, which allows you to search with a particular engine.

Maybe name the session urlbar.search?

@@ +66,5 @@
>          // URL bar focused.
>          public static final String URLBAR_FOCUSED = "urlbar.1:";
> +
> +        // Search shortcut is open.
> +        public static final String SEARCH_SHORTCUT = "searchshortcut.1:";

We also need a searchsuggestion method here, for when a user clicks on an engine-suggested item. (I didn't notice this before because I didn't have search suggestions turned on!)

::: mobile/android/base/home/BrowserSearch.java
@@ +280,5 @@
>                  final Cursor c = mAdapter.getCursor(position);
>                  final String url = c.getString(c.getColumnIndexOrThrow(URLColumns.URL));
>  
> +                // The "urlbar" and "searchsuggestions" sessions can be open at the same time. Use the LIST_ITEM
> +                // method to set this LOAD_URL event apart from the case where the user commits whats in

Nit: what's

::: mobile/android/base/home/SearchEngineRow.java
@@ +81,5 @@
>                  // search for the term.
>                  if (v != mUserEnteredView && !StringUtils.isSearchQuery(suggestion, false)) {
>                      if (mUrlOpenListener != null) {
> +                        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL,
> +                                              TelemetryContract.Method.SEARCH_SUGGESTION);

Is this actually the correct event? I think this is the go button, right?
Attachment #8411413 - Flags: review?(liuche) → feedback+
Comment on attachment 8411412 [details] [diff] [review]
Part 3: Loading URLs from the url bar

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

You're missing another loadUrl here - do an mxr search to make sure you have all the places covered where a user could load a url.

::: mobile/android/base/BrowserApp.java
@@ +1589,4 @@
>              return;
>          }
>  
>          // Otherwise, check for a bookmark keyword.

Add instrumentation here, too, because you can open a url from a bookmark keyword.
Attachment #8411412 - Flags: review?(liuche) → feedback+
Comment on attachment 8411411 [details] [diff] [review]
Part 2: Loading URLs in about:home

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

Good, with nits.

Can you add comments about bug 1000534 in the relevant places? We don't have separate sessions for last tabs and most recent, so make that clear.

::: mobile/android/base/TelemetryContract.java
@@ +41,5 @@
>      public interface Method {
>          // Event occurred via an intent.
>          public static final String INTENT = "intent";
> +
> +        // Event occurred via a context menu.

contextmenu seems pretty self-explanatory - no comment needed.

@@ +44,5 @@
> +
> +        // Event occurred via a context menu.
> +        public static final String CONTEXT_MENU = "contextmenu";
> +
> +        public static final String GRID_ITEM = "griditem";

These two probably need comments though, like "Grid item of element selected" or "Grid item tapped".
Attachment #8411411 - Flags: review?(liuche) → review+
This update to Part 3 adds probes for 3 other loadUrls:
* Paste and Go: I just used the contextmenu Method. There is a different bug for adding contextmenu actions to telemetry themselves.
* Two loadUrls in the keyword code:
    One that isn't a keyword, so it's just a normal LOAD_URL
    One that is a keyword, so I add a "keyword" Extra param
Attachment #8411412 - Attachment is obsolete: true
Attachment #8414484 - Flags: review?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #77)

> However, you're missing telemetry for the case where you enter a non-url
> into the urlbar, and hit enter - I don't see a telemetry event being fired,
> but we're doing a search. Take a look at BrowserSearch? Make sure you're
> covering all the cases!

At this point, I think the only loadUrl of consequence left to cover is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/RemoteTabsList.java#67

I'll add a patch for that. If you (or anyone else) find others, please let me know. The code is pretty scattered and I could be confused.

> ::: mobile/android/base/TelemetryContract.java

> >          public static final String LIST_ITEM = "listitem";
> > +        public static final String SEARCH_SUGGESTION = "searchsuggestion";
> 
> Hm, I think we should change the naming of the search session and methods.
> Reading through this now, searchsuggestion should actually be a method, like
> the Google Instant suggestions you get if you turn on "search suggestions".
> This is different from search shortcut, which allows you to search with a
> particular engine.
> 
> Maybe name the session urlbar.search?

I changed "searchsuggestion" Method to "suggestion" since the URL is being loaded via a suggestion. We might have other suggestions in the future too.

"urlbar.search" sounded too much like a Session and I suggested "frecency" for that (below)


> >          public static final String URLBAR_FOCUSED = "urlbar.1:";
> > +
> > +        // Search shortcut is open.
> > +        public static final String SEARCH_SHORTCUT = "searchshortcut.1:";
> 
> We also need a searchsuggestion method here, for when a user clicks on an
> engine-suggested item. (I didn't notice this before because I didn't have
> search suggestions turned on!)

I changed the Session to "frecency.1" since the Session is set when you start typing in the URLBar and we are doing a frecency (awesomescreen) search.

> 
> ::: mobile/android/base/home/BrowserSearch.java

> > +                // The "urlbar" and "searchsuggestions" sessions can be open at the same time. Use the LIST_ITEM
> > +                // method to set this LOAD_URL event apart from the case where the user commits whats in
> 
> Nit: what's

Fixed and also changed "searchsuggestion" Session to "frecency".

> ::: mobile/android/base/home/SearchEngineRow.java
> @@ +81,5 @@
> >                  // search for the term.
> >                  if (v != mUserEnteredView && !StringUtils.isSearchQuery(suggestion, false)) {
> >                      if (mUrlOpenListener != null) {
> > +                        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL,
> > +                                              TelemetryContract.Method.SEARCH_SUGGESTION);
> 
> Is this actually the correct event? I think this is the go button, right?

The way the code is written and from the comment, this appears to be a case where a search suggestion (from Bing or Google) returns something like "mozilla.org" as the suggestion. In that case, just open "mozilla.org" directly and don't do a Bing or Google search for "mozilla.org".
This patch tweaks the Method and Session names. It also adds an Extra to the two SUGGESTION Methods ("url" and "search") to help give more context.
Attachment #8411413 - Attachment is obsolete: true
Attachment #8414501 - Flags: review?(liuche)
Missed a qref
Attachment #8414501 - Attachment is obsolete: true
Attachment #8414501 - Flags: review?(liuche)
Attachment #8414503 - Flags: review?(liuche)
Comment on attachment 8414484 [details] [diff] [review]
Part 3: Loading URLs from the url bar v0.3

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

Looks good - thanks for finding the other loadUrl locations.
Attachment #8414484 - Flags: review?(liuche) → review+
Comment on attachment 8414484 [details] [diff] [review]
Part 3: Loading URLs from the url bar v0.3

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

Looks good - thanks for finding the other loadUrl locations.

::: mobile/android/base/BrowserApp.java
@@ +1604,5 @@
>                  // If there isn't a bookmark keyword, load the url. This may result in a query
>                  // using the default search engine.
>                  if (TextUtils.isEmpty(keywordUrl)) {
>                      Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);
> +                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);

Actually, wouldn't we want this to be a Search event? We already check for url/non-search-query earlier (!StringUtils.isSearchQuery(x)) - maybe we should just assume this is a search query, not a url.
Comment on attachment 8414503 [details] [diff] [review]
Part 4: Loading URL from browser search v0.4

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

Looks good but one last thing - I'd like to see us differentiate between tapping on search suggestions provided by search engines (if you have search suggestions turned on), and search terms entered by the user. Since we're adding the telemetry probe anyways, let's make it finer-grained. It's also just a few more lines of code afaict.

Feel free to push back on this, though.

::: mobile/android/base/home/SearchEngineRow.java
@@ +80,5 @@
>                  // and the search matches a URL pattern, go to that URL. Otherwise, do a
>                  // search for the term.
>                  if (v != mUserEnteredView && !StringUtils.isSearchQuery(suggestion, false)) {
>                      if (mUrlOpenListener != null) {
> +                        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.SUGGESTION, "url");

We should probably document the extras somewhere, so that analyzing probes is easier. index.rst?

@@ +86,3 @@
>                          mUrlOpenListener.onUrlOpen(suggestion, EnumSet.noneOf(OnUrlOpenListener.Flags.class));
>                      }
>                  } else if (mSearchListener != null) {

One thing here - we're not checking for mUserEnteredView, which would let us know if the user clicked the bubble with the magnifying glass icon (the user-entered search term), or clicked one of the bubbles with a search term provided by the search suggestion engine provided (e.g., Google Suggestions).

It'd be pretty easy to add in one more check (if v == mUserEnteredView) and then remove the Telemetry from BrowserApp.onSearch().

If you do this, make sure you add a Telemetry event under performUserEnteredSearch too.
Attachment #8414503 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #86)
> Comment on attachment 8414503 [details] [diff] [review]
> Part 4: Loading URL from browser search v0.4
> 
> Review of attachment 8414503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but one last thing - I'd like to see us differentiate between
> tapping on search suggestions provided by search engines (if you have search
> suggestions turned on), and search terms entered by the user. Since we're
> adding the telemetry probe anyways, let's make it finer-grained. It's also
> just a few more lines of code afaict.

I'm not sure I follow you. Do we actually know if what the user typed is going to be a search or not in Java? I thought that was only really known in JS, after we nsIURIFixup.

> We should probably document the extras somewhere, so that analyzing probes
> is easier. index.rst?

Only once these patches land. I don't need one more thing to keep changing.

> @@ +86,3 @@
> >                          mUrlOpenListener.onUrlOpen(suggestion, EnumSet.noneOf(OnUrlOpenListener.Flags.class));
> >                      }
> >                  } else if (mSearchListener != null) {
> 
> One thing here - we're not checking for mUserEnteredView, which would let us
> know if the user clicked the bubble with the magnifying glass icon (the
> user-entered search term), or clicked one of the bubbles with a search term
> provided by the search suggestion engine provided (e.g., Google Suggestions).
> 
> It'd be pretty easy to add in one more check (if v == mUserEnteredView) and
> then remove the Telemetry from BrowserApp.onSearch().
> 
> If you do this, make sure you add a Telemetry event under
> performUserEnteredSearch too.

I don't understand this code well enough to make that change right now. I'll file a followup bug and assign it to someone who does know the code well enough to make that change.
(In reply to Mark Finkle (:mfinkle) from comment #87)

> I'm not sure I follow you. Do we actually know if what the user typed is
> going to be a search or not in Java? I thought that was only really known in
> JS, after we nsIURIFixup.

Follow the logic for FHR recording searches (Bug 873496 and friends). It covers every case, including inside JS.

Most of the time we know in Java, but not all.
I stuck in the finer-tuned probes I was talking about - we can in fact track the difference between if we're clicking on the user-entered search bubble, or one of the suggestion bubbles provided by engine-suggest. The extras are all super-detailed to be clear about what's happening, but they obviously shouldn't be the extras that are actually used.

Just need to remove the telemetry from BrowserApp.onSearch and do the handling of those taps upstream.

There's one somewhat confusing thing, which is the probably-not-url, but that's because our isThisAUrl-guessing code uses some very simple assumptions.

> > It'd be pretty easy to add in one more check (if v == mUserEnteredView) and
> > then remove the Telemetry from BrowserApp.onSearch().
> > 
> > If you do this, make sure you add a Telemetry event under
> > performUserEnteredSearch too.
> 
> I don't understand this code well enough to make that change right now. I'll
> file a followup bug and assign it to someone who does know the code well
> enough to make that change.

It looks like "performUserEnteredSearch" is a little misleading, in that its only caller is the on-click listener for the row (so a little hack so you can tap anywhere on the row, instead of having to hit the little bubbles).
Attachment #8415424 - Flags: feedback?(mark.finkle)
Comment on attachment 8415424 [details] [diff] [review]
Patch: finer-tuned Search telemetry

>                     Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);
>-                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);
>+                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, "probably-not-url");

I think we should just leave this one as :
Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);

We are basically treating it like "the user typed something and pressed 'go'"

>+                    if (v == mUserEnteredView) {
>+                        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.SUGGESTION, "user-entered");
>+                    } else {
>+                        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.SUGGESTION, "engine-suggestion");
>+                    }

I'd be OK with using "user-search" and "engine-search" here. Both are already logged as SUGGESTIONS

>         String searchTerm = getSuggestionTextFromView(mUserEnteredView);
>         if (mSearchListener != null) {
>+            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.SUGGESTION, "search-row-clicked");

If we feel we need the additional context, I'd go with "row-search". From a "what happened?" perspective, this is basically the same result as "user-search" above, but I can live with the finer grain Extra. 

It's a slippery slope. We could add Extras for a ton of things and then get mired down in naming issues. Let's try to continue to avoid Extras wherever possible. URLs are just a big PITA and messy, so I can go along with the Extras here.
Attachment #8415424 - Flags: feedback?(mark.finkle) → feedback+
Simple patch to track URLs loaded from RemoteTabs panel. This has no Sessions or other Events related to RemoteTabs or the Tabs Panel in general. That is a different bug.
Attachment #8415477 - Flags: review?(liuche)
(In reply to Mark Finkle (:mfinkle) from comment #90)
> Comment on attachment 8415424 [details] [diff] [review]
> Patch: finer-tuned Search telemetry
> 
> >                     Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);
> >-                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);
> >+                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, "probably-not-url");
> 
> I think we should just leave this one as :
> Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);
> 
> We are basically treating it like "the user typed something and pressed 'go'"
> 
> >+                    if (v == mUserEnteredView) {
> >+                        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.SUGGESTION, "user-entered");
> >+                    } else {
> >+                        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.SUGGESTION, "engine-suggestion");
> >+                    }
> 
> I'd be OK with using "user-search" and "engine-search" here. Both are
> already logged as SUGGESTIONS
> 
> >         String searchTerm = getSuggestionTextFromView(mUserEnteredView);
> >         if (mSearchListener != null) {
> >+            Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.SUGGESTION, "search-row-clicked");
> 
> If we feel we need the additional context, I'd go with "row-search". From a
> "what happened?" perspective, this is basically the same result as
> "user-search" above, but I can live with the finer grain Extra. 
> 
> It's a slippery slope. We could add Extras for a ton of things and then get
> mired down in naming issues. Let's try to continue to avoid Extras wherever
> possible. URLs are just a big PITA and messy, so I can go along with the
> Extras here.

Yep, these extras sound fine. I definitely wasn't suggesting that the extras I stuck in the patch actually become the extras that we use - I put them in there so it'd be clear what differentiated each probe. Feel free to rename them as anything you want - I'd use "user-entered" and "engine-suggestion" (or even pare them down to "user" and "engine") and use "user(-entered)" for the row search extra too. Drop the other ones, they were there just for clarity in the patch.
Attachment #8415477 - Flags: review?(liuche) → review+
Comment on attachment 8411409 [details] [diff] [review]
Part 1: Loading URL from an intent

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: We miss out on some "before changes" telemetry that could show affect of Fx32 changes
Testing completed (on m-c, etc.): Working on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

(request for all patches)
Attachment #8411409 - Flags: approval-mozilla-aurora?
Attachment #8411411 - Flags: approval-mozilla-aurora?
Attachment #8414484 - Flags: approval-mozilla-aurora?
Attachment #8414503 - Flags: approval-mozilla-aurora?
Comment on attachment 8415424 [details] [diff] [review]
Patch: finer-tuned Search telemetry

This became an r+ after IRC chat
Attachment #8415424 - Flags: review+
Attachment #8415424 - Flags: feedback+
Attachment #8415424 - Flags: approval-mozilla-aurora?
Attachment #8415477 - Flags: approval-mozilla-aurora?
Comment on attachment 8411409 [details] [diff] [review]
Part 1: Loading URL from an intent

Mark, no review here, normal?
Flags: needinfo?(mark.finkle)
Attachment #8411411 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8414503 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8415424 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8415477 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8414484 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8411409 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #97)
> Comment on attachment 8411409 [details] [diff] [review]
> Part 1: Loading URL from an intent
> 
> Mark, no review here, normal?

The patch was r+ in an older version, the dev made the requested changes and put up a new version. Some people will "carry" the r+ forward to the new patch, others (like this) don't
Flags: needinfo?(mark.finkle)
The following probes are logged in logcat:

1. Typing an entire url:
SendUIEvent: action = loadurl.1 method = null timestamp = 23705196 extras = null

2. Typing a little and then Autocompleted URL:
SendUIEvent: action = loadurl.1 method = null timestamp = 23746170 extras = null

3. Typing a little and then Awesomebar recommendation (History or Bookmark):
SendUIEvent: action = loadurl.1 method = listitem timestamp = 23814752 extras = null

4. Typing a little and then Search suggestion by search engine:
SendUIEvent: action = loadurl.1 method = suggestion timestamp = 23906086 extras = engine

5. Typing a little and then Search shortcut (including provider):
SendUIEvent: action = loadurl.1 method = suggestion timestamp = 23984083 extras = user

6. Open page from Top Sites panel (grid):
SendUIEvent: action = loadurl.1 method = griditem timestamp = 24216901 extras = 1

7. Open page from Top Sites panel (list):
SendUIEvent: action = loadurl.1 method = listitem timestamp = 24254687 extras = null

8. Open page from other panel:
SendUIEvent: action = loadurl.1 method = null timestamp = 24142846 extras = null

9. Open page from Context menu:
SendUIEvent: action = action.1 method = contextmenu timestamp = 24432769 extras = home_open_new_tab
SendUIEvent: action = loadurl.1 method = contextmenu timestamp = 24432769 extras = null

10. Open page from external app:
SendUIEvent: action = loadurl.1 method = intent timestamp = 24679822 extras = null

Verified as fixed in builds:
- 32.0a1 (2014-05-19);
- 31.0a2 (2014-05-19);

Device: Google Nexus 4 (Android 4.4.2).
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.