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

Status: VERIFIED FIXED

(Firefox :: Search, enhancement)

Description

[Mark Banner (:standard8)]

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 1

[Mark Banner (:standard8)]

Attachment: Bug 1373718 - Update follow-on search add-on to 0.9.1 - fix double reporting and fix case sensitivity.

Review commit: https://reviewboard.mozilla.org/r/149894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149894/

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

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?

Comment 3

[Mark Banner (: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.

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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f1c1dc1725c

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Reminder to request uplift soon.

Comment 7

[Mark Banner (: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

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

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+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/454e097b885f

Comment 10

[Camelia Badau [:cbadau], Desktop QA]

Mark, could you please tell me what exactly I should look for when verifying this? Are there any metrics I should be aware of?

Comment 11

[Mark Banner (:standard8)]

(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

[Camelia Badau [:cbadau], Desktop QA]

Thanks, Mark! 

Justin, could you please tell me what exactly I should look for when verifying this?

Comment 13

[Justin [:JW_SoftvisionQA]]

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

Comment 14

[Camelia Badau [:cbadau], Desktop QA]

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.