Closed Bug 1303333 Opened 8 years ago Closed 8 years ago

Implement the probe to count URI loads triggered by search

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(2 files, 7 obsolete files)

This bug is about laying the foundations for the work in bug 1300104, by designing a probe that counts the URI loads breaking them down by what kind of search action triggered the load. The document at [1] contains a list of potentially interesting search actions to be covered. For the following components:

* Awesomebar *
* Search *
* about:home:search *
* about:newtab:search*
* activity stream search*

we should track:

Search by pressing enter
Search using suggestion
Search using historical suggestion
Search using one-click button
Search using one-click keyword

We should also record URI loads by context-menu search.

[1] - https://docs.google.com/spreadsheets/d/1gDQbcKkMC1qj75DRsN5t5IkRs5oWMUxRYUnXflybJWw/edit#gid=0
Blocks: 1300104
Points: --- → 3
Priority: -- → P1
Whiteboard: [measurement:client]
Assignee: nobody → alessio.placitelli
Temporarily untaking due to workweek.
Assignee: alessio.placitelli → nobody
Assignee: nobody → alessio.placitelli
We were thinking about this format to store the search data in, as suggested in bug 1295932 comment 1:

> engagement.navigation <- group name
> awesomebar <- probe name
> not_specified, search, bookmark <- keys
> 
> engagement.navigation.awesomebar[search_enter] = 12345
> engagement.navigation.awesomebar[search_click] = 12345
> engagement.navigation.awesomebar[search_historical] = 12345

It will also land a cumulative search counts scalar:

> engagement.navigation.search[search_enter] = 12345
> engagement.navigation.search[search_click] = 12345
> engagement.navigation.search[search_historical] = 12345

This is forward compatible, as it is easy to extend the probe to support new keys.

> engagement.navigation.search[new_key] = 3456
> engagement.navigation.new_component[new_key] = 123

Georg, Brendan, what do you think? Does this make sense?
Flags: needinfo?(gfritzsche)
Flags: needinfo?(bcolloran)
Yep, the pattern
    engagement.navigation.UI_component[nav_action_performed]
looks good to me.

(One minor detail I notice: we want to make sure the word "search" is not overloaded-- since it could indicate a search navigation action or the search bar, we should be sure to use "searchbar" to indicate the UI element and "search_x" to indicate the action done through whichever UI e.g.:
> engagement.navigation.searchbar[search_enter] = 12345
> engagement.navigation.searchbar[search_click] = 12345
> engagement.navigation.searchbar[search_historical] = 12345
)
Flags: needinfo?(bcolloran)
Looks good, i agree with what Brendan said.
Flags: needinfo?(gfritzsche)
Brendan, just to make sure I get this right, could you please double check my interpretation of the next items is correct?
 
> Search by pressing enter

e.g. in the awesomebar, just write text and press enter.

> Search using suggestion

e.g. in the awesomebar, write text and then click on one of the results.

> Search using historical suggestion

e.g. in the awesomebar, write text and then click on one of the suggested top results (websites that you already visited)

> Search using one-click button

e.g. in the awesomebar, write text and then click on one of the search engines at the bottom of the suggestions list

> Search using one-click keyword

e.g. in the awesomebar, write the search engine "keyword" followed by the text to search
Flags: needinfo?(bcolloran)
> > Search using historical suggestion
>
> e.g. in the awesomebar, write text and then click on one
> of the suggested top results (websites that you already visited)

This one i think doesn't actually apply to the awesome bar-- the "search_historical" category would apply to the search bar, and the search boxes on the about:home and about:newtab. In those search boxes, when you enter a search term that is a substring of search terms you have used previously, the previously used search terms will be displayed at the top of your list of search suggestions with a little clock icon. Those are historical search suggestions, which are distinct from historical navigation suggestions in the awesomebar (the suggestions of previously visited pages).

So it will be:


engagement.navigation.searchbar[search_enter]
- just type text and press enter to trigger a search with default engine (as you said)

engagement.navigation.searchbar[search_suggestion]
- type text and click or use cursor keys+enter to select one of the search suggestions

engagement.navigation.searchbar[search_historical]
- type text and click or use cursor keys+enter to select one of the search queries that you've used previously

engagement.navigation.searchbar[search_oneclick]
- type text and click or use cursor keys+enter to select one of the search engine options

(and these same options would apply to the search boxes on about:newtab and about:home, so
engagement.navigation.home_searchbox
engagement.navigation.newtab_searchbox
or whatever you decide to call them)




search keyword shoulds only be available in the awesome bar i believe, so it should be

engagement.navigation.awesomebar[search_enter]
- just type text and press enter to trigger a search with default engine (as you said)

engagement.navigation.awesomebar[search_suggestion]
- type text and click or use cursor keys+enter to select one of the search suggestions

engagement.navigation.awesomebar[search_keyword]
- type a search keyword and query and hit enter to trigger the search


(and just to emphasize again, awesomebar[search_suggestion] and searchbar[search_historical] will be distinct from the non-search related awesomebar[history_suggestion], which will be when you select an item from your browsing history from the awesomebar drop-down. Since we've decided to focus on search initially, we don't have to implement awesomebar[history_suggestion] in this first round, but it will be in the next round (along with awesomebar[bookmark] etc...))



I also just noticed that the list in comment 1 should also include:

* context menu

--which are the searches performed by highlighting text in content, right clicking, and selecting the option to search for the highlighted text with your default engine.



so for the sake of completeness, i think the list of search-related navigation actions we need to instrument right now should be:

engagement.navigation.searchbar[search_enter]
engagement.navigation.searchbar[search_suggestion]
engagement.navigation.searchbar[search_historical]
engagement.navigation.searchbar[search_oneclick]

engagement.navigation.home_searchbox[search_enter]
engagement.navigation.home_searchbox[search_suggestion]
engagement.navigation.home_searchbox[search_historical]
engagement.navigation.home_searchbox[search_oneclick]

engagement.navigation.newtab_searchbox[search_enter]
engagement.navigation.newtab_searchbox[search_suggestion]
engagement.navigation.newtab_searchbox[search_historical]
engagement.navigation.newtab_searchbox[search_oneclick]

engagement.navigation.awesomebar[search_enter]
engagement.navigation.awesomebar[search_suggestion]
engagement.navigation.awesomebar[search_keyword]

engagement.navigation.contextmenu[search]

