557665 - Allow specifying SearchForm as a normal <Url> in engine description files

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

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

This will make it possible to use special params (MozParams) and such.

I propose using rel="searchform" to differentiate. Or perhaps we should use something more moz-specific to avoid conflicts with "registered" rel values?

Comment 1

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

Attachment: patch

Comment 2

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

Attachment: patch

This actually still applies, apart from the google.xml change. I've updated it and tweaked the logic a bit (filter out <Url rel="searchform"> if they are type="POST").

Comment 3

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

Would need to add a test, though.

I suppose the original motivation for this was bug 557890 comment 1 (adding the "client" param to the searchform URL). I don't know whether we still want to do that, though.

Comment 4

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

I guess we'll WONTFIX until there's an actual need for this.

Comment 5

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

Bug 959576 is likely going to need this.

Comment 6

[u279076]

I don't think this needs any special QA attention. Please needinfo me if you think that's a mistake.

Comment 7

[Drew Willcoxon :adw]

Attachment: unbitrotted patch

Looks good to me.

Comment 8

[Drew Willcoxon :adw]

Attachment: tests

This adds two tests, one for ensuring <Url rel="searchform" method="GET"/> is recognized, and one for ensuring <Url rel="searchform" method="POST"/> falls back to <SearchForm/>.  It also updates browser_google.js since there are now three <Url>s in google.xml, not two.

Comment 9

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

Comment on attachment 8396085 [details] [diff] [review]
tests

I would prefer simplifying the two test engines you're adding to the bare minimum needed to ensure relevant test coverage, rather than just copying the Google engine. Having that complicated of an engine for a specific test is kind of confusing.

For test_rel_searchform.js, engine-rel-searchform.xml should probably have a searchForm value that differs more significantly from its text/html URI's template hostname (right now the only difference is http://www.google.com/ vs. https://www.google.com, which seems accidental), to make the test clearer, and to avoid us accidentally losing test coverage if they are later updated to be the same somehow. I would use e.g. "http://testurl.com/searchform" or somesuch as the searchForm URL. Using more unique values in the test engines in general is a good idea (e.g. "http://testurl.com/POST_searchform" rather than "bogus.com", etc.).

It's also probably best to combine both of those tests into one, to avoid the overhead of test setup for two tests of the same feature. The one test can load both engines and check them.

The do_load_manifest calls aren't necessary in these tests since they don't use JAR:-loaded engines.

Comment 10

[Drew Willcoxon :adw]

Attachment: tests v2

Comment 11

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

Comment on attachment 8396103 [details] [diff] [review]
tests v2

Hmm, now it's a bit too concise - hard to tell what's being tested exactly. Maybe just add a comment in the test mentioning what two things you're checking?

Actually, the engine-rel-searchform.xml test is not ideal. Because we fall back to the prePath of the default URL's template in the searchForm getter, I think the test would still pass even if we somehow break the "rel="searchform"" check there? Probably best to add a parameter to the URL, and check that it is correctly passed when used as a searchForm.

Comment 12

[Drew Willcoxon :adw]

Attachment: test v3

Comment 13

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

Comment on attachment 8396111 [details] [diff] [review]
test v3

Thanks!

Comment 14

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/3f384a4d845a
https://hg.mozilla.org/integration/fx-team/rev/9301d710a707

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f384a4d845a
https://hg.mozilla.org/mozilla-central/rev/9301d710a707