Closed Bug 326784 Opened 15 years ago Closed 15 years ago

add a notification that a newly-opened window has been initialized

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bryner, Assigned: bryner)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

Currently we have the "domwindowopened" notification that fires immediately after a new window has been created.  It would be useful to be able to inspect properties of the window such as the opener, name, and size, which have not yet been set.  Since "domwindowopened" is ingrained into the behavior of a few interface methods (nsPIWindowWatcher, nsIBaseWindow, nsIAppShellService), I'd like to add a new notification that would be suitable for MOZILLA_1_8_BRANCH.
Attached patch patch (obsolete) — Splinter Review
(Treat this as an either-or review request, it probably doesn't require two levels of review).

I called the new notification "domwindowready" for lack of something better.  It's a bit suboptimal in that it doesn't communicate the fact that the window is a newly-opened toplevel window.  I think as long as it's documented (where's a good place for that?) it should be ok.
Attachment #211473 - Flags: superreview?(jst)
Attachment #211473 - Flags: review?(bzbarsky)
> in that it doesn't communicate the fact that the window is a newly-opened
> toplevel window

Is that the intent of this notification?  If so, it's not being achieved with that patch.  For example, it will fire when loads are diverted into tabs or current windows via nsIWindowProvider...

(In reply to comment #2)
> > in that it doesn't communicate the fact that the window is a newly-opened
> > toplevel window
> 
> Is that the intent of this notification?  If so, it's not being achieved with
> that patch.  For example, it will fire when loads are diverted into tabs or
> current windows via nsIWindowProvider...
> 

Will checking windowIsNew take care of this?
Yeah, I think so.  Perhaps call the notification "newtoplevelwindowisready" to address your other concern?
Attached patch new patchSplinter Review
Ok.  "newtoplevelwindowisready" is pretty hard to read, so I changed over to the hyphenated style that's often used for observer notifications.  It's called "toplevel-window-ready", with the understanding that a window only "becomes ready" when it's initially created.
Attachment #211473 - Attachment is obsolete: true
Attachment #211525 - Flags: superreview?(bzbarsky)
Attachment #211525 - Flags: review?(bzbarsky)
Attachment #211473 - Flags: superreview?(jst)
Attachment #211473 - Flags: review?(bzbarsky)
Comment on attachment 211525 [details] [diff] [review]
new patch

Seems reasonable, if we change "dispatches" to "may dispatch" in the IDL.  Otherwise we're changing a frozen interface (in case someone overrides the window watcher contract).
Attachment #211525 - Flags: superreview?(bzbarsky)
Attachment #211525 - Flags: superreview+
Attachment #211525 - Flags: review?(bzbarsky)
Attachment #211525 - Flags: review+
checked in, thanks for the review.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
So is this going on MOZILLA_1_8_BRANCH? If so, it landing there should block bug 266371 landing there.
Blocks: 266371
Attachment #211525 - Flags: approval-branch-1.8.1?(bzbarsky)
Comment on attachment 211525 [details] [diff] [review]
new patch

Just don't forget the comment change!
Attachment #211525 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
Keywords: fixed1.8.1
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.