925521 - Remove filter on recorded search engine identifiers for Desktop

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Mike Connor [:mconnor]]

See rationale in bug 925515 comment 0

Greg has already built this as an add-on [1], but we can dispense with the monkeypatching once this is ratified by the right people.

[1] https://hg.mozilla.org/users/gszorc_mozilla.com/fxaddon-healthreport-all-searches/

Comment 1

[Richard Newman [:rnewman]]

We should also avoid pre-registering fields in this case.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 1: correct behavior for installed engines. v1

E_TOO_MANY_BUGS.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 2: extract identifier dynamically. v1

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 1: correct behavior for installed engines. v2

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 818136 [details] [diff] [review]
Part 1: correct behavior for installed engines. v2

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

We conducted review on Phabricator. So the comments are preserved for posterity:

[gps]

We should run the decision of collapsing primary and after-market into the same namespace past the metrics people. I suspect they may want a way to easily distinguish between search engine providers we shipped and those installed by users.
Inline Comments
services/healthreport/providers.jsm
1295 ↗	(On Diff #10)	

What about search engines that are installed between FHR init and search recording? Those will get attributed to an other-* field initially and a proper name after restart. I initially thought we shouldn't care too much. But, that data will linger in the DB for months and seem odd.
Via Web · Fri, Oct 18, 9:43 AM · D8#2
rnewman commented on this revision.

I think that all aftermarket engines will be missing an identifier…
Inline Comments
services/healthreport/providers.jsm
1295 ↗	(On Diff #10)	

I don't think that's true.

Either the added engine has an identifier, or it doesn't, and that's true whether it's installed before or after init.

If it does, we'll hit line 1290 regardless. If it doesn't, there would be no entry in nameMappings anyway.

(What this tells me is that nameMappings is no longer necessary at all…)
Via Web · Fri, Oct 18, 9:51 AM · D8#3
gps requested changes to this revision.
Inline Comments
services/healthreport/providers.jsm
1295 ↗	(On Diff #10)	

So I'm going to punt this back to you for revision then.
Via Conduit · Fri, Oct 18, 9:58 AM · D8#4
rnewman updated this revision to Diff #11.

Bug 925521 - Part 2: correctly record identifiers for non-pre-installed engines. r=gps

Remove name mappings. Tests still pass.
Via Web · Fri, Oct 18, 10:05 AM · D8#5
gps accepted this revision.

Looks good! Loving Phabricator's interdiff display!

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 818122 [details] [diff] [review]
Part 2: extract identifier dynamically. v1

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

http://phabricator.gregoryszorc.com/D8

Comment 7

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/c0ac5c3fd460
https://hg.mozilla.org/services/services-central/rev/ff2d338d15ad

Comment 8

[Richard Newman [:rnewman]]

For the record: not merging s-c until we get a final decision re council approval.

Comment 9

[Richard Newman [:rnewman]]

Follow-up for bc failure.

https://hg.mozilla.org/services/services-central/rev/b000f9dbd242

Comment 10

[Richard Newman [:rnewman]]

Waiting on merge (and landing for Fennec part).

Comment 11

[Richard Newman [:rnewman]]

John clarified via email that this feature is approved to land.

Comment 12

[Richard Newman [:rnewman]]

This and Bug 925517 will be merged into Nightly 28 later this week.

Comment 13

[Richard Newman [:rnewman]]

Follow-up for bustage:

https://hg.mozilla.org/services/services-central/rev/ed2343a4ad52

Comment 14

[Richard Newman [:rnewman]]

Attachment: Follow-up: adjust loadSearch to support recording from context searches.

Noticed the bc test failure was a little deeper. So now we track context searches correctly!

Comment 15

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/51c553b11e62

Comment 16

[Richard Newman [:rnewman]]

Goddamnit. We need a precommit hook that checks for outdated version numbers in tests.

https://hg.mozilla.org/services/services-central/rev/cdfe0db3e762

Comment 17

[Gregory Szorc [:gps]]

I wonder if we could move some of those mochitests into /services/healthreport...

Comment 18

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #17)
> I wonder if we could move some of those mochitests into
> /services/healthreport...

That would sure reduce the number of follow-ups I have to push every time I touch this code! :D

Comment 19

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/c0ac5c3fd460
https://hg.mozilla.org/mozilla-central/rev/ff2d338d15ad
https://hg.mozilla.org/mozilla-central/rev/b000f9dbd242
https://hg.mozilla.org/mozilla-central/rev/ed2343a4ad52
https://hg.mozilla.org/mozilla-central/rev/51c553b11e62
https://hg.mozilla.org/mozilla-central/rev/cdfe0db3e762

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 818136 [details] [diff] [review]
Part 1: correct behavior for installed engines. v2

[Approval Request Comment]

** Three-patch series **

Bug caused by (feature/regressing bug #): 
  None.

User impact if declined: 
  Another release in which we're unable to address search engine hijacking using FHR data (Bug 925527). See Bug 925515 Comment 6.

Testing completed (on m-c, etc.): 
  Has been baking for a while.

Risk to taking this patch (and alternatives if risky): 
  Pretty slim.

String or IDL/UUID changes made by this patch:
  None.

Comment 21

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/releases/mozilla-aurora/rev/aa8e43f9ec74
   https://hg.mozilla.org/releases/mozilla-aurora/rev/d2a2cfcd70fe
   https://hg.mozilla.org/releases/mozilla-aurora/rev/a018b8412692
   https://hg.mozilla.org/releases/mozilla-aurora/rev/d1d7b9c9fa2e
   https://hg.mozilla.org/releases/mozilla-aurora/rev/a4a7cd7883cc
   https://hg.mozilla.org/releases/mozilla-aurora/rev/cbd566346605

Comment 22

[Ioana (away)]

Verified as fixed on Firefox 27.0b1 and the 12/12 Firefox 28.0a2 - Windows 7 64-bit, Ubuntu 13.04 32-bit, Mac OS X 10.7.5.

I installed and used several engines on each OS. Here's a sample of how this looks:
"        ""org.mozilla.searches.counts"": {
          ""_v"": 3,
          ""eBay.searchbar"": 2,
          ""google.searchbar"": 1,
          ""google.urlbar"": 1,
          ""other-Ask.com.abouthome"": 3,
          ""other-Ask.com.contextmenu"": 1,
          ""other-Ask.com.searchbar"": 1,
          ""other-DuckDuckGo.abouthome"": 1,
          ""other-DuckDuckGo.searchbar"": 2,
          ""other-DuckDuckGo.urlbar"": 1,
          ""other-Makepolo Search.abouthome"": 1,
          ""other-Makepolo Search.contextmenu"": 2,
          ""other-Makepolo Search.searchbar"": 2,
          ""other-iLoveVitaly.searchbar"": 10,
          ""other-iLoveVitaly.urlbar"": 2,
          ""yahoo.searchbar"": 3
        },"