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)
Firefox
Search
Tracking
()
VERIFIED
FIXED
Firefox 56
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
past
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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•8 years 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•8 years 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.
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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
bugherder uplift |
status-firefox55:
--- → fixed
Updated•8 years ago
|
Flags: qe-verify+
Comment 10•8 years ago
|
||
Mark, could you please tell me what exactly I should look for when verifying this? Are there any metrics I should be aware of?
Assignee | ||
Comment 11•8 years ago
|
||
(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.
Comment 12•8 years ago
|
||
Thanks, Mark!
Justin, could you please tell me what exactly I should look for when verifying this?
Flags: needinfo?(jwilliams)
Comment 13•8 years ago
|
||
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)
Comment 14•8 years ago
|
||
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.
Description
•