Closed Bug 1131274 Opened 9 years ago Closed 9 years ago

DuckDuckGo plugin should specify a searchform

Categories

(Firefox :: Search, defect, P4)

Product:
Firefox
Component:
Search
Type:
defect
Points:
1

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 41
Iteration:
41.1 - May 25
Tracking Flags:
Tracking Status
firefox41 --- verified

People

(Reporter: Gavin, Assigned: technozoom88, Mentored)

References

Details

(Whiteboard: [partnerengine][good first bug][fxsearch])

User Story

Instructions for fixing this bug:
- we need to add a |rel="searchform"| attribute to the primary <Url> element in the ddg.xml search plugin file located at browser/locales/en-US/searchplugins/ddg.xml. The primary <Url> is the one specified on line 9: https://hg.mozilla.org/mozilla-central/file/b94bcbc389e8/browser/locales/en-US/searchplugins/ddg.xml#l9 

- it should be consistent with other search plugins, e.g. https://hg.mozilla.org/mozilla-central/file/b94bcbc389e8/browser/locales/en-US/searchplugins/yahoo.xml#l19

Instructions for testing the fix:
- you'll need to rebuild the "browser" subdirectory using ./mach build browser

- once rebuilt, test by opening the build (./mach run), change your default search engine to DuckDuckGo in Firefox preferences, and then focus the search bar and press enter while it is empty.

Before the patch, those steps should result in "https://duckduckgo.com/" loading. After the patch, you should instead see "https://duckduckgo.com/?q=&t=ffsb" loaded.

Attachments

(1 file)

patch.diff
17.27 KB, patch
Gavin
: feedback+
Details | Diff | Splinter Review 
This is used when performing "empty string" searches. Right now we don't send the proper referral codes for these searches.
Mentor: gavin.sharp
Whiteboard: [good first bug]
Points: --- → 1
Flags: firefox-backlog+
Hi I would like to work on this :)
Attached patch patch.diff — — Splinter Review 
I hope this patch fixes this bug .
Attachment #8571256 - Flags: review?(gavin.sharp)
Attachment #8571256 - Flags: feedback?(gavin.sharp)
Thanks for the patch, Kapil! Have you been able to test it according to the instructions above?
Flags: needinfo?(technozoom88)
NO Gavin. I didn't test it. is the patch correct ?
Flags: needinfo?(technozoom88)
Comment on attachment 8571256 [details] [diff] [review]
patch.diff

The patch looks fine, but it's hard to know if it's right without testing it! :)

Give it a shot using the steps I describe above - let me know if you run into trouble or need further clarification.
Attachment #8571256 - Flags: review?(gavin.sharp)
Attachment #8571256 - Flags: feedback?(gavin.sharp)
Attachment #8571256 - Flags: feedback+
Hey Kapil, were you able to test this patch?
Flags: needinfo?(technozoom88)
Hi Gavin, if Kapil isnt testing the fix, I could do that and work on the bug?

Thanks

Jesal
Hi Gavin, just checked the patch written by Kapil, it doesn't specify with result domain, is that ok, as its specified in the template variable?

Thanks

Jesal
Flags: needinfo?(gavin.sharp)
Hi Gavin, just done a quick test and the extension is coming up as "q= ". strange.

Thanks Jesal
(In reply to Jesal from comment #10)
> Hi Gavin, just done a quick test and the extension is coming up as "q= ".
> strange.

That seems to match the expected behavior in the "User Story" field.

Can you copy/paste the exact URLs you get before and after the patch?
Flags: needinfo?(gavin.sharp)
Hi Gavin.

Before Patch: https://duckduckgo.com/?q=test&ia=meanings
After Patch: https://duckduckgo.com/?q=test&t=ffsb&ia=meanings

What are we looking for here, does that mean the patch is working?

Thanks

Jesal
Flags: needinfo?(gavin.sharp)
Ah, to properly test this you nee to compare the behavior for an "empty" search - so make sure there is no text entered into the search bar when you press enter.
Flags: needinfo?(gavin.sharp)
Hi Gavin,

Before Patch: https://duckduckgo.com/
After Patch: https://duckduckgo.com/?q=

I am guessing this is wrong, but not sure why?
Thanks

Jesal
Flags: needinfo?(gavin.sharp)
Hi Gavin, any update on this?

Many thanks

Jesal
Priority: -- → P4
Whiteboard: [good first bug] → [good first bug][fxsearch][partnerengine]
Truly sorry for the response delay, Jesal - I'm doing a very poor job mentoring here. Florian, could you help Jesal investigate?

The results you get in comment 14 definitely don't look right, I would expect what I laid out in the user story.
Flags: needinfo?(gavin.sharp)
Depends on: 1159717
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #16)
> Truly sorry for the response delay, Jesal - I'm doing a very poor job
> mentoring here. Florian, could you help Jesal investigate?
> 
> The results you get in comment 14 definitely don't look right, I would
> expect what I laid out in the user story.

What the patch here does is correct, the problem of not sending the parameter correctly in that case isn't specific to the DuckDuckGo plugin, I filed bug 1159717 to cover this.

There are 2 details that should be improved in the patch here:
- the path isn't correct, so the patch doesn't apply unless one goes to the browser/locales/ folder first.
- there are 2 spaces instead of one before rel="searchform"

Jesal, are you still interested in working on this? If so, could you please attach an updated patch fixing these 2 details? Thanks!
Rank: 45
Whiteboard: [good first bug][fxsearch][partnerengine] → [partnerengine][good first bug][fxsearch]
Mentor: gavin.sharp → florian
I checked-in Kapil's patch with the defaults I pointed out in comment 17 addressed.

Steps to verify:
- Click on the glass icon while the searchbox is empty.
- Click on the DuckDuckGo icon.

Expected result: The URL loaded should be https://duckduckgo.com/?q=&t=ffsb (or at least it should contain "t=ffsb")

Without the patch the URL was https://duckduckgo.com/

Note: This patch will only work if bug 1159717 is fixed (I checked-in both patches at once, so it shouldn't be a problem to verify.)
Assignee: nobody → technozoom88
Flags: needinfo?(technozoom88) → qe-verify+
https://hg.mozilla.org/mozilla-central/rev/9088e1544b1d
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Iteration: --- → 41.1 - May 25
Verified as fixed using Nightly 41.0a1 2015-05-25 under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: