Closed Bug 463387 Opened 16 years ago Closed 16 years ago

Add an API for getting web progress notifications for all tabs

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.1b2

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

tabbrowser has addProgressListener, this allows you to register a progress listener for whichever is the visible tab.

Frequently in extension development you want to hear about events from all tabs. There are also likely to be useful uses for this within the product (bug 386835 will need this f.e.).

I propose adding a pair of new methods to tabbrowser, addAllProgressListener (please someone think of a better name) and removeAllProgressListener. It will register a progress listener that gets notified for all tabs, background or foreground. It will be passed a variant on nsIWebProgressListener. Essentially the object will have all the same methods with an additional argument, the browser element that fired the event.
Blocks: 386835
Attached patch patch rev 1 (obsolete) — Splinter Review
Went for addTabsProgressListener.

There are a couple of API changes to note here. Previously the object passed to addProgressListener would receive progress events for the front-most tab. It would also receive onLinkIconAvailable and onRefreshAttempted for all tabs. This seemed pretty inconsistent to me so this changes that to be only for the front-most tab as for the others. You can still get those events for all tabs on this new API so I've moved our use of it there. We could consider not making that change however this late.

The diff looks scarier than it is, the onRefreshAttempted method on XULBrowserWindow is moved to the new TabsProgressListener and a couple of minor changes made, the rest of XULBrowserWindow stays about the same.

The test included does a couple of foreground and background loads ensuring the right notifications are sent to the right progress listeners.
Attachment #346693 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Attachment #346693 - Flags: review?(mconnor) → review?(enndeakin)
Comment on attachment 346693 [details] [diff] [review]
patch rev 1

I don't have cycles to tackle this one in the next couple of weeks, but from a quick skim it seems well thought out and useful.  I assume its not for 3.1?
I want it for 3.1 if possible so that I can use it to fix bug 386836. It isn't necessary but I would only end up re-implementing the same sort if thing in browser.js instead.
Comment on attachment 346693 [details] [diff] [review]
patch rev 1

>+      <method name="addTabsProgressListener">
>+        <parameter name="aListener"/>
>+        <parameter name="aMask"/>

The aMask argument isn't used. It that just to mirror the existing method? It is important that the argument isn't used?

>+      <method name="removeTabsProgressListener">
>+        <parameter name="aListener"/>
>+        <body>
>+        <![CDATA[
>+          for (var i = 0; i < this.mTabsProgressListeners.length; i++) {
>+            if (this.mTabsProgressListeners[i] == aListener) {
>+              this.mTabsProgressListeners.splice(i, 1);
>+              break;
>+            }
>+          }

Use indexOf here?
Attachment #346693 - Flags: review?(enndeakin) → review+
(In reply to comment #4)
> (From update of attachment 346693 [details] [diff] [review])
> >+      <method name="addTabsProgressListener">
> >+        <parameter name="aListener"/>
> >+        <parameter name="aMask"/>
> 
> The aMask argument isn't used. It that just to mirror the existing method? It
> is important that the argument isn't used?

Yeah I guess I added it to match the other method, I will just drop it I guess. I wonder if for the other method we should put a console error or something if they try to apply any filter, because that doesn't work.
Attached patch patch rev 2Splinter Review
Fixes the comments given.
Attachment #346693 - Attachment is obsolete: true
Attachment #347770 - Flags: review+
Attachment #347770 - Flags: approval1.9.1b2?
Comment on attachment 347770 [details] [diff] [review]
patch rev 2

a1.9.1b2=beltzner
Attachment #347770 - Flags: approval1.9.1b2? → approval1.9.1b2+
Pushed: http://hg.mozilla.org/mozilla-central/rev/fbad4dfa05e5
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
For some reason that patch applied badly so it needed a bustage fix: http://hg.mozilla.org/mozilla-central/rev/497d9f3960de
We should also add docs to the tabbrowser reference and include the full list of methods the listener may get called with
No longer depends on: 463384
Depends on: 472677
Depends on: 479634
Blocks: FF2SM
No longer depends on: 472677
Blocks: 558995
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.