724116 - add additional URL types to implement "channel" parameter for Google plugin

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

To implement the "channel" parameter changes from bug 722352 in the long term, we'd like to depend on bug 587780. However, since we'd like to make the addition of the "channel" parameter in Firefox 12, which is already on Aurora, we'll want a less invasive change in the interim. We can use the "magical URL type" trick from bug 596439 to add the channel parameter without search service changes.

Comment 1

[Matthew N. [:MattN]]

Attachment: v.1 Add loadContextMenuSearch and google-params.inc

Should I add Google specific tests?

Comment 2

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

Comment on attachment 594864 [details] [diff] [review]
v.1 Add loadContextMenuSearch and google-params.inc

Seems like it would be simpler to just have loadSearch do the fallback itself if getSubmission fails and responseType != null, that way you don't need to add loadContextMenuSearch (and loadSearch doesn't need to return a submission object, which is a bit of an odd way to indicate "success").

Otherwise this looks good to me. Factoring the common parameters into an include like that is a good idea.

Comment 3

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

You can write a test that installs a generic search plugin that specifies the multiple magical URL types, and then confirms that they're used correctly when searches are triggered from the different search points (see e.g. browser_426329.js).

Comment 4

[Matthew N. [:MattN]]

Attachment: v.2 Have loadSearch fallback to text/html and add more tests

Comment 5

[Matthew N. [:MattN]]

Attachment: v.2.1 Have loadSearch fallback to text/html and add more tests

Oops, I was missing the test file I added.

Comment 6

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

Comment on attachment 595578 [details] [diff] [review]
v.2.1 Have loadSearch fallback to text/html and add more tests

Sorry it took me a while to get to this!

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

I'm not sure this test is that useful - it kind of relies on the default engine having these parameters, and we're going to get rid of this eventually, so I would just omit this change.

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

This test doesn't really seen to be adding much additional coverage - was the idea to just test loading an engine that has the extra URL types? That's probably better suited to an xpcshell test like those being added in bug 458810. I think you can omit this change.

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

