772299 - <browser> widget's addProgressListener method should pass through aNotifyMask argument

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

The <browser> widget's addProgressListener method is mostly a straight passthrough to the addProgressListener method of its webProgress property, but it doesn't pass through an aNotifyMask argument specifying the types of notifications to receive.

It should do so (while continuing to provide a default value if none is passed to it), as there are use cases for such functionality (cf. bug 771395, comment 8).

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: adds argument

Here's a patch that adds the aNotifyMask parameter to <browser>.addProgressListener() and refactors the runtime implementation and test code to use it.

I looked through the codebase and didn't see any other code that appears to use <browser>.webProgress.addProgressListener() because <browser>.addProgressListener() doesn't support this parameter, although at least four code chunks unnecessarily call <browser>.webProgress.addProgressListener() with a NOTIFY_ALL mask:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1339
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1873
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/web-panels.js#75
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/help/content/help.js#145

I'm unsure of the value of refactoring this code and haven't done so in this patch.

Note also bug 608628, in which the aMask parameter was removed from <tabbrowser>.addProgressListener(), although the reasoning in that bug doesn't appear to apply to this one.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 645498 [details] [diff] [review]
patch v1: adds argument

https://hg.mozilla.org/integration/mozilla-inbound/rev/f10e4ac36ed2

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Awesome, so this was backed out along with bug 780686 due to mochitest-other permaorange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5886a528d6db

After backing out, I realized that Fabrice pushed a follow-up that fixed the orange. Please be starring builds when pushing bustage fixes...

Re-pushed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fad6a5ad23a

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2fad6a5ad23a