Closed Bug 964875 Opened 6 years ago Closed 6 years ago

Figure out a plan for ChromeWindow in the WebIDL world

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files, 1 obsolete file)

We could make an interface inheriting from Window, but that runs into the slight issue that [Global] does not allow interfaces inheriting from it in the spec and also that [StoreInSlot] is not supported on non-leaf interfaces.

We could try to change those, or we could try to hack around this as follows:

1)  Define an empty [ChromeOnly] ChromeWindow interface and have Window implement it.  Change the hasInstance codegen to treat that case like the "have prototype object" case, by doing a QI to nsIDOMChromeWindow and put this in Bindings.conf:

  'ChromeWindow': {
    'concrete': False,
    'headerFile': 'nsGlobalWindow.h',
    'nativeType': 'nsGlobalChromeWindow',
  },

2) Put all the nsIDOMChromeWindow bits on Window, with Func that QIs the object to nsIDOMChromeWindow guarding them.

This will give us the right instanceof behavior and members.  The one thing that would break is adding things to ChromeWindow.prototype making theme exposed on the chrome window (and in fact ChromeWindow.prototype would no longer exist).  I suspect that's OK.
Addons mxr shows no hits for ChromeWindow.prototype, fwiw.
Peter points out that nsGlobalWindow::IsChromeWindow exists; we should use that in the Func instead.  And maybe in the HasInstance code.
Blocks: 789261
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
Blocks: 965153
Attachment #8367133 - Attachment is obsolete: true
Attachment #8367133 - Flags: review?(peterv)
Comment on attachment 8367131 [details] [diff] [review]
part 1.  Add a WebIDL ChromeWindow interface for use in instanceof once Window is on WebIDL bindings.

Review of attachment 8367131 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +1265,5 @@
>      return true;
>    }
>    """
> +        if self.descriptor.interface.identifier.name == "ChromeWindow":
> +            setBp = "*bp = UnwrapDOMObject<nsGlobalWindow>(js::UncheckedUnwrap(instance))->IsChromeWindow()"

I know there are very few of these special cases, but how about a IsInstanceOf function templated on prototype id that does this check for ChromeWindow and just returns true for all other ids? We could use it for ModalWindow too.
Attachment #8367131 - Flags: review?(peterv) → review+
Comment on attachment 8367132 [details] [diff] [review]
part 2.  Add a static nsGlobalWindow::IsChromeWindow test function for use in Func annotations.

Review of attachment 8367132 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +4045,5 @@
> +  nsGlobalWindow* win;
> +  nsresult rv = UNWRAP_OBJECT(Window, aObj, win);
> +  if (NS_FAILED(rv)) {
> +    nsCOMPtr<nsPIDOMWindow> piWin = do_QueryInterface(
> +      nsContentUtils::XPConnect()->GetNativeOfWrapper(aCx, aObj));

do_QueryWrapper?
Attachment #8367132 - Flags: review?(peterv) → review+
Attachment #8367824 - Flags: review?(peterv) → review+
> how about a IsInstanceOf function templated on prototype id that does this check for
> ChromeWindow 

The prototype id in this case is prototypes::id::Window.  That's why I need the additional check; the current setup assumes that if "Foo implements Bar" then any Foo instance is a Bar instance, but for "Window implements ChromeWindow" that's not actually true...

> do_QueryWrapper?

Yes, good idea.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.