Closed Bug 1131274 Opened 5 years ago Closed 5 years ago
Duck Go plugin should specify a searchform
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.
This is used when performing "empty string" searches. Right now we don't send the proper referral codes for these searches.
Whiteboard: [good first bug]
Points: --- → 1
Hi I would like to work on this :)
I hope this patch fixes this bug .
Thanks for the patch, Kapil! Have you been able to test it according to the instructions above?
NO Gavin. I didn't test it. is the patch correct ?
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.
Hey Kapil, were you able to test this patch?
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
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?
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
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.
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
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.
(In reply to :Gavin Sharp [email: firstname.lastname@example.org] 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!
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+
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.
You need to log in before you can comment on or make changes to this bug.