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)
Firefox
Tabbed Browser
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)
|
(deleted),
patch
|
mossop
:
review+
beltzner
:
approval1.9.1b2+
|
Details | Diff | Splinter Review |
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.
|
Assignee | |
Comment 1•16 years ago
|
||
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)
|
|
Assignee | |
Updated•16 years ago
|
Status: NEW → ASSIGNED
|
||
Updated•16 years ago
|
Attachment #346693 -
Flags: review?(mconnor) → review?(enndeakin)
|
|
||
Comment 2•16 years ago
|
||
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?
|
|
Assignee | |
Comment 3•16 years ago
|
||
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 4•16 years ago
|
||
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+
|
|
Assignee | |
Comment 5•16 years ago
|
||
(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.
|
|
Assignee | |
Comment 6•16 years ago
|
||
Fixes the comments given.
Attachment #346693 -
Attachment is obsolete: true
Attachment #347770 -
Flags: review+
|
|
Assignee | |
Updated•16 years ago
|
Attachment #347770 -
Flags: approval1.9.1b2?
|
||
Comment 7•16 years ago
|
||
Comment on attachment 347770 [details] [diff] [review]
patch rev 2
a1.9.1b2=beltzner
Attachment #347770 -
Flags: approval1.9.1b2? → approval1.9.1b2+
|
|
Assignee | |
Comment 8•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: dev-doc-needed,
late-compat
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
|
|
Assignee | |
Comment 9•16 years ago
|
||
For some reason that patch applied badly so it needed a bustage fix: http://hg.mozilla.org/mozilla-central/rev/497d9f3960de
|
||
Updated•16 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
|
|
Assignee | |
Comment 10•16 years ago
|
||
We should also add docs to the tabbrowser reference and include the full list of methods the listener may get called with
Keywords: dev-doc-complete → dev-doc-needed
|
|
||
Comment 11•16 years ago
|
||
This is now documented:
https://developer.mozilla.org/en/XUL/Method/addTabsProgressListener
https://developer.mozilla.org/en/XUL/Method/removeTabsProgressListener
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•