Closed Bug 1373718 Opened 8 years ago Closed 8 years ago

Update follow-on search add-on to 0.9.1 - fix double reporting and fix case sensitivity

Categories

(Firefox :: Search, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox55 --- verified
firefox56 --- verified

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

QA found a couple of issues when testing the follow-on search add-on: - Bing sometimes uses lower case in their search queries, not just upper-case - Occasionally, we can get duplicate reports due to the scripts somehow picking up reporting for different windows. These have already been reviewed & landed in the github repo, so here's 0.9.1.
Comment on attachment 8878569 [details] Bug 1373718 - Update follow-on search add-on to 0.9.1 - fix double reporting and fix case sensitivity. https://reviewboard.mozilla.org/r/149894/#review154570 r=me with the concern below addressed. ::: browser/extensions/followonsearch/content/followonsearch-fs.js:273 (Diff revision 1) > > addEventListener("DOMContentLoaded", onPageLoad, false); > docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebProgress) > .addProgressListener(webProgressListener, Ci.nsIWebProgress.NOTIFY_LOCATION); > > +gOriginalWindow = content; Shouldn't we nullify gOriginalWindow when we set gDisabled to true? Doesn't this cause leaks?
Attachment #8878569 - Flags: review?(past) → review+
Comment on attachment 8878569 [details] Bug 1373718 - Update follow-on search add-on to 0.9.1 - fix double reporting and fix case sensitivity. https://reviewboard.mozilla.org/r/149894/#review154570 > Shouldn't we nullify gOriginalWindow when we set gDisabled to true? Doesn't this cause leaks? I went through the various scenarios, and I'm pretty confident that there isn't a leak issue here, since the frame script will be kept alive anyway due to the tab being open, and if that's the case, there will be a content object anyway.
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8f1c1dc1725c Update follow-on search add-on to 0.9.1 - fix double reporting and fix case sensitivity. r=past
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Reminder to request uplift soon.
Flags: needinfo?(standard8)
Comment on attachment 8878569 [details] Bug 1373718 - Update follow-on search add-on to 0.9.1 - fix double reporting and fix case sensitivity. Approval Request Comment [Feature/Bug causing the regression]: Follow-on search telemetry [User impact if declined]: This is to improve some of the telemetry gathering we are doing on follow-on search to better understand the behaviour of our users. [Is this code covered by automated tests?]: Not yet, manual testing (bug 1371294) [Has the fix been verified in Nightly?]: Manually tested with an add-on of the same version. [Needs manual test from QE? If yes, steps to reproduce]: yes [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Small fixes to stand-alone code. [String changes made/needed]: None
Flags: needinfo?(standard8)
Attachment #8878569 - Flags: approval-mozilla-beta?
Comment on attachment 8878569 [details] Bug 1373718 - Update follow-on search add-on to 0.9.1 - fix double reporting and fix case sensitivity. followonsearch add-on bug fixes, beta55+
Attachment #8878569 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1375564
Flags: qe-verify+
Mark, could you please tell me what exactly I should look for when verifying this? Are there any metrics I should be aware of?
(In reply to Camelia Badau, QA [:cbadau] from comment #10) > Mark, could you please tell me what exactly I should look for when verifying > this? Are there any metrics I should be aware of? This has already shipped to FF 54 users via a Go Faster update - see bug 1375564 - the best person to ask would be Justin as he did the testing there.
Thanks, Mark! Justin, could you please tell me what exactly I should look for when verifying this?
Flags: needinfo?(jwilliams)
Hello Camelia, You can find the test cases here. Everything is fairly straight forward. https://testrail.stage.mozaws.net/index.php?/suites/view/1046&group_by=cases:section_id&group_order=asc
Flags: needinfo?(jwilliams) → needinfo?(camelia.badau)
I've tested using some test cases from Justin (see comment 13) on Windows 7 x64 and Mac OSX 10.12 using Firefox 55.0.1 and Firefox 56 Beta 2 and everything seems to look good.
Status: RESOLVED → VERIFIED
Flags: needinfo?(camelia.badau)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: