Record searches in Firefox Health Report

RESOLVED FIXED in Firefox 21

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
Firefox 21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fhr])

Attachments

(6 attachments, 10 obsolete attachments)

2.53 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.97 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.48 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
3.29 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
4.86 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.46 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
Recording search counts from about:home is likely more involved than other search "origins." So, I'm filing a separate bug to track it. See also bug 828540 (tracks search provider implementation) and bug 587780.

Mark's original patch lives at https://bug718067.bugzilla.mozilla.org/attachment.cgi?id=603363. I vaguely recall hearing some feedback on the security perspective. But, I can't find that comment on Bugzilla (even if it exists).
Firing an event from aboutHome.js and observing it as BrowserOnClick in browser.js does for click events should be relatively straightforward. You can use a simple CustomEvent and attach the engine name as "detail", though.
Whiteboard: [fhr]
Only f? because the test fails. (It hangs and it appears no search is performed. However, if I type text in the form and hit submit, things happen.) I think my DOM manipulation in the test is wrong. I know very little about DOM so I'm sure I'm doing something stupid like not manipulating or submitting the form properly.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #712681 - Flags: feedback?(gavin.sharp)
Comment on attachment 712681 [details] [diff] [review]
Part 1: Dispatch DOM event when search occurs, v1

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

::: browser/base/content/test/browser_aboutHome.js
@@ +101,5 @@
> +      ok(e.detail, "Event has detail.");
> +      is(e.detail, doc.documentElement.getAttribute("searchEngineName"), "Detail is search engine name");
> +
> +      executeSoon(runNextTest);
> +    });

this should also have ", true, true" additional arguments

@@ +104,5 @@
> +      executeSoon(runNextTest);
> +    });
> +
> +    doc.getElementById("searchText").value = "it works";
> +    form.submit();

the problem is that onsubmit is not activated by this, you should rather do
doc.getElementById("searchSubmit").click();
ah, and I suggest, inside the event listener to add a
gBrowser.selectedTab.linkedBrowser.stop();
With Marco's changes, things work!

This patch also has a nice side-effect: we now have test coverage of the search button on about:home!
Attachment #712681 - Attachment is obsolete: true
Attachment #712681 - Flags: feedback?(gavin.sharp)
Attachment #712955 - Flags: review?(gavin.sharp)
Comment on attachment 712955 [details] [diff] [review]
Part 1: Dispatch DOM event when search occurs, v2

>diff --git a/browser/base/content/abouthome/aboutHome.js b/browser/base/content/abouthome/aboutHome.js

>+    document.getElementById("searchForm").dispatchEvent(event);

It probably makes more sense to fire the event on the document rather than on the search form, though I suppose it doesn't matter much.

>diff --git a/browser/base/content/test/browser_aboutHome.js b/browser/base/content/test/browser_aboutHome.js