Should have a license header here (https://www.mozilla.org/MPL/headers/).

I'm a little worried that the selection and popup interactions (and related executeSoon calls) will lead to test flakiness (particularly cross platform). Keep an eye on it, we can deal if it ends up being a problem!

>+  function doOnloadOnce(callback) {
>+    gBrowser.addEventListener("DOMContentLoaded", function(event) {
>+      gBrowser.removeEventListener("DOMContentLoaded", arguments.callee, true);

I know you probably just copied this, but for new tests, given the anonymous function a name, and use that to remove it instead of arguments.callee (which might be going away at some point).

>\ No newline at end of file

nit: fix this :)

Comment 7

[Matthew N. [:MattN]]

Attachment: v.3 For checkin. Removed 2 test changes and address other comments.

(In reply to Gavin Sharp (use [e-mail] for email) from comment #6)
> Comment on attachment 595578 [details] [diff] [review]
> v.2.1 Have loadSearch fallback to text/html and add more tests
> 
> Sorry it took me a while to get to this!
> 
> >diff --git a/browser/base/content/test/browser_contextSearchTabPosition.js b/browser/base/content/test/browser_contextSearchTabPosition.js
> 
> I'm not sure this test is that useful - it kind of relies on the default
> engine having these parameters, and we're going to get rid of this
> eventually, so I would just omit this change.

It would also test the fallback case if the default engine didn't support the search type but I removed it anyways.

> 
> >diff --git a/browser/components/search/test/browser_addEngine.js b/browser/components/search/test/browser_addEngine.js
> 
> This test doesn't really seen to be adding much additional coverage - was
> the idea to just test loading an engine that has the extra URL types? That's
> probably better suited to an xpcshell test like those being added in bug
> 458810. I think you can omit this change.

Yes, and I agree it would be better as an xpcshell test but since this is where existing tests for adding an engine are I though it made sense to improve them.  I removed this though.

> >diff --git a/browser/components/search/test/browser_contextmenu.js b/browser/components/search/test/browser_contextmenu.js
> 
> Should have a license header here (https://www.mozilla.org/MPL/headers/).

I already had a PD header there.
 
> I'm a little worried that the selection and popup interactions (and related
> executeSoon calls) will lead to test flakiness (particularly cross
> platform). Keep an eye on it, we can deal if it ends up being a problem!
> 
> >+  function doOnloadOnce(callback) {
> >+    gBrowser.addEventListener("DOMContentLoaded", function(event) {
> >+      gBrowser.removeEventListener("DOMContentLoaded", arguments.callee, true);
> 
> I know you probably just copied this, but for new tests, given the anonymous
> function a name, and use that to remove it instead of arguments.callee
> (which might be going away at some point).
> 
> >\ No newline at end of file
> 
> nit: fix this :)

fixed

Comment 8

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94cd5a4d7a10

Try run: https://tbpl.mozilla.org/?tree=Try&rev=793f3da69232

(In reply to Gavin Sharp (use [e-mail] for email) from comment #6)
> was the idea to just test loading an engine that has the extra URL types?

It was also because we have 3 supported engines and that file was only testing the other 2.

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/94cd5a4d7a10

Comment 10

[Axel Hecht [:Pike]]

Can we back this out, please?

The l10n infrastructure isn't set up for this, this needs at least a fix to browser/locales/filters.py to ignore the new file.

Also, did anyone check what happens if a localization adds a different param snippet?

Comment 11

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

(In reply to Axel Hecht [:Pike] from comment #10)
> Can we back this out, please?
> 
> The l10n infrastructure isn't set up for this, this needs at least a fix to
> browser/locales/filters.py to ignore the new file.

Seems like it would be easier to just move the .inc out of browser/locales/en-US/searchplugins.

> Also, did anyone check what happens if a localization adds a different param
> snippet?

What do you mean by "param snippet"?

Comment 12

[Axel Hecht [:Pike]]

google-params.inc is the snippet.

How should we implement the corresponding changes for folks that have specific localized versions of the google plugin? Would there be a good proposed location for in inc? Should folks re-use the inc, is that properly accessible for a repack?

Really, I'd like to solve this without having the code in central.

Comment 13

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

(In reply to Axel Hecht [:Pike] from comment #12)
> How should we implement the corresponding changes for folks that have
> specific localized versions of the google plugin? Would there be a good
> proposed location for in inc? Should folks re-use the inc, is that properly
> accessible for a repack?

These seem like good issues to deal with case-by-case, in followup bugs. There's no pressing need to make changes to other locales with Google engines.

An easier move might be to just move from using a .inc to using a #define.

Comment 14

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

Attachment: followup fix for l10n issue

Comment 15

[Axel Hecht [:Pike]]

Comment on attachment 597470 [details] [diff] [review]
followup fix for l10n issue

That's a nifty idea, thanks.

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 597470 [details] [diff] [review]
followup fix for l10n issue

do.not.do.two.review.in.parallel.

Comment 17

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

Went ahead and landed that to fix l10n, after-the-fact review would still be appreciated!
https://hg.mozilla.org/mozilla-central/rev/ee85dca9fe03

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 597470 [details] [diff] [review]
followup fix for l10n issue

(In reply to Axel Hecht [:Pike] from comment #10)
> The l10n infrastructure isn't set up for this, this needs at least a fix to
> browser/locales/filters.py to ignore the new file.

I did take a look at it but saw it was already filtering for only .xml files in the searchplugins directory so it looked like it would handle this change.  To be honest, I couldn't figure out what the script was used for because the file & function name are not very descriptive and there is no comment explaining the purpose.  It also wasn't clear whether this should be run on the pre-processed version or not.

> Also, did anyone check what happens if a localization adds a different param
> snippet?

I didn't test it but it should just work if the localization has their own google.xml since the include is relative.  I guess it may be a problem if only the .inc is changed for a locale.

Comment 19

[Axel Hecht [:Pike]]

Yeah, filter.py is sub-optimal, we can make it a lot more digestible. filed bug 727591 on that. The tests are run on the source, so the non-processed files, fwiw.

Thanks for the quick follow-up.

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 596891 [details] [diff] [review]
v.3 For checkin. Removed 2 test changes and address other comments.

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: N/A
Testing completed (on m-c, etc.): m-c since 2012-02-15
Risk to taking this patch (and alternatives if risky): Potential for L10N automation problems. 
String changes made by this patch: None

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 597470 [details] [diff] [review]
followup fix for l10n issue

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: N/A
Testing completed (on m-c, etc.): m-c since 2012-02-15
Risk to taking this patch (and alternatives if risky): Potential for L10N automation problems. 
String changes made by this patch: None

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 596891 [details] [diff] [review]
v.3 For checkin. Removed 2 test changes and address other comments.

[Triage Comments]
Approved for Aurora 12.

Comment 23

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b259413df9d9
https://hg.mozilla.org/releases/mozilla-aurora/rev/643e4dd65350