Closed
Bug 1267720
Opened 8 years ago
Closed 8 years ago
Re-factor nsWindowWatcher::OpenWindowInternal to not be such a rat's nest
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: mconley, Unassigned)
References
Details
Attachments
(16 obsolete files)
nsWindowWatcher::OpenWindowInternal is what eventually gets called when a new window or tab needs to be opened. This method... does a lot. A _lot_. And it's used in both the content process and the parent process, and there is lots of special-casing throughout the method for the type of window that's being opened, the privileges of the caller, the URL we're going to, which process we're in... a lot of stuff. It makes reasoning about window opening very, very difficult. The code is brittle, and that makes it really hard to change with any degree of confidence that you haven't caused some subtle bugs somewhere. As there are various security protections set up in OpenWindowInternal, that can get pretty dice-y. Bug 1261842 is, at least for me, the straw that is going to break the back of the camel here. In that bug, I need to set the size of the newly opened window in the parent process, but with the added complication that the primary content shell will no longer exist. This worries me because OpenWindowInternal falls back to using the docShell of the chrome window itself then to make various decisions and set various properties instead of on the docshell of the newly opened window, and that worries me a great deal. So I want to refactor this mutha of a method. I want to break it into smaller bits that are well defined, and make it clearer under what conditions we want to use certain bits. Wish me luck.
Reporter | ||
Comment 1•8 years ago
|
||
Having studied OpenWindowInternal for a day or two now, I think I can break it down into a few phases. There's some bleed-over from phase to phase, but I think it can be safely reorganized to partition them from one another. Here's the outcome I'm aiming for: nsWindowWatcher::OpenWindowInternal(args1, args2, args3, argsforever) { // Common preparation // Create Trait based on arguments. Trait does assertions and internal preparation. // Ask Trait to find or create us a window, returning a bool for whether or not we’re re-using a window, and a result code. // Ensure that we have a window by now, and return an error code if we do not. // Do common security checks, assertions, and setup // Do Trait-specific security checks, assertions and setup // If we created a new window, do common new window configuration // If we created a new window, do Trait-specific new window configuration // Do common post-configuration tasks // Do Trait-specific post-configuration tasks } Where a Trait will be a C-struct, specializing in one of the following cases: 1) We're in the content process 2) We're in the parent process, responding to a request from the content process to open a new window 3) We're in the parent process, and we're responding to a request to open a new window from anything except the content process
Reporter | ||
Comment 2•8 years ago
|
||
Since in some cases, there might be some overlap between 2 of the Traits (but not 3, since that'd make me put it in "common"), I'll probably break that code out into helper functions that can be called from the Trait methods.
Reporter | ||
Comment 3•8 years ago
|
||
A top-to-bottom review of this code has shown that there's really not much that's common between all three. There's plenty of overlap between the (1) and (3) case of the cases I listed in comment 1, and a little bit of overlap between (2) and (3). I don't believe there's any overlap between (1) and (2). That's going to inform my refactoring somewhat. Stay tuned.
Reporter | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50563/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50563/
Reporter | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50565/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50565/
Reporter | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50567/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50567/
Reporter | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50569/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50569/
Reporter | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50571/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50571/
Reporter | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50573/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50573/
Reporter | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50575/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50575/
Reporter | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50577/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50577/
Reporter | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50579/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50579/
Reporter | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50581/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50581/
Reporter | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50583/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50583/
Reporter | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50585/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50585/
Reporter | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50587/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50587/
Reporter | ||
Comment 17•8 years ago
|
||
By the time that the parent is being asked to create a new window, the name really doesn't matter anymore. Review commit: https://reviewboard.mozilla.org/r/50589/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50589/
Reporter | ||
Comment 18•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50591/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50591/
Reporter | ||
Comment 19•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50593/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50593/
Reporter | ||
Comment 20•8 years ago
|
||
Just checkpointing - there's still a bit of work here.
Reporter | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c39fc3c9b7b7
Reporter | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28ef4925834b
Reporter | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aec38e747ba
Reporter | ||
Comment 24•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3cc20d56d6a
Reporter | ||
Comment 25•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23ac582cc3e0
Reporter | ||
Comment 26•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2491f26a32be
Reporter | ||
Comment 27•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9e0f1fa4930
Reporter | ||
Comment 28•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=111c85a1e935
Reporter | ||
Comment 29•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fa75e2264d6
Reporter | ||
Comment 30•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fc50dbcdb98
Reporter | ||
Comment 31•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c55a9b32edf
Reporter | ||
Comment 32•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4617658ff26d
Reporter | ||
Updated•8 years ago
|
Attachment #8748869 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748870 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748871 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748872 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748873 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748874 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748875 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748876 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748877 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748878 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748879 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748880 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748881 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748882 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748883 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8748884 -
Attachment is obsolete: true
Reporter | ||
Comment 33•8 years ago
|
||
The work for this was done in bug 1261842. The logic for opening windows from child processes was extracted from OpenWindowInternal, and specialized/isolated logic for processing chromeFlags for windows opened by content were added. I don't think there's any reason to keep this bug open.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•