>+  run: function () {
>+    let doc = gBrowser.selectedTab.linkedBrowser.contentDocument;

let doc = gBrowser.contentDocument;

>+      ok(e.detail, "Event has detail.");
>+      is(e.detail, doc.documentElement.getAttribute("searchEngineName"), "Detail is search engine name");

These checks are redundant, remove the "ok".

>+      gBrowser.selectedTab.linkedBrowser.stop();

gBrowser.stop();

This patch only covers the event firing, are you going to do the actual observing/recording in a separate bug?
Attachment #712955 - Flags: review?(gavin.sharp) → review+
While I could probably carry r+ forward, given this is about:home, I want to be sure. Part 2 will add observer to browser.js.
Attachment #712955 - Attachment is obsolete: true
Attachment #712970 - Flags: review?(gavin.sharp)
Attachment #712970 - Flags: review?(gavin.sharp) → review+
No longer blocks: 828540
Depends on: 828540
Gavin gets browser.js and mochitest sanity.
Richard gets FHR integration.

A note on perf:

Because FHR is delay loaded, if a search is performed before FHR has initialized, the search will trigger FHR to load. This is all async except for the SQLite DB open (which has no async API). Once FHR has fully initialized, a SQL query will be issued (again async).

This has performance implications. Will they be noticed? Time will tell. We should also keep in mind that navigating to a new page causes Places to issue SQL. Does that justify adding a new query for FHR? No. But it does put things in perspective.

Long term, for performance reasons we'll want some kind of buffer and flush mechanism to record events and flush them to DB periodically. I believe the scope of this is too large to complete by Firefox 21. I propose will eat this perceived perf loss in 21. If we find it impacting Telemetry significantly, we can look into uplifting whatever we implement in 22 or disable search recording in 21. i.e. until someone has numbers showing this to be an issue, I'm not inclined to prematurely optimize it.
Attachment #713058 - Flags: review?(rnewman)
Attachment #713058 - Flags: review?(gavin.sharp)
Yoric: See perf note in comment #8.
Perf note seen. Given that there is what looks like a big perf question mark, are we sure that want to add this feature immediately?

Also, couldn't some specific Telemetry help us diagnose whether a slowdown is related to searching from about:home when FHR is not initialized?
(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> Perf note seen. Given that there is what looks like a big perf question
> mark, are we sure that want to add this feature immediately?

As with any piece of FHR, we have to weigh the benefits of having data against a [possibly measurable] perf loss. This is not a decision I can make.

> Also, couldn't some specific Telemetry help us diagnose whether a slowdown
> is related to searching from about:home when FHR is not initialized?

Not sure how we'd isolate things unless we recorded a per-session value of when FHR was initialized and then were able to correlate Telemetry numbers against sessions with delayed vs early init. Can Telemetry even do that?
(In reply to Gregory Szorc [:gps] from comment #11)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> > Perf note seen. Given that there is what looks like a big perf question
> > mark, are we sure that want to add this feature immediately?
> 
> As with any piece of FHR, we have to weigh the benefits of having data
> against a [possibly measurable] perf loss. This is not a decision I can make.

Well, consider the question asked :)

> > Also, couldn't some specific Telemetry help us diagnose whether a slowdown
> > is related to searching from about:home when FHR is not initialized?
> 
> Not sure how we'd isolate things unless we recorded a per-session value of
> when FHR was initialized and then were able to correlate Telemetry numbers
> against sessions with delayed vs early init. Can Telemetry even do that?

Well, as long as you can detect the case from your code, you can map it to distinct histograms.
Comment on attachment 713058 [details] [diff] [review]
Part 2: Record search in FHR, v1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+#ifdef MOZ_SERVICES_HEALTHREPORT
>+    document.addEventListener("AboutHomeSearchEvent", function onSearch(e) {

I was going to call this a bug, but then I saw that this function's argument shadows the global "document". Not your fault, but can you fix it while here? (s/document/doc/)

>+      let reporter = Cc["@mozilla.org/datareporting/service;1"]
>+                       .getService(Ci.nsISupports)

getService()'s argument is optional, FWIW (applies to the test too)

>diff --git a/browser/base/content/test/browser_aboutHome.js b/browser/base/content/test/browser_aboutHome.js

>+    doc.addEventListener("AboutHomeSearchEvent", function onSearch(e) {

What guarantees that this AboutHomeSearchEvent listener will be fired after the browser chrome's AboutHomeSearchEvent listener? I guess the browser chrome's listener is guaranteed to be added first given the way the test harness works, but that's at the very least non-obvious. Not sure how to best address this - guess you could just add a comment.
Attachment #713058 - Flags: review?(gavin.sharp) → review+
I think we should instrument this using Telemetry and land it, and then evaluate based on data instead of theoretical fears.  Ten seconds for FHR init is already somewhat arbitrary, so invoking a few seconds earlier may or may not actually matter when we measure it in the real world.  Let's let the data guide us here. :)
Now with a helper function to record searches.
Attachment #713058 - Attachment is obsolete: true
Attachment #713058 - Flags: review?(rnewman)
Attachment #713095 - Flags: review?(rnewman)
Attachment #713095 - Flags: review?(gavin.sharp)
Comment on attachment 713095 [details] [diff] [review]
Part 2: Record search in FHR, v2

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+function recordSearch(engine, source) {

Let's not pollute the global window scope with this relatively-generic symbol. What other callers will this have? Can it live in its own module, or some other related module perhaps? Or I guess we could just give it a more specific name.

>+  if (!reporter) {

When would this reasonably happen?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16)
> >+function recordSearch(engine, source) {
> 
> Let's not pollute the global window scope with this relatively-generic
> symbol. What other callers will this have? Can it live in its own module, or
> some other related module perhaps? Or I guess we could just give it a more
> specific name.

This will be called by context menu searching and possibly awesomebar and search bar searching (I still need to find their call sites).

I'm going to assert it can't live in its own module because Perf will likely yell at me for the compartment overhead. Once zones lands, however...

I'm not sure if there is a better module for it. Maybe the main FHR XPCOM service or the search service's XPCOM?

I'm leaning towards more a more descriptive name.

> >+  if (!reporter) {
> 
> When would this reasonably happen?

There is a pref to effectively disable the XPCOM service which will result in reporter always being null.
Wouldn't that cause getService() to throw?
Comment on attachment 713095 [details] [diff] [review]
Part 2: Record search in FHR, v2

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

::: browser/base/content/browser.js
@@ +7465,5 @@
> + * @param source
> + *        (string) Where the search originated from. See the FHR
> + *        SearchesProvider for allowed values.
> + */
> +function recordSearch(engine, source) {

I would rename this to "reportSearchFromSource" or somesuch, and only have it as a separate function if it can also be used for context menu and other search sources.

(If we're going to have the same function in several places, let's either fix that or suck it up and inline it.)

@@ +7474,5 @@
> +                   .wrappedJSObject
> +                   .healthReporter;
> +
> +  if (!reporter) {
> +    return;

Firstly, why would this occur?

Secondly, if it's common, can we log and/or report telemetry here that we're missing out on searches?

@@ +7479,5 @@
> +  }
> +
> +  reporter.onInit().then(function record() {
> +    try {
> +      reporter.getProvider("org.mozilla.searches").recordSearch(engine, source);

Can we do some basic checking outside of this onInit? For example, if engine or source is falsy for some reason (custom build?) then we can save initing the reporter.

::: browser/base/content/test/browser_aboutHome.js
@@ +110,5 @@
> +  desc: "Check that performing a search records to Firefox Health Report.",
> +  setup: function () { },
> +  run: function () {
> +    if (!("@mozilla.org/datareporting/service;1" in Components.classes)) {
> +      runNextTest();

Log!
I'm going to expand scope of this bug to include other search sources as well since review of earlier parts seems to rely on knowledge of what all is going on. And, the code will be similar for all the sources, so we might as well do all the similar pieces in one go.

Updated part 2 coming shortly.
Attachment #713123 - Flags: review?(rnewman)
Attachment #713123 - Flags: review?(gavin.sharp)
(In reply to Richard Newman [:rnewman] from comment #19)
> @@ +7474,5 @@
> > +                   .wrappedJSObject
> > +                   .healthReporter;
> > +
> > +  if (!reporter) {
> > +    return;
> 
> Firstly, why would this occur?
> 
> Secondly, if it's common, can we log and/or report telemetry here that we're
> missing out on searches?

See comment in forthcoming patch. It's not common and it's not worth logging because logging would just pollute the console of everybody who has the entire FHR service disabled.

> @@ +7479,5 @@
> > +  }
> > +
> > +  reporter.onInit().then(function record() {
> > +    try {
> > +      reporter.getProvider("org.mozilla.searches").recordSearch(engine, source);
> 
> Can we do some basic checking outside of this onInit? For example, if engine
> or source is falsy for some reason (custom build?) then we can save initing
> the reporter.

Why? This is an internal API. If we are getting crap in, we should have tests for that or the reportError below will catch things. I just don't think this warrants additional checking.

> 
> ::: browser/base/content/test/browser_aboutHome.js
> @@ +110,5 @@
> > +  desc: "Check that performing a search records to Firefox Health Report.",
> > +  setup: function () { },
> > +  run: function () {
> > +    if (!("@mozilla.org/datareporting/service;1" in Components.classes)) {
> > +      runNextTest();
> 
> Log!

Meh. This is essentially #ifdef MOZ_SERVICES_HEALTHREPORT without feeding the test through a preprocessor (because we don't have good make rules for that).
I /think/ I hit points of feedback.
Attachment #713095 - Attachment is obsolete: true
Attachment #713095 - Flags: review?(rnewman)
Attachment #713095 - Flags: review?(gavin.sharp)
Attachment #713129 - Flags: review?(rnewman)
Attachment #713129 - Flags: review?(gavin.sharp)
Summary: Record searches from about:home for FHR → Record searches in Firefox Health Report
Attachment #713129 - Flags: review?(gavin.sharp) → review+
Comment on attachment 713123 [details] [diff] [review]
Part 3: Record context menu searches in FHR, v1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   loadSearch: function BrowserSearch_search(searchText, useNewTab, pur

>     // getSubmission can return null if the engine doesn't have a URL
>     // with a text/html response type.  This is unlikely (since
>     // SearchService._addEngineToStore() should fail for such an engine),
>     // but let's be on the safe side.
>-    if (!submission)
>-      return;
>+    if (!submission) {
>+      return null;
>+    }

This really can't happen, and it's silly to try to "be on the safe side". Since it makes your life a bit more complicated now, let's just rip this check out and avoid the need to add PerformContextMenuSearch.
Attachment #713123 - Flags: review?(gavin.sharp) → review-
Comment on attachment 713129 [details] [diff] [review]
Part 2: Record about:home searches, v3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+function recordSearchInHealthReport(engine, source) {
>+// We only do preprocessor here so call sites can remain clean.
>+#ifdef MOZ_SERVICES_HEALTHREPORT

uber-nit: comments related to preprocessing should be preprocessor comments (s@//@#@)
Tom: I can't remember if you've answered this, but what's Privacy's opinion on recording search counts when in private browsing mode? FHR records the search engine {Google, Yahoo, Bing, Amazon.com, other} (companies Mozilla has a relationship with) and where that search was performed from (about:home, search bar, etc). If you install a non-default search engine, we count that in a unified "other" bucket (i.e. no details of that search engine are released). We do not record any details of the search, just a count.
Flags: needinfo?(tom)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #23)
> Comment on attachment 713123 [details] [diff] [review]
> Part 3: Record context menu searches in FHR, v1
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >   loadSearch: function BrowserSearch_search(searchText, useNewTab, pur
> 
> >     // getSubmission can return null if the engine doesn't have a URL
> >     // with a text/html response type.  This is unlikely (since
> >     // SearchService._addEngineToStore() should fail for such an engine),
> >     // but let's be on the safe side.
> >-    if (!submission)
> >-      return;
> >+    if (!submission) {
> >+      return null;
> >+    }
> 
> This really can't happen, and it's silly to try to "be on the safe side".
> Since it makes your life a bit more complicated now, let's just rip this
> check out and avoid the need to add PerformContextMenuSearch.

According to your link on IRC, add-ons use loadSearch(). I'm asserting we don't want FHR to lump add-on initiated searches into the "context menu" bucket. That leaves us with the following options:

1) Change browser-context.inc to perform the extra function call to recordSearchInHealthReport().

2) Detect purpose == "contextmenu" and hope add-ons don't lie. (I suppose add-ons could call PerformContextMenuSearch() themselves, so we can't prevent all mis-use).

3) Add yet another parameter to loadSearch() to define the FHR search source. We could possibly add an "add-on" bucket to FHR.

4) ???
Good point. Let's keep PerformContextMenuSearch (but as BrowserSearch.loadSearchFromContext) but remove the "returns the engine name" changes from loadSearch.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #27)
> Good point. Let's keep PerformContextMenuSearch (but as
> BrowserSearch.loadSearchFromContext) but remove the "returns the engine
> name" changes from loadSearch.

So you want me to re-implement the logic for determining the current engine? That seems prone to breakage. Adding the return value seemed like the least fragile solution.
No, I do not want that! I don't know what I want! :)

Let's go with your patch essentially as-is, but with PerformContextMenuSearch ->BrowserSearch.loadSearchFromContext then.
Comment on attachment 713129 [details] [diff] [review]
Part 2: Record about:home searches, v3

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

::: browser/base/content/browser.js
@@ +2628,5 @@
>                          AboutHomeUtils.defaultSearchEngine.searchURL);
> +
> +    doc.addEventListener("AboutHomeSearchEvent", function onSearch(e) {
> +      recordSearchInHealthReport(e.detail, "abouthome");
> +    }, true, true);

Let's wrap the addEventListener in MOZ_SERVICES_HEALTHREPORT, too. No sense having an event listener and function calls for nothing.

I know this will offend your sense of cleanliness.
Attachment #713129 - Flags: review?(rnewman) → review+
Comment on attachment 713123 [details] [diff] [review]
Part 3: Record context menu searches in FHR, v1

Waiting for updated patch.
Attachment #713123 - Flags: review?(rnewman)
(In reply to Gregory Szorc [:gps] from comment #21)

> > Can we do some basic checking outside of this onInit? For example, if engine
> > or source is falsy for some reason (custom build?) then we can save initing
> > the reporter.
> 
> Why? This is an internal API. If we are getting crap in, we should have
> tests for that or the reportError below will catch things. I just don't
> think this warrants additional checking.

Why? Because it's an available API with potentially expensive side-effects. Better to error *before* doing the expensive side-effects, IMO, even if you don't think it could ever happen, because you don't have nearly so much control over the future as you might think!

(This is pure defensive programming, but with perf justification.)

The additional checking is cheap: if (!engine || !source) { ... }.


> Meh. This is essentially #ifdef MOZ_SERVICES_HEALTHREPORT without feeding
> the test through a preprocessor (because we don't have good make rules for
> that).

You mean you've never had to debug a problem and found that a test was short-cutting rather than failing? Better noisy short-cutting than quiet short-cutting.
I'm pretty sure the test is wrong in many ways. While it passes when executed by itself, when run as part of a suite, it takes like 30s to run and spews a lot of terminal output. I'm pretty sure I need to clean up a tab or something. However, I have no clue how all those APIs work. If I need to wait for events, you are going to need to spell it out for me.
Attachment #713170 - Flags: review?(gavin.sharp)
So how exactly does one hook into the awesomebar? The original patch used docshell/base/nsDefaultURIFixup.cpp. Will I have to patch C++ to get this working?

If so, perhaps we should add recordSearchInHealthReport to nsISearchService so it can be called from C++. FHR does not yet expose an IDL so AFAIK it would be somewhat difficult to call it from C++. (Adding IDL is on our TODO list, of course. We just haven't had a need for it.)
Comment on attachment 713170 [details] [diff] [review]
Part 4: Record searches from search bar, v1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  dump("Recording search: " + engine + " " + source + "\n");

oops

>diff --git a/browser/components/search/test/browser_healthreport.js b/browser/components/search/test/browser_healthreport.js

>+function test() {

>+  if (!("@mozilla.org/datareporting/service;1" in Components.classes)) {
>+    finish();

Calling finish() without making any assertions will cause a test failure, add an ok(true, "skipping test") or something.

>+      searchBar.value = "firefox health report";
>+      searchBar.focus();

You'll need to re-set the search bar value before finish() as well (and it might be a good idea to focus the content area as well).
Attachment #713170 - Flags: review?(gavin.sharp) → review+
(In reply to Gregory Szorc [:gps] from comment #33)
> I'm pretty sure the test is wrong in many ways. While it passes when
> executed by itself, when run as part of a suite, it takes like 30s to run
> and spews a lot of terminal output. I'm pretty sure I need to clean up a tab
> or something. However, I have no clue how all those APIs work. If I need to
> wait for events, you are going to need to spell it out for me.

Is that test using the default engine to do a search?  If so, I'm pretty sure that touching the network during tests is frowned upon. I think that network access is blocked from the test infra. but that doesn't prevent developers from hitting the network on their own machines.  

Take a look at https://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/browser_426329.js for an existing test that uses a test search engine, waits for a tab to open (rather than using executeSoon), and closes the tabs when it's done.
Comment on attachment 713170 [details] [diff] [review]
Part 4: Record searches from search bar, v1

Oh, I missed the latest comments here. I guess all of the tests here will need some work to use a "dummy" search provider.
Attachment #713170 - Flags: review+
To hook into the awesome bar search, you could add an observer notification similar to defaultURIFixup-using-keyword-pref to the branch of nsDefaultURIFixup::KeywordToURI that calls getSubmission, I guess. It's a bit tricky because the code that actually triggers the load is not closely tied to the getSubmission caller, but we can probably get something "good enough".
Blocks: 820834
I moved the FHR helper into BrowserSearch. Not sure why I didn't do this from the start.
Attachment #713129 - Attachment is obsolete: true
Attachment #713230 - Flags: review?(gavin.sharp)
Comment on attachment 713230 [details] [diff] [review]
Part 2: Record about:home searches, v4

I guess perhaps having the test trigger a search with the default search provider may not be a problem if it is immediately stopped and the result of the load isn't observed in any way.
Attachment #713230 - Flags: review?(gavin.sharp) → review+
I think I hit all points of feedback.
Attachment #713123 - Attachment is obsolete: true
Attachment #713262 - Flags: review?(rnewman)
Attachment #713262 - Flags: review?(gavin.sharp)
Attachment #713262 - Flags: review?(gavin.sharp) → review+
Attachment #713262 - Flags: review?(rnewman) → review+
This seems to work!
Attachment #713170 - Attachment is obsolete: true
Attachment #713493 - Flags: review?(gavin.sharp)
Landed about:home integration.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c95d8f93249
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0c5ea9e93b1

Will land the rest later (hopefully).
Whiteboard: [fhr] → [fhr][leave open]
Target Milestone: --- → Firefox 21
Reset search bar value. Focus back to window.
Attachment #713493 - Attachment is obsolete: true
Attachment #713493 - Flags: review?(gavin.sharp)
Attachment #713507 - Flags: review?(gavin.sharp)
Slight changes to how gBrowser.stop() is called. Also documented a potential future test failure.
Attachment #713262 - Attachment is obsolete: true
Attachment #713521 - Flags: review?(gavin.sharp)
Attachment #713521 - Flags: review?(gavin.sharp) → review+
Comment on attachment 713507 [details] [diff] [review]
Part 4: Record searches from search bar, v3

This test should open a new tab and have the search be performed there, and then just close it (rather than triggering the search in the current tab and then loading about:blank).
Comment on attachment 713507 [details] [diff] [review]
Part 4: Record searches from search bar, v3

>diff --git a/browser/components/search/test/browser_healthreport.js b/browser/components/search/test/browser_healthreport.js

>+function test() {

>+    // We need a test or else we'll be marked as failure.
>+    ok("Firefox Health Report is not enabled.");

ok(true, "Firefox...");

>+        function afterSearch() {

>+          content.location.href = "about:blank";
>+          window.focus();

No need for the focus() thing, the search bar takes care of that for you, and no need for the load of about:blank if you use a new tab.
Attachment #713507 - Flags: review?(gavin.sharp)
This is my first patch to C++ in Gecko.
Attachment #713567 - Flags: review?(gavin.sharp)
Incorporated review feedback.

Dropped focus(). Using new tab.
Attachment #713507 - Attachment is obsolete: true
Attachment #713572 - Flags: review?(gavin.sharp)
This should be the final patch in this bug!
Attachment #713646 - Flags: review?(gavin.sharp)
Attachment #713572 - Flags: review?(gavin.sharp) → review+
Attachment #713646 - Attachment description: Part 5: Record urlbar searches, v1 → Part 6: Record urlbar searches, v1
Comment on attachment 713567 [details] [diff] [review]
Part 5: Create notification when keyword searches are performed, v1

>diff --git a/docshell/base/nsDefaultURIFixup.cpp b/docshell/base/nsDefaultURIFixup.cpp

It's worth considering that this notification will only fire for searches where we use the default engine, and not for those that use the keyword.URL pref value. Depending on how we want to use this data we may want to also have a defaultURIFixup-using-keyword-pref observer that puts searches in the "other" bucket. This would be very useful for determining the extent of the keyword.URL hijacking problem.

>diff --git a/docshell/test/browser/browser_search_notification.js b/docshell/test/browser/browser_search_notification.js

>+function test() {

>+    let engine = Services.search.defaultEngine;

The back-end uses originalDefaultEngine, so you should use that here too (though in practice in test runs they will always be the same).
Attachment #713567 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #54)
> It's worth considering that this notification will only fire for searches
> where we use the default engine, and not for those that use the keyword.URL
> pref value. Depending on how we want to use this data we may want to also
> have a defaultURIFixup-using-keyword-pref observer that puts searches in the
> "other" bucket. This would be very useful for determining the extent of the
> keyword.URL hijacking problem.

Good point. I'm going to assert that some data from the awesomebar is better than no data and that filing a follow-up to capture keyword.URL searches will suffice.
Attachment #713646 - Flags: review?(gavin.sharp) → review+
Blocks: 841551
Landed parts 5 and 6 in services-central because inbound is a rainbow. rnewman should (hopefully) merge to m-c by EOD so this hits Friday's Nightly.

Thank you so much for all the attentive reviews, Gavin!

https://hg.mozilla.org/services/services-central/rev/1725b56ec1a3
https://hg.mozilla.org/services/services-central/rev/953b1db7a246
Whiteboard: [fhr][leave open] → [fhr][fixed in services]
https://hg.mozilla.org/mozilla-central/rev/1725b56ec1a3
https://hg.mozilla.org/mozilla-central/rev/953b1db7a246
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fhr][fixed in services] → [fhr]
Depends on: 844537
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.