browser_popupNotification_3.js should use addProgressListener+executeSoon rather than addTabsProgressListener

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Gavin, Assigned: 526avijit, Mentored)

Tracking

unspecified
mozilla38
x86
All
Points:
1
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [lang=js][good first bug])

User Story

Fixing this requires minimal refactoring in a test which can be executed easily. It would make a great first bug for a new contributor.

Currently browser/base/content/test/popupNotifications/browser_popupNotification_3.js calls addTabsProgressListener()[1] to wait for onLocationChange[2] to occur. This listens for onLocationChange in all tabs, where the test really only cares about a single <browser>[3].

To listen for just the single browser we care about we should be calling addProgressListener()[4] instead. This involves replacing where addTabsProgressListener is called[5] with addProgressListener. Also the call to removeTabsProgressListener[6] will need to be replaced with removeProgressListener.

As mentioned in bug 915951 comment 58, it looks like the onLocationChange handler[7] might depend on something running in another onLocationChange handler. In order to ensure the test's handler runs second, we should also call 'executeSoon()'[8] to run our code.

Lastly, since we now know we are only listening to onLocationChange for the correct browser, we can get rid of the code that checks the browser in the onLocationChange handler[9]

This test can be executed from the firefox source repository by running:
 ./mach mochitest-browser browser/base/content/test/popupNotifications/browser_popupNotification_3.js


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/AddTabsProgressListener
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebProgressListener#onLocationChange%28%29
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/browser


[4] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/addProgressListener
[5] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/popupNotifications/browser_popupNotification_3.js?rev=1f51eb900e57#264
[6] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/popupNotifications/browser_popupNotification_3.js?rev=1f51eb900e57#259

[7] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/popupNotifications/browser_popupNotification_3.js?rev=1f51eb900e57#249
[8] executeSoon is a helper function which "Executes a function shortly after the call, but lets the caller continue working (or finish)."

[9] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/popupNotifications/browser_popupNotification_3.js?rev=1f51eb900e57#250

Attachments

(1 attachment, 1 obsolete attachment)

Blocks: 950073

Updated

5 years ago
Whiteboard: p=0
Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.
Component: General → Notifications and Alerts
Product: Firefox → Toolkit
Version: Trunk → unspecified

Updated

4 years ago
No longer blocks: 950073
Flags: firefox-backlog+
Whiteboard: p=0 → p=1

Updated

4 years ago
Points: --- → 1
Flags: qe-verify?
Whiteboard: p=1
This would make a great first bug.
Mentor: smacleod
User Story: (updated)
OS: Mac OS X → All
Summary: browser_popupNotification.js should use addProgressListener+executeSoon rather than addTabsProgressListener → browser_popupNotification_3.js should use addProgressListener+executeSoon rather than addTabsProgressListener
Whiteboard: [lang=js][good first bug]
(Assignee)

Comment 3

4 years ago
Created attachment 8557815 [details] [diff] [review]
Patch

all of the necessary changes have been made except : `executeSoon()`.
i am unable to comprehend that where should the `executeSoon()` statement be placed.
Flags: needinfo?(smacleod)
Hi Avijit, thanks for working on this!

I've assigned the bug to you and I will take a look at the code tonight to provide feedback.
Assignee: nobody → 526avijitgupta
Status: NEW → ASSIGNED
Flags: needinfo?(smacleod)
Attachment #8557815 - Flags: review?(smacleod)
Comment on attachment 8557815 [details] [diff] [review]
Patch

Review of attachment 8557815 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great so far! As you mention though it is missing the `executeSoon` call.

We'd like to wrap the code inside of `onLocationChange()` with the `executeSoon()`. This will cause it to execute after all of the other onLocationChange handlers have completed. The new body of the `onLocationChange()` method would then look something like:
>  gBrowser.removeProgressListener(progressListener);
>
>  executeSoon(() => {
>    // Old body of onLocationChange()
>  });

Note: In the above code snippet I'm using an ES6 feature you may be unfamiliar with called Arrow Functions[1]. This allows specifying an anonymous function with a shorter syntax "function () { ... }" vs "() => { ... }" - Arrow functions are used throughout the Firefox code base.


[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

::: browser/base/content/test/popupNotifications/browser_popupNotification_3.js
@@ +245,5 @@
>      },
>      onShown: function (popup) {
>        let self = this;
>        let progressListener = {
>          onLocationChange: function onLocationChange(aBrowser) {

Since we are now calling "addProgressListener" instead of "addTabsProgressListener" this function will not be passed a browser argument. We should remove the 'aBrowser' argument to the function.

onLocationChange will actually be passed a couple of arguments (which are listed at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebProgressListener#onLocationChange%28%29 ). We don't use any of them in the function though and there are many places in the code base we don't bother specifying them so it's fine to just leave the set of arguments empty: "function onLocationChange() {"

@@ +257,3 @@
>          },
>        };
> +	

Can you please remove the whitespace from this line (259)
Attachment #8557815 - Flags: review?(smacleod) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8558643 [details] [diff] [review]
Patch - includes `executeSoon()`

a single patch accommodating all the changesets.
Attachment #8557815 - Attachment is obsolete: true
Attachment #8558643 - Flags: review?(smacleod)
Comment on attachment 8558643 [details] [diff] [review]
Patch - includes `executeSoon()`

Review of attachment 8558643 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks for fixing this up. I've made a try[1] push to run the test, if that passes we can land this.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=423e4cdf95ce


[1] https://wiki.mozilla.org/Build:TryServer
Attachment #8558643 - Flags: review?(smacleod) → review+
The try push looks good - I've marked this bug as checkin-needed so that it will be landed soon.

Thanks for fixing this! Please let me know if you'd like any help finding more bugs to work on or contributing in general.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/cab80595dfc3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cab80595dfc3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → mozilla38

Updated

4 years ago
Iteration: --- → 38.2 - 9 Feb
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.