Javaun, please confirm that this looks comprehensive and that these probes match your understanding of the conversation we had in late August (in particular, recall that we'll be aiming to get the more detailed info you wanted on things like query length, suggestion position selected, etc., down the road once event-based pings are fully baked)
Flags: needinfo?(bcolloran) → needinfo?(jmoradi)
Note that in Nightly (for now) we should be counting engagement.navigation.awesomebar[search_oneclick]
 too.
Blocks: 1308705
Attached patch bug1303333.patch (obsolete) — Splinter Review
Attached patch bug1303333.patch (obsolete) — Splinter Review
Attachment #8799772 - Attachment is obsolete: true
Comment on attachment 8799775 [details] [diff] [review]
bug1303333.patch

Marco, I was finally able to put a rough patch together. However, I'm struggling with some issues:

- One-off searches from about:home don't seem to be counted counted (could be related to bug 1306639?).
- Keyword searches don't seem to be working in about:home: the keyword doesn't get intercepted and the search ends up being done with the default search engine.
- Keyword searches not working in about:newtab - same as about:home.
- Keyword searches not being counted in the urlbar (probably bug 1308211)
- Keyword search are not using the correct engine in the searchbar, same as about:home.
- BrowserUITelemetry is recording a one-off search when typing stuff in the searchbar without using any keyword (and so not one-off).

It would be great if you could give a high level look to the patch in its current, in case there's some obvious issue with the current design.

One last thing: do you have any suggestion on how to catch a "search suggestion"? I mean, how to determine if a search comes from the suggested results from a search engine from e.g. the urlbar?
Attachment #8799775 - Flags: feedback?(mak77)
Blocks: 1309256
Brendan, who is going to own these probes? You or :rweiss?
Status: NEW → ASSIGNED
Flags: needinfo?(bcolloran)
I admit I'm not sure what "ownership" means in this context, but let's say that it will be me. I think :rweiss has too much going on.
Flags: needinfo?(bcolloran)
(In reply to Alessio Placitelli [:Dexter] from comment #10)
> - One-off searches from about:home don't seem to be counted counted (could
> be related to bug 1306639?).

no it's unlikely related. That bug only causes us to not count when you type and press enter IN THE LOCATION BAR. 

I was testing this, on SEARCH_COUNT I see:
 "bing.abouthome": {
          "range": [
            1,
            2
          ],
          "bucket_count": 3,
          "histogram_type": 4,
          "values": {
            "0": 1,
            "1": 0
          },
          "sum": 1
that looks correct, but I don't see it counted in UITelemetry / countableevents / search-oneoff... That isn existing bug that should be fixed apart.

> - Keyword searches don't seem to be working in about:home: the keyword
> doesn't get intercepted and the search ends up being done with the default
> search engine.
> - Keyword searches not working in about:newtab - same as about:home.
> - Keyword search are not using the correct engine in the searchbar, same as
> about:home.

Yes, abouthome and newtab fields work like the searchbar field, rather than like the urlbar. The only input search field that manages search aliases and bookmark keywords is the urlbar.

> - Keyword searches not being counted in the urlbar (probably bug 1308211)

Are they accounted if you type, DOWN, UP, ENTER? If so it's likely bug 1306639. It should be merged soon and I hope it will be in tomorrow's nightly. So far it's in autoland.

> - BrowserUITelemetry is recording a one-off search when typing stuff in the
> searchbar without using any keyword (and so not one-off).

yes, it's counting a oneoff search with "unknown" engine IIRC. It's an existing bug that should be fixed apart.

> One last thing: do you have any suggestion on how to catch a "search
> suggestion"? I mean, how to determine if a search comes from the suggested
> results from a search engine from e.g. the urlbar?

Search suggestions in the urlbar are special moz-action uris with a searchengine property and a searchSuggestion property.
See what we do here: http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/browser/base/content/urlbarBindings.xml#430
Basically it may be enough to look for action.params.searchSuggestion

For the searchbar, everything that is not one-off or unknown should be a suggestion, I think?
(In reply to brendan c from comment #12)
> I admit I'm not sure what "ownership" means in this context, but let's say
> that it will be me. I think :rweiss has too much going on.

That basically means monitoring the probes, being responsible for explaining breakage and regressions, deciding whether its used or can be removed. Maybe you can talk with Rebecca and check who should be owning these long-term... just to be safe!
Flags: needinfo?(bcolloran)
(In reply to Alessio Placitelli [:Dexter] from comment #14)
> That basically means monitoring the probes, being responsible for explaining
> breakage and regressions, deciding whether its used or can be removed. Maybe
> you can talk with Rebecca and check who should be owning these long-term...
> just to be safe!

Let's go ahead and put my name in the form. I won't sign anything in blood, but i can do most of that (explaining breakage or regression would probably require help from you though, no matter who owns the probes).
Flags: needinfo?(bcolloran)
Comment on attachment 8799775 [details] [diff] [review]
bug1303333.patch

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

yeah the approach looks good

::: browser/base/content/browser.js
@@ +3798,5 @@
>     *        SearchesProvider for allowed values.
>     * @param selection [optional]
>     *        ({index: The selected index, kind: "key" or "mouse"}) If
>     *        the search was a suggested search, this indicates where the
>     *        item was in the suggestion list and how the user selected it.

Please add a javadoc for options

::: browser/components/search/content/search.xml
@@ +439,5 @@
>              telemetrySearchDetails = null;
>            }
> +          const opts = {"isOneOff": aOneOff};
> +          BrowserSearch.recordSearchInTelemetry(engine, "searchbar", telemetrySearchDetails,
> +                                                opts);

The entire _selectionDetails/telemetrySearchDetails stuff in autocomplete looks hackish to me, I'm not sure why search needs autocomplete to save this info, rather than overriding "handleEnter" and storing this info locally. The original changesets introducing this doesn't make sense to me.
I also don't know why part of the telemetry collection is in handleSearchCommandWhere and part is in doSearch, rather than being well collected together in a method.

It's not your fault and you don't have to fix this here to get a positive review, but I'm honestly puzzled by that code and it's worth filing a bug to clean it up.
Regardless, we can ask for review to florian about the search part if we have doubts.

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +43,5 @@
>  
> +const KNOWN_ONEOFF_SOURCES = [
> +  "oneoff-urlbar",
> +  "oneoff-searchbar",
> +  "unknown", // Weird case: this is the searchbar when using keywords.

abouthome and newtab?

@@ +47,5 @@
> +  "unknown", // Weird case: this is the searchbar when using keywords.
> +];
> +
> +// Try to improve readability by shortening the calls.
> +const Telemetry = Services.telemetry;

nit: I know it's shorter, but honestly it doesn't help much and only adds blame changes and size to the patch. Personally in my code I prefer to avoid shortcuts unless they remove a lot of noise, but I leave the decision to you.
The other subtle thing that happens here is that just by including the module it starts the telemetry service... but that's probably ignorable since it's likely to start early already.

@@ +264,5 @@
> +    this._handleSearchAction(source, options);
> +  },
> +
> +  _handleSearchAction(source, data) {
> +    const dataWrapper = data || {};

just use data={} argument and you can avoid this.

@@ +269,5 @@
> +    dump("\n**** DEBUG source: " + source + " data: " + JSON.stringify(dataWrapper) + "\n");
> +    const isOneOff = !!dataWrapper.isOneOff;
> +
> +    switch (source) {
> +      case "urlbar": // That's the awesomebar!

the comment seems pointless, but it's funny

@@ +273,5 @@
> +      case "urlbar": // That's the awesomebar!
> +      case "oneoff-urlbar":
> +      case "searchbar":
> +      case "oneoff-searchbar":
> +      case "unknown": // This is sent by the searchbar when using keywords.

well, it's not completely correct since the searchbar does not use "keywords"?
I'd suggest to not use the words "keyword" or "alias" to avoid confusion with bookmark keywords or search engine aliases...
You could use something like "confirming the typed text", or similar.
That said, this is a bug as previously said, I'd be happy if we'd fix that first in a separate bug and then do the right thing here.

@@ +274,5 @@
> +      case "oneoff-urlbar":
> +      case "searchbar":
> +      case "oneoff-searchbar":
> +      case "unknown": // This is sent by the searchbar when using keywords.
> +        // Intentional fall-through.

this comment doesn't look useful, it's pretty clear in this case since previous cases don't have a body

@@ +276,5 @@
> +      case "oneoff-searchbar":
> +      case "unknown": // This is sent by the searchbar when using keywords.
> +        // Intentional fall-through.
> +        this._handleSearchAndUrlbar(source, dataWrapper);
> +      break;

please align break with case contents

@@ +279,5 @@
> +        this._handleSearchAndUrlbar(source, dataWrapper);
> +      break;
> +      case "abouthome": {
> +        Telemetry.keyedScalarAdd("browser.engagement.navigation.about_home", "search_enter", 1);
> +      } break;

doesn't looks like bracing these cases is needed, we usually only brace when needed to reduce visual noise

@@ +321,5 @@
> +        return;
> +      }
> +
> +      // It's still a one-off search, but didn't come with a mouse or keyboard hint,
> +      // so this must be a "keyword" one-off.

please. let's find another term :)

But I'd suggest to rather fix the unknown bug and here add a "bogus" or unmetered/unhandled category. If we notice stuff in the bogus category then we know we have a bug somewhere.

::: browser/modules/test/browser_UsageTelemetry_search.js
@@ +36,5 @@
> +  // Don't press "Enter" if we don't want to.
> +  if (!simulateEnter) {
> +    return;
> +  }
> +  EventUtils.synthesizeKey("VK_RETURN", {});

You can also use EventUtils.sendKey("return");

Unfortunately it's not enough to just simulate enter to be sure we're done. This can cause intermittent failures.
When you press enter the awesomebar may decide it was not ready and delay it.
I'd suggest to look at promiseAutocompleteResultPopup, the short story is that if you wait for the search to be complete, then Enter will be handled immediately.
Another possibility could be to use load handlers to check when a page starts loading (see waitForDocLoadAndStopIt), or to have a way for your telemetry code to notify that it's done collecting, for example you could add a .testMode boolean to it and when it's done if .testMode is true it could send a notification to nsIObserverService.

@@ +64,5 @@
> +  // Create a new search engine.
> +  Services.search.addEngineWithDetails("MozSearch", "", "mozalias", "", "GET",
> +                                       "http://example.com/?q={searchTerms}");
> +
> +  /* // The following is needed for keyword searches.

ditto for "keyword"

@@ +84,5 @@
> +  Services.search.moveEngine(engine, 0);
> +
> +  // Make sure to restore the engine once we're done.
> +  registerCleanupFunction(function* () {
> +    Services.search.currentEngine = originalEngine;

you should also remove your custom engine

@@ +127,5 @@
> +
> +  yield BrowserTestUtils.removeTab(tab);
> +});
> +
> +add_task(function* test_searchbar() {

I'd suggest to move common utils to head.js and split the test per source.
First because in the future single tests may grow, second because the timeout is short and you risk to cause intermittent timeout failures by adding more and more cases.
I'd split at least into 3: urlbar, searchbar, content
Then you could also better split each single test into subtests using add_task. the result would be far more readable.
Attachment #8799775 - Flags: feedback?(mak77)
Depends on: 1311013
(In reply to Marco Bonardo [::mak] from comment #13)
> (In reply to Alessio Placitelli [:Dexter] from comment #10)
> > - One-off searches from about:home don't seem to be counted counted (could
> > be related to bug 1306639?).
> 
> no it's unlikely related. That bug only causes us to not count when you type
> and press enter IN THE LOCATION BAR. 
> 
> I was testing this, on SEARCH_COUNT I see:
>  "bing.abouthome": {
>           "range": [
>             1,
>             2
>           ],
>           "bucket_count": 3,
>           "histogram_type": 4,
>           "values": {
>             "0": 1,
>             "1": 0
>           },
>           "sum": 1
> that looks correct, but I don't see it counted in UITelemetry /
> countableevents / search-oneoff... That isn existing bug that should be
> fixed apart.

Filed bug 1311013 for that.
Depends on: 1311653
Attached patch bug1303333.patch (obsolete) — Splinter Review
Attachment #8799775 - Attachment is obsolete: true
Depends on: 1311691
Comment on attachment 8802901 [details] [diff] [review]
bug1303333.patch

@Georg, would you please check the usages of the probes from BrowserUsageTelemetry.jsm?

@Marco, I think this is ready for another look. Some comments to your points from the previous pass, below:

(In reply to Marco Bonardo [::mak] from comment #16)
> Comment on attachment 8799775 [details] [diff] [review]
> bug1303333.patch
> 
> Review of attachment 8799775 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/components/search/content/search.xml
> @@ +439,5 @@
> >              telemetrySearchDetails = null;
> >            }
> > +          const opts = {"isOneOff": aOneOff};
> > +          BrowserSearch.recordSearchInTelemetry(engine, "searchbar", telemetrySearchDetails,
> > +                                                opts);
> 
> [...]
> It's not your fault and you don't have to fix this here to get a positive
> review, but I'm honestly puzzled by that code and it's worth filing a bug to
> clean it up.

Filed bug 1311691!

> ::: browser/modules/BrowserUsageTelemetry.jsm
> @@ +43,5 @@
> >  
> > +const KNOWN_ONEOFF_SOURCES = [
> > +  "oneoff-urlbar",
> > +  "oneoff-searchbar",
> > +  "unknown", // Weird case: this is the searchbar when using keywords.
> 
> abouthome and newtab?

Due to bug 1311013, we're not getting any Telemetry from abouthome and newtab, so they are kind of unexpected. Maybe we can add them once that bug is fixed?

> @@ +273,5 @@
> > +      case "urlbar": // That's the awesomebar!
> > +      case "oneoff-urlbar":
> > +      case "searchbar":
> > +      case "oneoff-searchbar":
> > +      case "unknown": // This is sent by the searchbar when using keywords.
> 
> [...]
> That said, this is a bug as previously said, I'd be happy if we'd fix that
> first in a separate bug and then do the right thing here.

So, I did some bug archeology and found out that this behaviour "is not a bug, it's a feature".
There's a good discussion about that in bug 1195733 comment 7.

I made the comments clearer here and simplified my code.

> @@ +321,5 @@
> > +        return;
> > +      }
> > +
> > +      // It's still a one-off search, but didn't come with a mouse or keyboard hint,
> > +      // so this must be a "keyword" one-off.
> 
> please. let's find another term :)
> 
> But I'd suggest to rather fix the unknown bug and here add a "bogus" or
> unmetered/unhandled category. If we notice stuff in the bogus category then
> we know we have a bug somewhere.

Thanks for bringing this up! I tried to be as clear as possible in this new patch. Please let me know if we still need to change something.
Attachment #8802901 - Flags: review?(mak77)
Attachment #8802901 - Flags: feedback?(gfritzsche)
Brendan, just to recap the current state from the patch, this is what we can currently track:

* Urlbar (awesomebar) *

browser.engagement.navigation.urlbar["search_enter"] = common searches in the urlbar
browser.engagement.navigation.urlbar["search_keyword"] = searches from the urlbar using a search engine alias
browser.engagement.navigation.urlbar["search_oneoff"] = searches from the urlbar using a one-off engine, either selected using a mouse or the keyboard
browser.engagement.navigation.urlbar["search_suggestion"] = searches from the urlbar using a suggested search

* Searchbar *

browser.engagement.navigation.searchbar["search_enter"] = common searches in the searchbar
browser.engagement.navigation.searchbar["search_oneoff"] = searches from the searchbarusing a one-off engine, either selected using a mouse or the keyboard
browser.engagement.navigation.searchbar["search_suggestion"] = searches from the searchbarusing a suggested search

The searchbar does not support search engine aliases, so no "search_keyword" key here.

* about:home*

browser.engagement.navigation.about_home["search_enter"] = common searches in the search field of about:home

We don't have a way to get suggestion/one-off data until bug 1311013 is fixed. Moreover, these searches go in the "search_enter" key, as I've not found a way to take them out.

* about:newtab*

browser.engagement.navigation.about_newtab["search_enter"] = common searches in the search field of about:home

We don't have a way to get suggestion/one-off data until bug 1311013 is fixed. Moreover, these searches go in the "search_enter" key, as I've not found a way to take them out.

* activity stream search*

I will be addressing this as a separate patch in this bug or as a followup bug.

In addition to the keys listed above, each scalar supports the "search_unhandled" key. This is used to trap any edge/side case which is not explicitly covered.
Flags: needinfo?(bcolloran)
This is excellent Alessio, I'm thrilled about the progress!

It looks like every thing that it's currently possible to cover has been addressed, except for the context menu search? What is the status on that one -- in progress, or blocked by some difficulty?
Flags: needinfo?(bcolloran)
(In reply to brendan c from comment #21)
> This is excellent Alessio, I'm thrilled about the progress!
> 
> It looks like every thing that it's currently possible to cover has been
> addressed, except for the context menu search? What is the status on that
> one -- in progress, or blocked by some difficulty?

Whoops, my bad, the context menu search is there too, I just forgot to mention it :-D

browser.engagement.navigation.contextmenu["search"] = searches triggered by right clicking on content and selecting "Search with.."
Perfect! Thanks!
(In reply to Alessio Placitelli [:Dexter] from comment #19)
> > > +const KNOWN_ONEOFF_SOURCES = [
> > > +  "oneoff-urlbar",
> > > +  "oneoff-searchbar",
> > > +  "unknown", // Weird case: this is the searchbar when using keywords.
> > 
> > abouthome and newtab?
> 
> Due to bug 1311013, we're not getting any Telemetry from abouthome and
> newtab, so they are kind of unexpected. Maybe we can add them once that bug
> is fixed?

what are we gaining by doing that? the fix sounds simple like just adding strings here, and once bug is fixed, we'll automatically get telemetry. On the other side, not adding them now, when that bug will be fixed recordOneOffSearch will throw and report to the console, since we may forget about this addition.
I'd suggest at least to annotate in a comment in bug 1311013 that probe names should be added to this list, if we don't do that now.

> > @@ +273,5 @@
> > > +      case "urlbar": // That's the awesomebar!
> > > +      case "oneoff-urlbar":
> > > +      case "searchbar":
> > > +      case "oneoff-searchbar":
> > > +      case "unknown": // This is sent by the searchbar when using keywords.
> > 
> > [...]
> > That said, this is a bug as previously said, I'd be happy if we'd fix that
> > first in a separate bug and then do the right thing here.
> 
> So, I did some bug archeology and found out that this behaviour "is not a
> bug, it's a feature".
> There's a good discussion about that in bug 1195733 comment 7.

That comment doesn't tell me it's a feature, it just tells me that the naming bug can be interpreted as a search through the current engine. I bet it happened by chance when the current engine was hidden from one-offs. It's still a bug, first because it uses a weird name (could use "current" instead of "unknown"), second because it's not a search that starts from a one-off button, the fact the button in hidden is unrelated.
If this wouldn't be reported as a oneoff search, it would still be reported in the search counts, and then it would still be trivial to subtract one-off searches from the count to know how many searches went through the current engine.

Now, it's clear this bug is not responsible to fix that mess, but surely we should not propagate dumb "unknown" naming in this new probe. I think you are indeed not propagating the mistake, and that's fine.
And I suppose it's hard to change an existing probe cause we may break existing studies.
(In reply to Alessio Placitelli [:Dexter] from comment #20)
> browser.engagement.navigation.urlbar["search_enter"] = common searches in
> the urlbar
> browser.engagement.navigation.urlbar["search_keyword"] = searches from the
> urlbar using a search engine alias

can we rename this to search_alias?
since we also have bookmarks keywords (that COULD run searches but we don't have very good way to reverse parse that) and the fact we use "keywords" word also when speaking about a bunch-of-words, it seems confusing.

> browser.engagement.navigation.searchbar["search_suggestion"] = searches from
> the searchbarusing a suggested search

I wonder if, in all the cases, you may want to distinghuish whether the suggestion was "local" (form history) or "remote" (coming directly from the engine). Do you care about that? Cause if you do it could be worth to split this further.

> * about:home*
> * about:newtab*
> We don't have a way to get suggestion/one-off data until bug 1311013 is
> fixed. Moreover, these searches go in the "search_enter" key, as I've not
> found a way to take them out.

Could you please also annotate this into bug 1311013, we should not have a probe for newtab working differently from a probe in the searchbar, it would be really confusing if one counts one-off in Enter and the other doesn't.
Comment on attachment 8802901 [details] [diff] [review]
bug1303333.patch

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

Just in case, I'd suggest to do a try run and retrigger the b-c tests ten times, to be further sure there's no intermittent, since there's quite a bit of async handling there.

that said, I have some doubts regarding the one-off handling that I'd like having clarified, below.

::: browser/base/content/browser.js
@@ +3794,5 @@
>     * @param engine
>     *        (nsISearchEngine) The engine handling the search.
>     * @param source
> +   *        (string) Where the search originated from. BrowserUsageTelemetry for
> +   *        allowed values.

you lost a "See ".

::: browser/base/content/urlbarBindings.xml
@@ +487,5 @@
>              if (fixup && fixup.keywordProviderName) {
>                [url, postData] =
>                  this._recordSearchEngineLoad(selectedOneOff.engine, value,
> +                                             event, where, openUILinkParams,
> +                                             {isSuggestion: false, isKeyword: false});

why do you need to do this? the object is optional afaict, so there should be the need to pass an object with all false entries. It should work even not touching this at all.
It doesn't matter much what we do here cause in bug 1310737 I will remove this code that currently we never hit, because UnifiedComplete already returns a searchengine action for this case, so this is a noop atm.

@@ +610,5 @@
> +              eventType = "key";
> +            } else if (event instanceof MouseEvent) {
> +              eventType = "mouse";
> +            }
> +          }

The external if (event) should not be needed, instanceof is sufficient

@@ +616,5 @@
> +          let opts = searchActionDetails || {};
> +          opts.isOneOff = isOneOff;
> +          opts.type = eventType;
> +
> +          BrowserSearch.recordSearchInTelemetry(engine, "urlbar", undefined, opts);

what about we rename "selection" argument in recordSearchInTelemetry to "details" and we also use it to pass the other details?
It will just need a small fix to countSearchEvent, where instead of if (selection) we should check if (selection && "index" in selection) or we could add a further layer like details.selection, then we'd only check "selection" in details.
I'd prefer avoiding adding another obscure parameter here.

::: browser/components/search/content/search.xml
@@ +403,5 @@
>              }
>            }
>  
> +          // This is a one-off search only if oneOffRecorded is true.
> +          this.doSearch(textValue, aWhere, aEngine, aParams, oneOffRecorded);

conceptually it may be wrong to register telemetry BEFORE the fact actually happens (it may throw)... but at this point I'd leave that to bug 1311691 to figure out. I'd add a comment there to see if we can coalesce all the telemetry code to a single method and run it after the search.

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +269,3 @@
>     * @throws if source is not in the known sources list.
>     */
> +  recordSearch(engine, source, options) {

I'd suggest to rename options to details

@@ +286,5 @@
> +    }
> +
> +    // Dispatch the search signal to other handlers.
> +    this._handleSearchAction(source, options);
> +  },

Is there a reason we need this rather than just reusing recordSearch and check the isOneOff flag to exclude SEARCH_COUNTS? the double code path doesn't seem to bring much more clarity.

@@ +289,5 @@
> +    this._handleSearchAction(source, options);
> +  },
> +
> +  _handleSearchAction(source, data={}) {
> +    const isOneOff = !!data.isOneOff;

this is unused

@@ +343,5 @@
> +      // keyboard hint, increment the scalar using the relative key.
> +      if (KNOWN_TYPES.includes(data.type)) {
> +        Services.telemetry.keyedScalarAdd(scalarName, "search_oneoff", 1);
> +        return;
> +      }

Sounds like a bug, maybe the one you said about newtab/abouthome not accounting?
Why should a consumer do differently from urlbar/searchbar? that sounds like a recipe for bugs. all the consumers should act in a coherent way.
If all the consumers would be coherent, we could completely avoid invoking _handleSearchAction if isOneOff is set in recordSearch. So that when we are here we always know stuff should be registered.
So please let me know what case this code is covering.

@@ +350,5 @@
> +      if (data.isSuggestion) {
> +        // It came from a suggested search, so count it as such.
> +        Services.telemetry.keyedScalarAdd(scalarName, "search_suggestion", 1);
> +        return;
> +      } else if (data.isKeyword) {

else after return (note: you should enable eslint in this folder and use --no-ignore when working on new code, it really helps)

@@ +352,5 @@
> +        Services.telemetry.keyedScalarAdd(scalarName, "search_suggestion", 1);
> +        return;
> +      } else if (data.isKeyword) {
> +        // This one came from a search that used an alias.
> +        Services.telemetry.keyedScalarAdd(scalarName, "search_keyword", 1);

Again I'd name this "alias" to avoid confusion with bookmark keywords

::: browser/modules/test/browser_UsageTelemetry_content.js
@@ +67,5 @@
> +  // Select all the text in the page.
> +  yield ContentTask.spawn(tab.linkedBrowser, "", function*() {
> +    return new Promise(resolve => {
> +      content.document.addEventListener("selectionchange", function selectionChanged() {
> +        content.document.removeEventListener("selectionchange", selectionChanged);

you can use the once option on addEventListener so you don't need to remove it. See https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

@@ +72,5 @@
> +        resolve();
> +      });
> +      content.document.getSelection().selectAllChildren(content.document.body);
> +    });
> +  });

please add some info() in browser chrome test, before doing important actions, so in case of a timeout we can more easily figure out where we were. Very often you can replace comments like "// about to do this." with "info("about to do this);"

::: browser/modules/test/browser_UsageTelemetry_urlbar.js
@@ +6,5 @@
> +const SUGGEST_URLBAR_PREF = "browser.urlbar.suggest.searches";
> +// The name of the search engine used to generate suggestions.
> +const SUGGESTION_ENGINE_NAME = "browser_UsageTelemetry usageTelemetrySearchSuggestions.xml";
> +
> +let searchInAwesomebar = Task.async(function* (inputText, win=window, fireInputEvent=false) {

fireInputEvent is not used atm and could be removed along with its code. it's actually only needed for very specific tests, not here.

@@ +38,5 @@
> +  const expectedSuggestionName = entryName + " " + SUGGESTION_ENGINE_NAME + " Search";
> +  for (let child of gURLBar.popup.richlistbox.children) {
> +    if (child.label === expectedSuggestionName) {
> +      // This entry is the search suggestion we're looking for.
> +      EventUtils.synthesizeMouseAtCenter(child, {}, window);

I think you can use child.click()?
Attachment #8802901 - Flags: review?(mak77) → feedback+
Attached patch bug1303333.patch (obsolete) — Splinter Review
Attachment #8802901 - Attachment is obsolete: true
Attachment #8802901 - Flags: feedback?(gfritzsche)
(In reply to Marco Bonardo [::mak] from comment #26)
> > browser.engagement.navigation.searchbar["search_suggestion"] = searches from
> > the searchbarusing a suggested search
> 
> I wonder if, in all the cases, you may want to distinghuish whether the
> suggestion was "local" (form history) or "remote" (coming directly from the
> engine). Do you care about that? Cause if you do it could be worth to split
> this further.

We will be adding "local" suggestions in another bug. This bug should only count searches coming from the engine. Can you suggest any way to discard "local" suggestions?
I think the results have a "style" = "fromhistory" (srsly, I don't know if this is a typo!) http://searchfox.org/mozilla-central/source/toolkit/components/satchel/nsFormAutoCompleteResult.jsm#135
The style can be obtained using .controller.getStyleAt(selectedIndex) you should be able to find other examples
Comment on attachment 8803947 [details] [diff] [review]
bug1303333.patch

(In reply to Marco Bonardo [::mak] from comment #29)
> I think the results have a "style" = "fromhistory" (srsly, I don't know if
> this is a typo!)
> http://searchfox.org/mozilla-central/source/toolkit/components/satchel/
> nsFormAutoCompleteResult.jsm#135
> The style can be obtained using .controller.getStyleAt(selectedIndex) you
> should be able to find other examples

Mh, I just double checked and it looks like we're already skipping suggestions coming from the form history, thanks to the check at: http://searchfox.org/mozilla-central/rev/3079f45ce59e105e2490b1c8b3a5a52cd47a5ac0/browser/base/content/urlbarBindings.xml#452

I think this should be ready for another pass. Thanks Marco!
Attachment #8803947 - Flags: review?(mak77)
Comment on attachment 8803947 [details] [diff] [review]
bug1303333.patch

data-review?rweiss -  Rebecca, can you take on the data-review for these probes as Benjamin is a bit swamped at the moment?

We're adding a new set of engagement measurements to break down the count of searches by the action and UI component which triggered them. The counts are being tracked in a set of new opt-out keyed scalar probes (see the Scalars.yaml file).

We're adding test coverage for each single behaviour we're tracking.

The probes are set to expire in FF55.
Attachment #8803947 - Flags: review?(rweiss)
I assert the following for data review:
- Brendan is tagged to serve as the primary analyst (and Javaun should also be considered a sponsor for these measurements)
- These probes are temporary, but pending Brendan and Javaun's evaluation they are candidates for permanent tracking.

Signing off on data review.
Attachment #8803947 - Flags: review?(rweiss) → review+
Hi Brendan and Rebecca. Sorry, I've been out for two weeks. The list looks great. Besides the awesomebar[bookmark] and [history_suggestion] you mentioned that we don't need for first round, there may be a few we've missed.

For both Awesome Bar and Search Bar it would be:
[search_settings] -- (clicking on the gear icon to bring up settings, i.e. to change defaults)
[keyword_search] -- (using a prefix to search an engine, i.e. "wi:" for wikipedia.

We may already have these and if not we don't need to block in round 1. we'll catch in round2.  "keyword_search" is necessarily constrained for privacy reasons. While we log engine name for search engine plugins, a prefix could apply to any web search box (i.e. a search box on an adult content site) and we don't want to know where they've gone. 


(In reply to brendan c from comment #6)
> we don't have to implement
> awesomebar[history_suggestion] in this first round, but it will be in the
> next round (along with awesomebar[bookmark] etc...))
> 

> I also just noticed that the list in comment 1 should also include:
> 
> * context menu
> 
> so for the sake of completeness, i think the list of search-related
> navigation actions we need to instrument right now should be:
> 
> engagement.navigation.searchbar[search_enter]
> engagement.navigation.searchbar[search_suggestion]
> engagement.navigation.searchbar[search_historical]
> engagement.navigation.searchbar[search_oneclick]
> 
> engagement.navigation.home_searchbox[search_enter]
> engagement.navigation.home_searchbox[search_suggestion]
> engagement.navigation.home_searchbox[search_historical]
> engagement.navigation.home_searchbox[search_oneclick]
> 
> engagement.navigation.newtab_searchbox[search_enter]
> engagement.navigation.newtab_searchbox[search_suggestion]
> engagement.navigation.newtab_searchbox[search_historical]
> engagement.navigation.newtab_searchbox[search_oneclick]
> 
> engagement.navigation.awesomebar[search_enter]
> engagement.navigation.awesomebar[search_suggestion]
> engagement.navigation.awesomebar[search_keyword]
> 
> engagement.navigation.contextmenu[search]
> 
> Javaun, please confirm that this looks comprehensive and that these probes
> match your understanding of the conversation we had in late August (in
> particular, recall that we'll be aiming to get the more detailed info you
> wanted on things like query length, suggestion position selected, etc., down
> the road once event-based pings are fully baked)
Flags: needinfo?(rweiss)
Flags: needinfo?(jmoradi)
Flags: needinfo?(bcolloran)
I NI'ed you both around the question for the other probe, but I want to be clear I'm approving for landing if we're ready to go.
(In reply to Javaun Moradi [:javaun] from comment #33)
> For both Awesome Bar and Search Bar it would be:
> [search_settings] -- (clicking on the gear icon to bring up settings, i.e.
> to change defaults)
> [keyword_search] -- (using a prefix to search an engine, i.e. "wi:" for
> wikipedia.

This tracks "search_keyword", a breakdown is shown in comment 20.

search_settings wouldn't really fit in this bug - the probe here is about (web) navigation as a result of a search UI interaction.
If we need to track search setting interactions we should file a new bug on this (or start with adding them to a "planned/urgent measurements" tracking document).
(In reply to Alessio Placitelli [:Dexter] from comment #30)
> Mh, I just double checked and it looks like we're already skipping
> suggestions coming from the form history, thanks to the check at:
> http://searchfox.org/mozilla-central/rev/
> 3079f45ce59e105e2490b1c8b3a5a52cd47a5ac0/browser/base/content/urlbarBindings.
> xml#452

The urlbar doesn't provide (for now) local suggestions (bug 1181644), but the searchbar does, and as such you may still want to exclude those.
You can try just searching for something from the searchbar, then typing again part of what you searched, you should see the formhistory entry.
the above code sounds unrelated to search suggestions, that's handling one-off buttons.
Per IRC discussion, it's ok to keep local and remote search suggestions in the same probe since both cause the same kind of search.
Depends on: 1286594
(In reply to Alessio Placitelli [:Dexter] from comment #20)
> * activity stream search*
> 
> I will be addressing this as a separate patch in this bug or as a followup
> bug.

It looks like that, due to bug 1286594, we won't be able to count the searches coming from activity stream.
Comment on attachment 8803947 [details] [diff] [review]
bug1303333.patch

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

::: browser/base/content/browser.js
@@ +3830,5 @@
>    recordOneoffSearchInTelemetry: function (engine, source, type, where) {
>      let id = this._getSearchEngineId(engine) + "." + source;
>      BrowserUITelemetry.countOneoffSearchEvent(id, type, where);
> +    try {
> +      const details = {"type": type, "isOneOff": true};

you can use property shorthands, "type: type" becomes just "type". And you don't need the apices.
const details = { type, isOneOff: true };

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +279,5 @@
> +      if (!KNOWN_SEARCH_SOURCES.includes(source)) {
> +        throw new Error("Unknown source for search: " + source);
> +      }
> +      let countId = getSearchEngineId(engine) + "." + source;
> +      Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS").add(countId);

I'd rewrite this using a string template (backticks) as

if (!KNOWN_SEARCH_SOURCES.includes(source)) {
  throw new Error(`Unknown source for${isOneOff ? " one-off " : " "}search: ${source}`);
}
// add a brief comment about why we are not increasing SEARCH_COUNTS for OneOff
if (!isOneOff) {
  ...increase search_count
}

Don't worry if you go alightly over 80 chars on the exception, we accept longer text when splitting makes it less readable.

@@ +302,5 @@
> +      case "newtab":
> +        Services.telemetry.keyedScalarAdd("browser.engagement.navigation.about_newtab", "search_enter", 1);
> +        break;
> +      case "contextmenu":
> +        Services.telemetry.keyedScalarAdd("browser.engagement.navigation.contextmenu", "search", 1);

You can probably move the second and third argument to the next line (aligned with the first argument) to shorten these 3 lines.

@@ +328,5 @@
> +      // search came from "oneoff-urlbar". That's because both signals
> +      // are propagated from search.xml. Skip it if that's the case.
> +      // Moreover, we skip the "unknown" source that comes from the searchbar
> +      // when performing searches from the default search engine. See bug 1195733
> +      // comment 7 for contaxt.

typo: context

@@ +334,5 @@
> +        return;
> +      }
> +
> +      // If that's a legit one-off search signal, increment the scalar using the
> +      //relative key.

missing whitespace

@@ +343,5 @@
> +      if (details.isSuggestion) {
> +        // It came from a suggested search, so count it as such.
> +        Services.telemetry.keyedScalarAdd(scalarName, "search_suggestion", 1);
> +        return;
> +      } else if (details.isKeyword) {

why is this still named isKeyword, let's be coherent and use alias everywhere.

@@ +349,5 @@
> +        Services.telemetry.keyedScalarAdd(scalarName, "search_alias", 1);
> +        return;
> +      }
> +
> +      // Still a plain search, with no cue about the triggering input device.

what does "with no cue about the triggering input device" mean?
Attachment #8803947 - Flags: review?(mak77) → review+
Attached patch bug1303333.patch (obsolete) — Splinter Review
Attachment #8803947 - Attachment is obsolete: true
Attachment #8804595 - Flags: review+
(In reply to Marco Bonardo [::mak] from comment #39)
> Comment on attachment 8803947 [details] [diff] [review]
> bug1303333.patch
> 
> Review of attachment 8803947 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks Marco!

> I'd rewrite this using a string template (backticks) as
> 
> if (!KNOWN_SEARCH_SOURCES.includes(source)) {
>   throw new Error(`Unknown source for${isOneOff ? " one-off " : " "}search:
> ${source}`);
> }
> // add a brief comment about why we are not increasing SEARCH_COUNTS for
> OneOff
> if (!isOneOff) {
>   ...increase search_count
> }
> 
> Don't worry if you go alightly over 80 chars on the exception, we accept
> longer text when splitting makes it less readable.

As discussed over IRC, I'm skipping this since we have different sources whitelists for oneoffs and non-oneoff searches. 

> @@ +349,5 @@
> > +        Services.telemetry.keyedScalarAdd(scalarName, "search_alias", 1);
> > +        return;
> > +      }
> > +
> > +      // Still a plain search, with no cue about the triggering input device.
> 
> what does "with no cue about the triggering input device" mean?

I've clarified the comment. This search comes from typing text + enter.
Attached patch bug1303333.patch (obsolete) — Splinter Review
Attachment #8804595 - Attachment is obsolete: true
Attachment #8804807 - Flags: review+
Marco, I've attached an updated patch that removes the "search_unhandled" key, since that branch was never hit due to the returns. I've also refactored that part to suppress the eslint error that finally showed up (had to refresh m-c). Does that still look good?

As discussed over IRC, there is a single intermittent issue here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b59734ae7034&selectedJob=29926988

> TEST-UNEXPECTED-FAIL | browser/modules/test/browser_UsageTelemetry_content.js | Test timed out -
> TEST-UNEXPECTED-FAIL | browser/modules/test/browser_UsageTelemetry_content.js | Found a tab after previous test timed out: about:home -

It seems that, when a single test writing into the search field in about:home or about:newtab runs twice, the second time it always fails. It doesn't seem to be limited to this new test, as I can reproduce the same behaviour by doing:

 ./mach mochitest browser/base/content/test/general/browser_aboutHome.js --repeat 2

I traced down the issue in my test, and it points me at [1]. It looks like when running the test the second time, we hit that, as if the about:home search controller was not initialized. I double checked and the Controller constructor is hit. Any suggestion on how to move forward, as I'm a bit lost?

In case we're not able to fix that, I'd rather land this probes along with all the tests (potentially introducing a new intermittent) and moving the fix to a separate bug: we wouldn't want to regress these data points.

What do you think?

[1] - http://searchfox.org/mozilla-central/source/browser/base/content/contentSearchUI.js#252
Flags: needinfo?(mak77)
I don't know if what you can reproduce with --repeat is the same bug we see on the Try server.

From what I see, --repeat is broken.
When using it, the browser-chrome harness fires "shutdown-leaks-before-check", that kills things like the ContentSearch controller.
The controller is only initialized in final-ui-startup, but looks like --repeat doesn't go through it (maybe it doesn't completely shutdown the browser). So once the first instance kills the controller, it won't be initialized again.
That means basically --repeat breaks ContentSearch and cannot be used to debug this problem.
I'll file a bug about this and try to cc someone who may know about it.

But I don't think on Try we could hit that case, since I think we always restart the process between folders.
I have filed bug 1313358, we should still not rely on that for out intermittent, cause it may be unrelated.
(In reply to Marco Bonardo [::mak] from comment #46)
> I have filed bug 1313358, we should still not rely on that for out
> intermittent, cause it may be unrelated.

Thanks for filing the bug and for helping with this one. By examining the screenshot taken on the failing test [1], I can see that focusing the search bar in content works, as we're able to see the "test query" string that is synthesized by | yield typeInSearchField(tab.linkedBrowser, "test query", "searchText");|.

It can't be stuck on the Telemetry calls, as we're not yielding on |snapshotKeyedScalars| and we're printing logs for the calls after that.

That points to the two remaining lines of the test:

> info("Trigger a simple serch, just test + enter.");
> let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> yield typeInSearchField(tab.linkedBrowser, "test query", "searchText");
> // *** The two lines below ***
> yield BrowserTestUtils.synthesizeKey("VK_RETURN", {}, tab.linkedBrowser);
> yield p;

Assuming |synthesizeKey| it's doing its job, the problem could be |yield p;|: that line should unblock once the results page is loaded.

Any suggestion on where to go from there?

[1] - http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/9671a3430f468418ef03bc12db1bee9a24e4d5c52affe0c12ed0922e5315e3a66e2e90c50ea7d5b4b50544c5bc09fbc52a38a20497703d1a951a71f4b48626c0
(In reply to Alessio Placitelli [:Dexter] from comment #44)
> What do you think?

If I'd tell you that you can land an intermittent, the sheriffs would burn me alive.

My suggestion is to land it with the subtest for about:home disabled (provided that way it doesn't fail intermittently) and immediately file (and work on) a follow-up bug to try to figure out that failure and re-enable the sub-test asap.

The only suggestion I may give so far, is maybe to try waiting for the controller to be hooked up in the page? something like:

    info("waiting for the search controller to have been initialized");
    yield ContentTask.spawn(browser, null, function* () {
      while(!content.wrappedJSObject.gContentSearchController) {
        yield new Promise(resolve => setTimeout(resolve, 10));
      }
    });

Would this help? I don't know but could be worth a try run.
also, if --run-until-failure does a real restart it could work better than --repeat (I didn't try)

(In reply to Alessio Placitelli [:Dexter] from comment #47)
> > info("Trigger a simple serch, just test + enter.");
> > let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> > yield typeInSearchField(tab.linkedBrowser, "test query", "searchText");
> > // *** The two lines below ***
> > yield BrowserTestUtils.synthesizeKey("VK_RETURN", {}, tab.linkedBrowser);
> > yield p;
> 
> Assuming |synthesizeKey| it's doing its job, the problem could be |yield
> p;|: that line should unblock once the results page is loaded.

Hm wait, are you waiting for about:home to be fully loaded before trying to search? doesn't look like.
So it's possible the search controller has not been initialized yet (it's initialized in "pageshow")
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #48)
> (In reply to Alessio Placitelli [:Dexter] from comment #44)
> > What do you think?
> 
> If I'd tell you that you can land an intermittent, the sheriffs would burn
> me alive.

EHhehe, you're right :-D

> My suggestion is to land it with the subtest for about:home disabled
> (provided that way it doesn't fail intermittently) and immediately file (and
> work on) a follow-up bug to try to figure out that failure and re-enable the
> sub-test asap.

I'd rather not land a new probe without test coverage. I'll try to investigate some more
and then see if I can follow your suggestion. Thanks!

> The only suggestion I may give so far, is maybe to try waiting for the
> controller to be hooked up in the page? something like:
> 
>     info("waiting for the search controller to have been initialized");
>     yield ContentTask.spawn(browser, null, function* () {
>       while(!content.wrappedJSObject.gContentSearchController) {
>         yield new Promise(resolve => setTimeout(resolve, 10));
>       }
>     });
> 
> Would this help? I don't know but could be worth a try run.

I'm trying that as we speak.

> also, if --run-until-failure does a real restart it could work better than
> --repeat (I didn't try)

Unfortunately, --repeat and --run-until-failure seem to behave the same way locally :(

> (In reply to Alessio Placitelli [:Dexter] from comment #47)
> > > info("Trigger a simple serch, just test + enter.");
> > > let p = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> > > yield typeInSearchField(tab.linkedBrowser, "test query", "searchText");
> > > // *** The two lines below ***
> > > yield BrowserTestUtils.synthesizeKey("VK_RETURN", {}, tab.linkedBrowser);
> > > yield p;
> > 
> > Assuming |synthesizeKey| it's doing its job, the problem could be |yield
> > p;|: that line should unblock once the results page is loaded.
> 
> Hm wait, are you waiting for about:home to be fully loaded before trying to
> search? doesn't look like.
> So it's possible the search controller has not been initialized yet (it's
> initialized in "pageshow")

|yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:home");| defaults to waiting for the page to load. That should be enough, right?
(In reply to Alessio Placitelli [:Dexter] from comment #49)
> |yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:home");|
> defaults to waiting for the page to load. That should be enough, right?

ah yes, though load is fired before pageshow, iirc.
Attached patch bug1303333.patch (obsolete) — Splinter Review
Attachment #8804807 - Attachment is obsolete: true
Attachment #8805666 - Flags: review+
Carrying over the r+.

As discussed over IRC, I've moved the about:home tests to a separate test file and I'm waiting for the "AboutHomeLoadSnippetsCompleted" message before doing anything. It looks like this works and doesn't produce any intermittent issue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=637448852b78
https://hg.mozilla.org/integration/mozilla-inbound/rev/73aacf4a8f568cfd66a7a306cce0879bbdd5de5b
Bug 1303333 - Implement the probe to count URI loads triggered by search. r=mak, data-review=rweiss
(In reply to Phil Ringnalda (:philor) from comment #54)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> f408d5bef95d9dce2cbbc1251e40a360f2a4cc01 for OS X failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=38402979&repo=mozilla-
> inbound

Ok, that intermittent was unexpected :-/

I'm disabling the test for now and following up with investigations on bug 1313825.
Attached patch bug1303333.patchSplinter Review
Attachment #8805666 - Attachment is obsolete: true
Attachment #8805744 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c1f4f53f7c1e950f107e3dae0b4e5a93d237de
Bug 1303333 - Implement the probe to count URI loads triggered by search. r=mak, data-review=rweiss
Keywords: checkin-needed
Depends on: 1313825
https://hg.mozilla.org/mozilla-central/rev/e7c1f4f53f7c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to Javaun Moradi [:javaun] from comment #34)
> I NI'ed you both around the question for the other probe, but I want to be
> clear I'm approving for landing if we're ready to go.

I concur with Georg in comment 35 that that probes for search settings are probably best for a secondary effort once we have all the probes for navigation (actions that trigger URI loads sorted out). Thanks Javaun!
Flags: needinfo?(bcolloran)
Depends on: 1314650
Hi Alessio,
could you please nominate this uplift to Aurora51 based on comment #22 of bug 1310737?
Flags: needinfo?(alessio.placitelli)
(In reply to Gerry Chang [:gchang] from comment #60)
> Hi Alessio,
> could you please nominate this uplift to Aurora51 based on comment #22 of
> bug 1310737?

Hi Gerry,

before nominating this for uplift, I need to take a closer look at the data that's coming in. If everything checks out, I'm going to request uplifts for this and the related patches today.

The validation is taking place in bug 1308705.
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8805744 [details] [diff] [review]
bug1303333.patch

Approval Request Comment
[Feature/regressing bug #]: This patch lands a new set of important browser engagement measurements. This patch needs to be uplifted together with bug 1313825 (which stacks on this).
[User impact if declined]: None.
[Describe test coverage new/current, TreeHerder]: New coverage is added as part of this patch.
[Risks and why]: We're adding new probes, so we shouldn't regress old data. Moreover, if something is not working properly, the worst that can happen is that we get wrong data. Early data validation is taking place in bug 1308705.
[String/UUID change made/needed]: None
Attachment #8805744 - Flags: approval-mozilla-aurora?
Comment on attachment 8805744 [details] [diff] [review]
bug1303333.patch

This patch adds new probes for search. Take it in 51 aurora.
Attachment #8805744 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(alessio.placitelli)
Depends on: 1304446
Thanks Ryan, taking care of that now.
Flags: needinfo?(alessio.placitelli)
That's the rebased patch for aurora. It also changes 2 tests that were failing exclusively on aurora, due to features being disabled there.
Attachment #8809008 - Attachment is patch: true
Depends on: 1316835
Depends on: 1318333
Blocks: 1295932
No longer blocks: 1300104
Depends on: 1315650
Flags: needinfo?(rweiss)
(In reply to Alessio Placitelli [:Dexter] from comment #31)
> 
> The probes are set to expire in FF55.

this will not be enough time to evaluate these probes, seeing as 55 is upon us. we need at least 6 more months on this.
Flags: needinfo?(rweiss)
(In reply to brendan c from comment #68)
> (In reply to Alessio Placitelli [:Dexter] from comment #31)
> > 
> > The probes are set to expire in FF55.
> 
> this will not be enough time to evaluate these probes, seeing as 55 is upon
> us. we need at least 6 more months on this.

Brendan, these probes were set to never expire in bug 1341219, so you should be covered ;-)
Flags: needinfo?(rweiss)
You need to log in before you can comment on or make changes to this bug.