Closed Bug 1267720 Opened 5 years ago Closed 4 years ago

Re-factor nsWindowWatcher::OpenWindowInternal to not be such a rat's nest

Categories

(Core :: General, defect)

defect
Not set
normal

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.
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
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.
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.
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/
Just checkpointing - there's still a bit of work here.
Depends on: 1276738
Depends on: 1276966
Attachment #8748869 - Attachment is obsolete: true
Attachment #8748870 - Attachment is obsolete: true
Attachment #8748871 - Attachment is obsolete: true
Attachment #8748872 - Attachment is obsolete: true
Attachment #8748873 - Attachment is obsolete: true
Attachment #8748874 - Attachment is obsolete: true
Attachment #8748875 - Attachment is obsolete: true
Attachment #8748876 - Attachment is obsolete: true
Attachment #8748877 - Attachment is obsolete: true
Attachment #8748878 - Attachment is obsolete: true
Attachment #8748879 - Attachment is obsolete: true
Attachment #8748880 - Attachment is obsolete: true
Attachment #8748881 - Attachment is obsolete: true
Attachment #8748882 - Attachment is obsolete: true
Attachment #8748883 - Attachment is obsolete: true
Attachment #8748884 - Attachment is obsolete: true
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: 4 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.