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

VERIFIED FIXED in Firefox 55

Status

()

VERIFIED FIXED
a year ago
a year ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
Firefox 56
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 verified, firefox56 verified)

Details

Attachments

(1 attachment)

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 hidden (mozreview-request)

Comment 2

a year ago
mozreview-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

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+
(Assignee)

Comment 3

a year ago
mozreview-review-reply
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.

Comment 4

a year ago
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

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f1c1dc1725c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
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

Comment 9

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/454e097b885f
status-firefox55: --- → fixed
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
status-firefox55: fixed → verified
status-firefox56: fixed → verified
Flags: needinfo?(camelia.badau)
You need to log in before you can comment on or make changes to this bug.