Closed Bug 1036914 Opened 10 years ago Closed 10 years ago

Log selections of awesomebar results that "are like" the url in the current search provider.

Categories

(Firefox :: General, defect)

x86
All
defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.1
Tracking Status
firefox35 --- verified

People

(Reporter: bwinton, Assigned: bwinton)

References

Details

Attachments

(1 file, 2 obsolete files)

"Selections" mean both "clicks on" or "enter keypresses when focused".
Also, be sure to log the type of selection (enter key or mouse click).
Flags: firefox-backlog+
Blocks: 1036916
We may also want to log results from other search engines, in which case we may want to log whether the result is from the users currently selected search engine or not.
QA Whiteboard: [qa+]
Added to Iteration 33.3.  Blake, can you provide the point estimate.
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Iteration: --- → 33.3
Flags: needinfo?(bwinton)
Points: --- → 3
Flags: needinfo?(bwinton)
Attached patch bug1036914.diff (obsolete) — Splinter Review
Mak: You've reviewed code in toolkit/components/autocomplete/nsAutoCompleteController.cpp before, so I'm pinging you for that.  (It's a small change to pass the text that was changed in the urlbar so that I can use it elsewhere.)

Mconley: The rest of it is for you.  :)
You might think that doing all that string manipulation and regexp creation all the time would be heinously slow, but it turns out to only take between 2 and 4 ms, and we only do it when the user has selected an autocomplete entry, so it seems like a reasonable amount of time to take that infrequently.  Also, if you feel comfortable reviewing the toolkit code, feel free to take that part, too.  :)

Thanks!
Attachment #8458322 - Flags: review?(mconley)
Attachment #8458322 - Flags: review?(mak77)
To test this, select (using either a mouse click or the enter key) a couple of items from the urlbar's autocomplete dropdown.
Some should be searches with your current search engine, some should be searches with a different engine, some should just be saved urls.
In about:telemetry » Simple Measurements » UITelemetry there should be a "search" section, with an "autocomplete-default" number equal to the total number of default engine searches you selected and an "autocomplete-other" number equal to the total number of non-default engine searches you selected.
(We aren't tracking the number of autocomplete selections you made which weren't searches.)
Comment on attachment 8458322 [details] [diff] [review]
bug1036914.diff

>+  _regexQuote: function(str) {
>+      return (str+'').replace(/([.?*+^$[\]\\(){}|-])/g, "\\$1");
>+  },

It would be pretty bad if str were something other than a string here. Judging by the call side it also seems impossible, so you should just write str.replace.

>+      let spec = this._regexQuote(engine.getSubmission("foo").uri.spec)
>+        .replace("foo", ".*")

What if some other random part of the URL contains foo?

>+        .replace("com", "[^\/]*")

What if some other random part of the URL contains com?

>+        .replace("http:\/\/", "(http:\/\/)?");

"http:\/\/" should probably be /^http:\/\//. Also, you don't seem to have https covered.

>+      if (engineName !== Services.search.currentEngine.name) {

nit: use !=, as the extra strictness of !== isn't helpful here.
(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8458322 [details] [diff] [review]
> bug1036914.diff
> >+  _regexQuote: function(str) {
> It would be pretty bad if str were something other than a string here.
> Judging by the call side it also seems impossible, so you should just write
> str.replace.

Good call.  Fixed!

> >+        .replace("com", "[^\/]*")
> What if some other random part of the URL contains com?

Hmm…  I could make that a little stricter, I suppose.
Having it match "\.com\/" would cover all the built-in search engines and duckduckgo.
And it only replaces the first occurrence, which is the one we're interested in.
Finally, see below.  ;)

> >+        .replace("foo", ".*")
> What if some other random part of the URL contains foo?

I could make this a little stricter, too, by searching for "=foo", or by adding a more unique string.
But, the risk here, and in the paragraph above, is that the data we get back would be a little dirty.
As far as I can tell, it would only happen in cases where people installed a strange search engine which happened to have ".com/" somewhere in the hostname and/or "=foo" somewhere before the keyword in the url.
I suspect there are sufficiently few people who have installed an extra search engine, much less one with those weird properties, for this to be an acceptable level of noise in the data.

To put it in context, I'm far more worried about us not handling Wikipedia search clicks (because their url is "http://en.wikipedia.org/wiki/Special:Search?search=foo&sourceid=Mozilla-search", but the page in the autocomplete box ends up as "http://en.wikipedia.org/wiki/Foo") and Amazon and eBay search clicks, for the same reason.

Still, if you feel that there's a better string I can use, I'll be more than happy to switch it out for something uniquer.  ;)

> >+        .replace("http:\/\/", "(http:\/\/)?");
> "http:\/\/" should probably be /^http:\/\//. Also, you don't seem to have
> https covered.

We only seem to strip the "http://" off the front of urls in the urlbar, so I don't need to cover https.

> >+      if (engineName !== Services.search.currentEngine.name) {
> nit: use !=, as the extra strictness of !== isn't helpful here.

Is the extra strictness harmful?  Coming from more strongly-typed languages, I prefer to go with the stricter version, where possible.

Thank you for the comments!
Iteration: 33.3 → 34.1
QA Contact: alexandra.lucinet
Comment on attachment 8458322 [details] [diff] [review]
bug1036914.diff

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

::: browser/modules/BrowserUITelemetry.jsm
@@ +177,5 @@
>                                           () => UITour.getTelemetry());
>  
>      Services.obs.addObserver(this, "sessionstore-windows-restored", false);
>      Services.obs.addObserver(this, "browser-delayed-startup-finished", false);
> +    Services.obs.addObserver(this, "autocomplete-did-enter-text", false);

where are you removing this observer?

@@ +190,5 @@
>        case "browser-delayed-startup-finished":
>          this._registerWindow(aSubject);
>          break;
> +      case "autocomplete-did-enter-text":
> +        if (aSubject.popup.selectedIndex != -1) {

for the sake of correctness, I'd QI aSubject

@@ +569,5 @@
> +  },
> +
> +  _logSearchResult: function (url) {
> +    // We selected something!!!  Get the url!!!
> +    let engines = Services.search.getEngines();

I'd probably use getVisibleEngines, do we want to log searches to removed engines too? (I don't know what's the purpose so this is an actual question)

I'd probably also make this function act asynchronously to ensure the searchservice is inizitialized, or we might fallback to its synchronous initialization path, and doesn't look like we need to be synchronous here. Might also be a good thing to do perf-wise, since delaying did-enter-text even just by 2ms (on some older systems I expect that to be more around 10ms), is still bad, we must try to keep autocomplete as fast as possible. so call SS.init() and act in its callback.

@@ +575,5 @@
> +    for (let engine of engines) {
> +      let spec = this._regexQuote(engine.getSubmission("foo").uri.spec)
> +        .replace("foo", ".*")
> +        .replace("com", "[^\/]*")
> +        .replace("http:\/\/", "(http:\/\/)?");

paolo is working on a new SS API that can parse an url back to search engine and terms in bug 1040721, I think here we should use that API instead of reinventing it, regardless stuff like this should live in the service since it might be useful more globally and we don't want to reinvent it every time.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +1242,5 @@
>      mSearchString = value;
>    }
>  
> +  input->GetTextValue(value);
> +  obsSvc->NotifyObservers(input, "autocomplete-did-enter-text", value.get());

The notification is already receiving input, so why do you need to get value here rather than just using input.textValue in the notification handler?
Attachment #8458322 - Flags: review?(mak77)
Depends on: 1040721
(A couple of quick replies inline.  There's definitely some work I need to do here, too.)

> @@ +575,5 @@
> > +    for (let engine of engines) {
> > +      let spec = this._regexQuote(engine.getSubmission("foo").uri.spec)
> > +        .replace("foo", ".*")
> > +        .replace("com", "[^\/]*")
> > +        .replace("http:\/\/", "(http:\/\/)?");
> paolo is working on a new SS API that can parse an url back to search engine
> and terms in bug 1040721, I think here we should use that API instead of
> reinventing it, regardless stuff like this should live in the service since
> it might be useful more globally and we don't want to reinvent it every time.

Yep, that's why we've logged bug 1036916, but I don't want this code to be blocked on that landing.
(I am really looking forward to using that API when it lands, though.  :)

> ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
> @@ +1242,5 @@
> > +  input->GetTextValue(value);
> > +  obsSvc->NotifyObservers(input, "autocomplete-did-enter-text", value.get());
> The notification is already receiving input, so why do you need to get value
> here rather than just using input.textValue in the notification handler?

I believe it was because by the time the notification got called, the input's text had changed from "define:foo" to "https://www.google.ca/search?q=define:foo&ie=utf-8&oe=utf-8&rls=org.mozilla:en-US:unofficial&client=firefox-nightly&channel=fflb&gws_rd=cr&ei=U8LWU6mGM5CgyAS69YGYCA", which isn't what I was searching for (or something similar).  But I'll double-check that, and if I can use input.textValue, I'll totally do that instead.

I'll fix up the rest of the things you mentioned, and post a new patch as soon as I can.

Thanks,
Blake.
(In reply to Blake Winton (:bwinton) from comment #8)
> I believe it was because by the time the notification got called, the
> input's text had changed from "define:foo" to
> "https://www.google.ca/search?q=define:foo&ie=utf-8&oe=utf-8&rls=org.mozilla:
> en-US:unofficial&client=firefox-
> nightly&channel=fflb&gws_rd=cr&ei=U8LWU6mGM5CgyAS69YGYCA", which isn't what
> I was searching for (or something similar).  But I'll double-check that, and
> if I can use input.textValue, I'll totally do that instead.

notifications are synchronous, so that'd be strange :/
(In reply to Blake Winton (:bwinton) from comment #8)
> > > +    for (let engine of engines) {
> > > +      let spec = this._regexQuote(engine.getSubmission("foo").uri.spec)
> > > +        .replace("foo", ".*")
> > > +        .replace("com", "[^\/]*")
> > > +        .replace("http:\/\/", "(http:\/\/)?");
> > paolo is working on a new SS API that can parse an url back to search engine
> > and terms in bug 1040721, I think here we should use that API instead of
> > reinventing it, regardless stuff like this should live in the service since
> > it might be useful more globally and we don't want to reinvent it every time.
> 
> Yep, that's why we've logged bug 1036916, but I don't want this code to be
> blocked on that landing.
> (I am really looking forward to using that API when it lands, though.  :)

Bug 1040721 will land soon - we should wait for it and avoid adding the manual parsing code quoted above.
Removed from Iteration 34.1 based on Blake's request since dependency 1040721 is not ready.
Status: ASSIGNED → NEW
Iteration: 34.1 → ---
Comment on attachment 8458322 [details] [diff] [review]
bug1036914.diff

Canceling review request until bug 1040721 lands and this change gets rebased.
Attachment #8458322 - Flags: review?(mconley)
Assignee: bwinton → nobody
QA Whiteboard: [qa+]
Flags: qe-verify+
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Iteration: --- → 35.1
QA Contact: alexandra.lucinet → andrei.vaida
Attached patch A more modern patch. (obsolete) — Splinter Review
Okay, this is a little better, I think.

I had added the value previously because I didn't have easy access to the urlbar from the callback (because I didn't have a window), but I needed to get one so that I could make sure we were only listening to the urlbar's autocomplete selections, so I reverted the changes to nsAutoCompleteController.cpp.

I'm also using the really nice new parseSubmissionURL method, which makes my life _so_ much easier.  :)

I think the "autocomplete-did-enter-text" observer should get removed in the same place as the two observers above it, but will defer to mconley on that.

I think I don't need to QI aSubject now, since I'm only comparing it to urlbar, and then using the urlbar for the rest of the method.

And I believe that's all the previous comments.  Thank you all for your patience!  :)
Attachment #8458322 - Attachment is obsolete: true
Attachment #8484494 - Flags: review?(mconley)
Attachment #8484494 - Flags: review?(mak77)
Comment on attachment 8484494 [details] [diff] [review]
A more modern patch.

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

::: browser/modules/BrowserUITelemetry.jsm
@@ +183,1 @@
>      CustomizableUI.addListener(this);

So indeed none of these is removed, nor the CustomizableUI listener is...

for the observers we could just use nsISupportsWeakReference, I'm not sure about the CustomizableUI listener though.
Eventually a .uninit() could be invoked from nsBrowserGlue::_onQuitApplicationGranted

Sounds like stuff for a follow-up.  Let's see what Mike thinks.

@@ +195,5 @@
> +        let win = RecentWindow.getMostRecentBrowserWindow({
> +          private: false,
> +          allowPopups: false,
> +        });
> +        if (win && win.document) {

This looks more complicated than needed, the notification already gives you everything you need.

I just made up this small scratchpad experiment to show you what I mean:

Services.obs.addObserver(function look(subject) {
  Services.obs.removeObserver(look, "autocomplete-did-enter-text", false);
  let input = subject.QueryInterface(Ci.nsIAutoCompleteInput);
  if (!input || input.id != "urlbar" || input.inPrivateContext ||
      !input.popupOpen || input.popup.selectedIndex == -1)
    return;
  alert(input.textValue);
}, "autocomplete-did-enter-text", false);
Attachment #8484494 - Flags: review?(mak77)
Comment on attachment 8484494 [details] [diff] [review]
A more modern patch.

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

::: browser/modules/BrowserUITelemetry.jsm
@@ +183,1 @@
>      CustomizableUI.addListener(this);

I'd be more worried if this function was running once per window or tab or something - but it's just run on browser initialization. Sure, we can add an uninit, but I agree that this is not a high priority right now. Might make a good mentored bug.

@@ +573,5 @@
>        this._countEvent(["search", "urlbar-keyword"]);
>      }
>    },
>  
> +  _logSearchResult: function (url) {

Maybe we should make this function name more specific:

_logUrlbarSearchResult, or _logAutocompleteSearchResult, or _logAwesomeBarSearchResult. Just to make it clear that this isn't logging data from the myriad of other search inputs that we have.

@@ +574,5 @@
>      }
>    },
>  
> +  _logSearchResult: function (url) {
> +    var spec = Services.search.parseSubmissionURL(url);

let not var
Attachment #8484494 - Flags: review?(mconley) → review+
Attached patch bug1036914.diffSplinter Review
Okay, everything fixed as per reviews.

Notes:
I went with _logAwesomeBarSearchResult, since that's what we're trying to do.
I'm not checking !input.popupOpen, since the allowPopups: false in the code that it's replacing was to ensure I wasn't getting popup windows, not that the input's popup wasn't open (which I don't think I care about).
And I went with a positive test with the code in the if, instead of a negative test with a return, because I like them better and it's only one line.  ;)

Finally, I've applied this patch after bug 1036912 (which is after bug 1036908).  I don't think there will be conflicts if it gets checked in before those patches, but I just wanted to make it explicit.
Attachment #8484494 - Attachment is obsolete: true
Attachment #8485230 - Flags: review+
Keywords: checkin-needed
To test this change:

Open about:telemetry in a new tab.
Expand the "Simple Measurements" section.
Look for the "UITelemetry" key.
Make sure it doesn't contain:
"search":{"autocomplete

Make sure the search box is using Google.
Click in the urlbar.
type "test"
Scroll down to a Google search result.
Hit enter.

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key now contains:
"search":{"autocomplete-default":1}

Go back to the previous page
Click in the urlbar.
type "test"
Scroll down to a regular (non-search) result.
Hit enter.

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key still contains:
"search":{"autocomplete-default":1}

Go back to the previous page
Click in the urlbar.
type "test"
Scroll down to a non-Google non-search result.
Hit enter.

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key now contains:
"search":{"autocomplete-default":1,"autocomplete-other":1}
sorry had to backout this changes (for Bug 1036914, Bug 1036912 and Bug 1036908) since one of this changes caused test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=47585310&tree=Fx-Team
Whiteboard: [fixed-in-fx-team]
Fixed by the latest patch in bug 1036912.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0fef0cd8390f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Verified fixed on Nightly 35.0a1 (2014-09-10) using Windows 7 64-bit. Testing was based on Comment 17.

Acceptance criteria for this patch:
- about:telemetry → Simple Measurements → UITelemetry key displays the following content after accessing different entries in the suggestions pane for the same keyword (i.e. default search engine entry, non-default search engine entry, regular/non-search entry):
> "search":{"autocomplete-other":1,"autocomplete-default":1}
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.