975528 - Searches using Bing are missing codes for search bar, about:home, and context menu

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Mike Connor [:mconnor]]

This was sort of working prior to May 2013, in that we passed MOZSBR for everything except for location bar searches, using URL type instead of explicit purpose.  This wasn't technically correct, but was somewhat "close enough" for tracking.

As of May 2013, due to changes in bug 587780, this stopped working for about:home and context menu searches.  The reason is that the search plugin set MOZSBR in cases where there was no purpose argument passed to the service, and those access points began passing a purpose at that time.  When we added a purpose to the search bar in bug 925892, we exacerbated the bug.  pc=MOZI is still being passed correctly, so we'll sort out the crediting issue, but this should get fixed correctly.

The correct resolution is to actually explicitly provide form codes to match purposes:

search bar: MOZSBR
about:home: MOZSPG
context menu: (TBD, with MOZSBR as a placeholder)

Comment 1

[Mike Connor [:mconnor]]

Attachment: bingCodeFix

Comment 2

[Mike Connor [:mconnor]]

We're going to need this on branches.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Can you add a bing test to match the google ones we have? Should be copy-paste-tweak.

Comment 4

[Mike Connor [:mconnor]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/da138438d584
aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/f6ed13594ecd
beta: https://hg.mozilla.org/releases/mozilla-beta/rev/fbad897c38bf

still needs a test, leaving open until that's sorted.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

marking fixed on branches, it can be updated for FF30 when it lands on trunk.

Comment 6

[u279076]

Flagging for in-testsuite as per comment 4.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/fbad897c38bf

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da138438d584

Comment 9

[Drew Willcoxon :adw]

Attachment: test patch

I think this is right.  Matt, would you mind reviewing since you wrote the Google test in bug 587780?  This is largely copied from that test.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8385808 [details] [diff] [review]
test patch

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

::: browser/components/search/test/browser_bing.js
@@ +48,5 @@
> +    description: "Bing. Search by Microsoft.",
> +    searchForm: "http://www.bing.com/search",
> +    type: Ci.nsISearchEngine.TYPE_MOZSEARCH,
> +    hidden: false,
> +    wrappedJSObject: {

Would you mind adding queryCharset: "UTF-8" here and to Google while you're touching them?

::: browser/components/search/test/browser_google_behavior.js
@@ +35,3 @@
>  function test() {
>    let engine = Services.search.getEngineByName("Google");
>    ok(engine, "Google is installed");

I suppose we could also copy this test for Bing too but it seems like overkill. Perhaps others disagree.

Comment 11

[Mike Connor [:mconnor]]

This test also should test for pc=MOZI when official branding gets set.  That's the code that makes sure we get full credit.

Comment 12

[Drew Willcoxon :adw]

It does, by checking the browser.search.param.ms-pc pref.  Should it use an #ifdef MOZ_OFFICIAL_BRANDING instead?

Comment 13

[Mike Connor [:mconnor]]

I'd prefer that, since the test should really ensure that the value is correct, not just that the plugin is lifting the pref correctly.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Right, testing that the pref makes it through the search service correctly is important coverage.

The _behavior test is also important, because it tests that actual searches as triggered by the UI are correct (as opposed to just testing the search service directly), which also adds coverage. We should get that one copied too!

Comment 15

[Drew Willcoxon :adw]

Attachment: test patch v2 (and Beta 29 patch)

This uses #ifdef MOZ_OFFICIAL_BRANDING instead of getting the browser.search.param.ms-pc pref directly, adds browser_bing_behavior.js, and checks for queryCharset.  Requesting review again to be safe.

I was having trouble getting the build system to pick up these two new tests now that they're preprocessed.  The problem was that browser/components/search/moz.build doesn't add the test directory to TEST_DIRS, so the Makefile.in in the test directory wasn't being picked up.  (And once you do that, you also need a moz.build in the test directory.)  I talked to gps about it to make sure.

That means that since browser_google.js and browser_google_behavior.js are also preprocessed, they have not been running.  You can look at recent log files on m-c, for example, to see that they aren't there.  (Non-preprocessed files in the test directory are there.)  How long have they not been running?  Since bug 924463 -- October 2013 -- it looks like.  Before that, moz.build correctly set TEST_DIRS.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Good catch! Losing that test coverage is bad :( I see you pinpointed bug 924463 as the cause, maybe we should compare test logs from before/after that patch to audit for other such cases...

Comment 17

[Drew Willcoxon :adw]

Attachment: test patch v3

https://hg.mozilla.org/integration/fx-team/rev/f2839d31d72d

Bug 968403 landed on Friday and changed how preprocessed mochitests work, so I had to change the patch.  I didn't realize that until I tried to land after Gavin's r+ yesterday.  The changes were small, so I didn't ask for review yet again.

Here's a try run from before those changes: https://tbpl.mozilla.org/?tree=Try&rev=8683046aaf58

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f2839d31d72d

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Drew, can we uplift the test patch as well? I assume we'll need something more like the "test patch v2" given bug 968403 isn't on Aurora/Beta.

Comment 20

[Drew Willcoxon :adw]

Comment on attachment 8386500 [details] [diff] [review]
test patch v2 (and Beta 29 patch)

Sure.  test patch v2 applies cleanly to Aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
This is a test patch for the original patch in this bug that already landed on Aurora.

User impact if declined:
None

Testing completed (on m-c, etc.):
Two browser-chrome tests on m-c since 3/13

Risk to taking this patch (and alternatives if risky):
Since this patch consists of only tests, the only risk is that they'll break on Aurora.

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

Comment 21

[Drew Willcoxon :adw]

Attachment: test patch for Beta (28)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
This is a test patch for the original patch in this bug that already landed on Beta.

User impact if declined:
None

Testing completed (on m-c, etc.):
Two browser-chrome tests on m-c since 3/13

Risk to taking this patch (and alternatives if risky):
Since this patch consists of only tests, the only risk is that they'll break on Beta.

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

Comment 22

[:Gavin Sharp [email: [e-mail]]]

We're going to miss the 28 train, but that's OK. Let's just make sure to get the test on 29.

Comment 23

[Drew Willcoxon :adw]

I tried to land, but apparently 30 has already moved to Aurora because the patch doesn't apply anymore.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8386500 [details] [diff] [review]
test patch v2 (and Beta 29 patch)

Just need this patch on beta now, if I'm understanding this stuff all correctly.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

in testing on try server (inbound based), I see this error on linux and linux-debug (https://tbpl.mozilla.org/php/getParsedLog.php?id=36414002&tree=Try#error0):
uncaught exception - NotFoundError: The operation failed because the requested database object could not be found. For example, an object store did not exist but was being opened. at chrome://browser/content/abouthome/aboutHome.js:252

I am changing the way I run tests so that I run a single directory at a time, then close the entire browser session and create a new profile.  is there a chance that browser_google_behavior.js depends on a previous test case to set a state?

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Joel: that sounds like another manifestation of bug 972684.

Comment 27

[Drew Willcoxon :adw]

I think Gavin is right:

(In reply to Marco Bonardo [:mak] from bug 968597 comment #4)
> the aborts are expected, when you close the tab there might be an indexedDB
> transaction ongoing and it gets aborted

The test loads about:home and then immediately does a search from it, which replaces about:home with a Google search results page.  (Or starts to replace it anyway, since the Google load is immediately canceled.)

I guess browser_google_behavior.js doesn't always fail when run normally because by the point it runs, about:home's gSnippetsMap is set up, so the path where the exception is thrown isn't hit?  Your setup changes must have changed that.

Sure seems like browser_google_behavior.js shouldn't have to worry about this, which means that about:home ought to handle this exception internally.

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

yes, it looks to be related to that bug, thanks for pointing it out Gavin.  Is somebody working on this?  For my efforts to run browser chrome per directory this test currently needs to be disabled on all platforms for opt and debug.

Comment 29

[Drew Willcoxon :adw]

I don't think anybody's working on it.  You should probably file a bug and mark it blocking the desktop backlog triage bug 981530 so Gavin and the other managers can prioritize it.