Closed Bug 557665 Opened 10 years ago Closed 6 years ago

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

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: Gavin, Assigned: adw)

References

Details

(Whiteboard: p=3 s=it-31c-30a-29b.1 [qa-])

Attachments

(2 files, 4 obsolete files)

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?
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #437643 - Flags: review?(rflint)
Attached patch patch (obsolete) — Splinter Review
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").
Attachment #437643 - Attachment is obsolete: true
Attachment #437643 - Flags: review?(rflint)
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.
Assignee: gavin.sharp → nobody
I guess we'll WONTFIX until there's an actual need for this.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Bug 959576 is likely going to need this.
Status: REOPENED → NEW
Whiteboard: p=0
Status: NEW → ASSIGNED
Whiteboard: p=0 → p=3 s=it-31c-30a-29b.1
Assignee: nobody → adw
I don't think this needs any special QA attention. Please needinfo me if you think that's a mistake.
Whiteboard: p=3 s=it-31c-30a-29b.1 → p=3 s=it-31c-30a-29b.1 [qa-]
Looks good to me.
Attachment #721564 - Attachment is obsolete: true
Attachment #8396083 - Flags: review+
Attached patch tests (obsolete) — Splinter Review
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.
Attachment #8396085 - Flags: review?(gavin.sharp)
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.
Attachment #8396085 - Flags: review?(gavin.sharp) → feedback+
Attached patch tests v2 (obsolete) — Splinter Review
Attachment #8396085 - Attachment is obsolete: true
Attachment #8396103 - Flags: review?(gavin.sharp)
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.
Attachment #8396103 - Flags: review?(gavin.sharp) → review-
Attached patch test v3Splinter Review
Attachment #8396103 - Attachment is obsolete: true
Attachment #8396111 - Flags: review?(gavin.sharp)
Comment on attachment 8396111 [details] [diff] [review]
test v3

Thanks!
Attachment #8396111 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/3f384a4d845a
https://hg.mozilla.org/mozilla-central/rev/9301d710a707
Status: ASSIGNED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Status: RESOLVED → VERIFIED
Blocks: 990799
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Blocks: 1039004
Blocks: 1039003
You need to log in before you can comment on or make changes to this bug.