The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 12

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: MattN)

Tracking

Trunk
Firefox 13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 3 obsolete attachments)

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.
Blocks: 722352
Assignee: nobody → mnoorenberghe+bmo
Depends on: 587719
Created attachment 594864 [details] [diff] [review]
v.1 Add loadContextMenuSearch and google-params.inc

Should I add Google specific tests?
Attachment #594864 - Flags: review?(gavin.sharp)
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.
Attachment #594864 - Flags: review?(gavin.sharp) → feedback+
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).
Created attachment 595309 [details] [diff] [review]
v.2 Have loadSearch fallback to text/html and add more tests
Attachment #594864 - Attachment is obsolete: true
Attachment #595309 - Flags: review?(gavin.sharp)
tracking-firefox12: --- → ?
Created attachment 595578 [details] [diff] [review]
v.2.1 Have loadSearch fallback to text/html and add more tests

Oops, I was missing the test file I added.
Attachment #595309 - Attachment is obsolete: true
Attachment #595309 - Flags: review?(gavin.sharp)
Attachment #595578 - Flags: review?(gavin.sharp)

Updated

5 years ago
tracking-firefox12: ? → +
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 :)
Attachment #595578 - Flags: review?(gavin.sharp) → review+
Created attachment 596891 [details] [diff] [review]
v.3 For checkin. Removed 2 test changes and address other comments.

(In reply to Gavin Sharp (use gavin@gavinsharp.com 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
Attachment #595578 - Attachment is obsolete: true
Attachment #596891 - Flags: review+
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 gavin@gavinsharp.com 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.
Status: NEW → ASSIGNED
Flags: in-testsuite+
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/94cd5a4d7a10
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 10

5 years ago
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?
(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

5 years ago
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.
(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.
Created attachment 597470 [details] [diff] [review]
followup fix for l10n issue
Attachment #597470 - Flags: review?(mnoorenberghe+bmo)

Comment 15

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

That's a nifty idea, thanks.
Attachment #597470 - Flags: feedback-

Comment 16

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

do.not.do.two.review.in.parallel.
Attachment #597470 - Flags: feedback- → feedback+
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 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.
Attachment #597470 - Flags: review?(mnoorenberghe+bmo) → review+

Comment 19

5 years ago
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 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
Attachment #596891 - Flags: approval-mozilla-aurora?
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
Attachment #597470 - Flags: approval-mozilla-aurora?
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.
Attachment #596891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #597470 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b259413df9d9
https://hg.mozilla.org/releases/mozilla-aurora/rev/643e4dd65350
status-firefox12: --- → fixed
Blocks: 596439
Blocks: 724091
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.