Peter Van der Beken [:peterv] Assignee
	Description • 11 years ago

      No description provided.

	Peter Van der Beken [:peterv] Assignee
	Comment 1 • 11 years ago

Finally green on try.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

Comment on attachment 812239 [details] [diff] [review]
v2

>+++ b/dom/base/nsGlobalWindow.cpp
>+nsGlobalWindow::GetWindow(ErrorResult& aError)
>+  FORWARD_TO_OUTER_OR_THROW(GetWindow, (aError), aError, nullptr);

So we should figure out how this stuff will really work with inner/outer stuff.

It seems to me that the WebIDL object is the _inner_ window, hence whever we get an nsGlobalWindow, or return one, in WebIDL methods, we should be dealing with an inner window.

I realize that's not how some of these methods work right now; we should do this in a followup bug.  This applies at least to GetWindow, GetSelf, GetFrames, and probably GetParent, GetOpener, maybe GetContent.  Not sure about Open()/OpenDialog.

>+nsGlobalWindow::GetContent(ErrorResult& aError)

This should do what GetScriptableContent does, not what the XPCOM GetContent does, no?

>+nsGlobalWindow::SetInnerWidth(double aInnerWidth, ErrorResult& aError)

Why double?  The code actually takes/returns integer CSS pixels.  Does any browser treat it as a double?

I realize the CSSOM spec has double here.  It also has the attribute as readonly.  I suspect both are bugs in CSSOM in practice in tems of site compat.  Please definitely file a bug on the latter: I'm pretty sure it's not web-compatible because it will throw in strict mode.  Writable properties with no-op setters are probably what needs to be specced here if the intent is to not allow pages to set these properties.

I guess returning integer-valued doubles will still be site-compatiblem but of course then we're not doing what the spec says at all.  Please file a followup to sort this out?  This also applies to various x/y stuff, moveTo, etc, etc.

>-  CheckSecurityWidthAndHeight(&aInnerWidth, nullptr);

Why this change?  Seems like it's better to do it up front like we used to, not separately on the two different codepaths...

>+nsGlobalWindow::SetInnerHeight(double aInnerHeight, ErrorResult& aError)

Same comments here as for SetInnerWidth.

>+double
>+nsGlobalWindow::GetOuterWidth(ErrorResult& aError)

Same issue here with double vs int.

>+nsGlobalWindow::SetOuterSize(double aLengthCSSPixels, bool aIsWidth,

Same issue with double vs int here.

>+  int32_t lenghtCSSPixels = aLengthCSSPixels;

And if this does not go away, please fix the spelling error in "length"?

>+nsGlobalWindow::GetScreenXY(ErrorResult& aError)
>+  aError = treeOwnerAsWin->GetPosition(&x, &y);
>+  return nsIntSize(x, y);

This is returning device pixels.  We should document that somewhere.

Also, for this function's return value nsIntPoint would make a lot more sense than nsIntSize, I would think.

>+double
>+nsGlobalWindow::GetScreenX(ErrorResult& aError)

Again, not clear to me that we should switch to returning double here.

> nsGlobalWindow::MozRequestAnimationFrame(nsIFrameRequestCallback* aCallback,

It looks like this patch is changing us to only do the !aCallback deprecation warning on the XPCOM version of this method.  That's wrong.  We want to do it on both.  And we _definitely_ want to bail out of the non-XPCOM version on null callback.

So I think the XPCOM MozRequestAnimationFrame should call the non-XPCOM one, which should do the warning-and-bail dance, as well as forwarding to the inner and whatnot.

>+nsGlobalWindow::GetFullScreen(ErrorResult& aError)
>+        bool fullScreen;
>+        window->GetFullScreen(&fullScreen);
>+        return fullScreen;

What if it throws?  I think you want more like:

  bool fullScreen = false;
  aResult = window->GetFullScreen(&fullScreen);
  return fullScreen;

>@@ -5424,135 +5849,162 @@ nsGlobalWindow::Alert(const nsAString& a
>+  if (NS_FAILED(rv)) {
>+    aError = rv;

  aError.Throw(rv);

>+nsGlobalWindow::Confirm(const nsAString& aMessage, ErrorResult& aError)
>+    aError = NS_ERROR_NOT_AVAILABLE;

  aError.Throw(NS_ERROR_NOT_AVAILABLE);

>+  if (NS_FAILED(rv)) {
>+    aError = rv;

  aError.Throw(rv);

>+  bool result;

Probably better to default it to false, in case the callees throw without setting it.  Bad things can happen if clang sees an uninitialized bool.  :(

>@@ -5570,22 +6022,27 @@ nsGlobalWindow::Prompt(const nsAString& 
>+  if (NS_FAILED(rv)) {
>+    aError = rv;

  aError.Throw(rv);

>+nsGlobalWindow::MoveTo(double aXPos, double aYPos, ErrorResult& aError)
>+  nsIntSize devPos(aXPos, aYPos);

This should be cssPos or something, since those are css pixels.

>   // mild abuse of a "size" object so we don't need more helper functions

This comment should presumably come before devPos/cssPos is declared.  :(

>+  devPos = CSSToDevIntPixels(devPos);

And this should be a new nsIntSize.  That way we're not mixing different kinds of units in the same object.

>+nsGlobalWindow::PostMessageMoz(JSContext* aCx, JS::Handle<JS::Value> aMessage,
>+                               const nsAString& aTargetOrigin,
>                                const JS::Value& aTransfer,

aTransfer should become a JS::Handle<JS::Value>.


>+nsGlobalWindow::ShowModalDialog(const nsAString& aUrl, nsIVariant* aArgument,
>+  if (Preferences::GetBool("dom.disable_window_showModalDialog", false)) {

Should that check maybe be in the IDL?

>+nsGlobalWindow::ShowModalDialog(JSContext* aCx, const nsAString& aUrl,
>+  MOZ_ASSERT(!aError.Failed());

Why can we assert that VariantToJS didn't fail?  I don't see why we can.

We should get a followup bug filed to wean this stuff off variants....

>+nsGlobalWindow::Find(const nsAString& aString, bool aCaseSensitive,
>+  if (Preferences::GetBool("dom.disable_window_find", false)) {

And this should maybe be in the IDL too.

>+  bool didFind;
>+  aError = finder->FindNext(&didFind);

Should initialize didFind to false or something, so we can't return uninitialized values.

>+nsGlobalWindow::GetLocation(ErrorResult& aError)
>   if (!mLocation && docShell) {

So if !docShell we can return null.  But the WebIDL says this isn't nullable.

We need to either throw if !docShell or make the IDL say this is nullable.

>+nsGlobalWindow::GetLocation(nsIDOMLocation ** aLocation)
>+  nsCOMPtr<nsIDOMLocation> location = GetLocation(rv);
>+  location.forget(aLocation);

Fwiw, this could also be:

  *aLocation = GetLocation(rv).get();

and similar in various other places in this patch.  Either way, I guess.

>+nsGlobalWindow::GetSessionStorage(ErrorResult& aError)
>+  if (!principal || !docShell || !Preferences::GetBool(kStorageEnabled)) {

Again, should the pref check be in the IDL?

>+    if (NS_FAILED(rv)) {
>+      aError = rv;

  aError.Throw(rv);

>+nsGlobalWindow::GetLocalStorage(ErrorResult& aError)
>   if (!Preferences::GetBool(kStorageEnabled)) {

And here, pref in IDL?  Followup probably ok for all of these pref things.

>+    if (NS_FAILED(rv)) {
>+      aError = rv;

  aError.Throw(rv);

>+nsGlobalWindow::InnerForSetTimeoutOrInterval(ErrorResult& aError)
>+  // This needs to forward to the inner window, but since the current
>+  // inner may not be the inner in the calling scope, we need to treat
>+  // this specially here as we don't want timeouts registered in a
>+  // dying inner window to get registered and run on the current inner
>+  // window.

Hmm.  This needs a spec issue, I believe; the spec doesn't have this sort of thing going on, so if we think it's needed the spec needs to change.

>+  // If the caller and the callee share the same outer window,
>+  // forward to the callee inner.

I know that's what the comment used to say, but it means "forward to the caller inner", as far as I can tell.

>+// Note that this changes the value of aTimeout sometimes.
>+IsInterval(const Optional<int32_t>& aTimeout, int32_t& aResultTimeout)

How does it change the value of aTimeout?  I don't see how....

>+  // If no interval was specified, treat this like a timeout, to avoid setting
>+  // an interval of 0 milliseconds.

I know we used to do this, but again, the spec has nothing like this.  Please either raise a spec issue or file a bug to remove this special case?

>+nsGlobalWindow::SetTimeoutOrInterval(Function& aFunction, int32_t aTimeout,
>+  ErrorResult rv;
>+  nsCOMPtr<nsIScriptTimeoutHandler> handler =
>+    NS_CreateJSTimeoutHandler(this, aFunction, aArguments, rv);

Why is that not using aError?  If there is a good reason, it at least deserves a comment.

The old code is ... odd.  It returns NS_OK after sometimes setting an exception on the JSContext and sometimes not.  Then CallMethodHelper::Call will notice the exception on the context and throw even though the C++ returned NS_OK.

I guess it's doing that to avoid clobbering the exception it reported?  But would we really clobber it nowadays, or do we check for an existing exception?

As in, can we just propagate the error return here in cases when it's an error return?  And can we just change the CSP-blocked case to return null without throwing, if it's trying to avoid throwing an exception?

>+++ b/dom/base/nsGlobalWindow.h
>+  nsIDocument* Document()
>+    return GetDoc();

Can GetDoc() never return null?

If so, a followup to get rid of it and make callers use Document()?  If it can return null, though, need to mark it so in the IDL.

>+  already_AddRefed<nsIDOMWindow> GetTop(mozilla::ErrorResult& aError)

The IDL says this never returns null, but I see nothing enforcing that, offhand. The IDL needs changing here, I think.

>+  already_AddRefed<nsIDOMWindow> Open(const nsAString& aUrl,

The idl says this never returns null.  Is that true in practice?

>+  nsIDOMStorage* GetSessionStorage(mozilla::ErrorResult& aError);

This can return null, but the IDL says otherwise.  Needs fixing.

>+  nsISelection* GetSelection(mozilla::ErrorResult& aError);

This can also return null, but the IDL says otherwise.

>+    GetComputedStyle(mozilla::dom::Element& aElt, const nsAString& aPseudoElt,

This can also return null, but the IDL says otherwise...

I really wish we had a type we could use for the return value here to make returning nullptr fail to compile.  :(

>+  already_AddRefed<nsIDOMMediaQueryList> MatchMedia(const nsAString& aQuery,

You know the drill.  This can return null, but the IDL says it can't.

>+    GetMozIndexedDB(mozilla::ErrorResult& aError)

This can be null, just like GetIndexedDB().  Please fix the IDL.

>+  already_AddRefed<nsIDOMWindow> OpenDialog(JSContext* aCx,

I'd think this can also return null; the IDL says it can't.

>+  already_AddRefed<nsIDOMWindow> GetContent(mozilla::ErrorResult& aError);

If GetTop() can return null, so can this one.  Please double-check!

>+++ b/dom/base/nsJSTimeoutHandler.cpp
>+  // The expression to evaluate or function to call, if !mExpr

!mExpr is meaningless now.  Please reword accordingly?

>+CheckCSPForEval(JSContext* aCx, nsGlobalWindow *aWindow)
>+  // if CSP is enabled, and setTimeout/setInterval was called with a string
>+  // or object, disable the registration and log an error

s/or object//, right?

>+nsJSScriptTimeoutHandler::nsJSScriptTimeoutHandler(nsGlobalWindow *aWindow,
>+                                                   Function& aFunction,
>+  mArgs.AppendElements(aArguments);

That's doing an infallible allocation.  The old code did a fallible one, and erorred out on failure.

Given the ease of passing in arrays of arbitrary length here, I think we should be doing a fallible allocation.

>+NS_CreateJSTimeoutHandler(JSContext* aCx, nsGlobalWindow *aWindow,
>+    if (rv.ErrorCode() != NS_ERROR_DOM_TYPE_ERR) {
>+      aError = rv;

If this is being done here, then we should _definitely_ be able to pass in aError in the caller instead of the temporary on-stack rv we actually pass in.

>+++ b/dom/bindings/Bindings.conf
>+    'wantsQI': False,

Hmm.  Why not true?

>+++ b/dom/webidl/EventHandler.webidl
>+Window implements OnErrorEventHandlerForWindow;

This line needs to be in Window.webidl.

>+++ b/dom/webidl/IDBEnvironment.webidl

I would actually prefer mozIndexedDB on a partial interface of IDBEnvironment
here, I think....

>+++ b/dom/webidl/Window.webidl
>+typedef any Transferable;

I guess this is no worse than what we have now, but the spec here makes no sense.  I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=23477

>+  [Throws] readonly attribute WindowProxy? parent;

I believe this is replaceable in our code.  If it's not replaceable in the spec and in other UAs, we should change our current code to do that too before we flip the WebIDL switch, to minimize risk.

>+  getter WindowProxy (unsigned long index);

I believe this is wrong, as in a bug in the spec, which I have already raised.  The indexed getter actually needs to live on the WindowProxy, not the Window, and we do that already.  Please comment out this line, with a note about why.

>+interface WindowTimers {

It would be good to link to the defining spec before every one of these snippets.

Same thing for all the partial interfaces.

And document which ones are Mozilla-extension-specific?

>+  [Throws] readonly attribute double scrollX;
>+  [Throws] readonly attribute double scrollY;

Again, these are currently replaceable.  We should change that before flipping the switch.

>+  [Replaceable, Throws] readonly attribute Performance? performance;

This one is _not_ currently replaceable.  Should it be?

Also, this this IDL comes from http://w3c-test.org/webperf/specs/NavigationTiming/#sec-window.performance-attribute but that's not mentioned anywhere, as far as I can tell.

>+  [Throws] readonly attribute nsIDOMCrypto        crypto;

This is in the "Mozilla extensions" section but isn't one, actually.  We should try to break this up per-spec...

>+  /* [replaceable] controllers */
>+  [Throws] readonly attribute MozControllers      controllers;

This should actually be [Replaceable].

>+  [Throws] readonly attribute long                scrollMaxX;
>+  [Throws] readonly attribute long                scrollMaxY;

These are currently replaceable.  They should stay that way or get flipped to not be replaceable now...

>+  // XXX Should this be in nsIDOMChromeWindow?
>+  void                      updateCommands(DOMString action);

Might be worth putting stuff like this that we want to move off Window on a separate partial interface and file a bug to move it.

As far as I can tell, we lose onmouseenter and onmouseleave on Window in this patch.  Where did they go?

>+  [Throws] readonly attribute WindowProxy                    content;

This is currently replaceable.  It should probably stay that way.

>+           attribute EventHandler ontouchstart;
>+           attribute EventHandler ontouchend;
>+           attribute EventHandler ontouchmove;
>+           attribute EventHandler ontouchenter;
>+           attribute EventHandler ontouchleave;
>+           attribute EventHandler ontouchcancel;

No, this is wrong.  For one thing, those are all conditional.  What you want instead is:

  Window implements TouchEventHandlers;

r=me with all that fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

> I guess this is no worse than what we have now,

That said, we should file a followup to typedef Transferable to a union of some sort, I guess.

	Peter Van der Beken [:peterv] Assignee
	Comment 5 • 11 years ago

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #3)
> It seems to me that the WebIDL object is the _inner_ window, hence whever we
> get an nsGlobalWindow, or return one, in WebIDL methods, we should be
> dealing with an inner window.
> 
> I realize that's not how some of these methods work right now; we should do
> this in a followup bug.

Yeah, we should definitely figure this out in a followup bug. Originally I'd made just the XPCOM methods forward, but that doesn't really help until we make the inner/outer split clearer in the C++ objects.

> >+nsGlobalWindow::GetContent(ErrorResult& aError)
> 
> This should do what GetScriptableContent does, not what the XPCOM GetContent
> does, no?

Hmm, yes. Unfortunately that seems to mean we need to make it return object :-(.

> Why double?  The code actually takes/returns integer CSS pixels.  Does any
> browser treat it as a double?

Changed to long.

> I realize the CSSOM spec has double here.  It also has the attribute as
> readonly.  I suspect both are bugs in CSSOM in practice in tems of site
> compat.  Please definitely file a bug on the latter: I'm pretty sure it's
> not web-compatible because it will throw in strict mode.  Writable
> properties with no-op setters are probably what needs to be specced here if
> the intent is to not allow pages to set these properties.

Will file a bug.

> Why this change?  Seems like it's better to do it up front like we used to,
> not separately on the two different codepaths...

Done.

> >+nsGlobalWindow::SetInnerHeight(double aInnerHeight, ErrorResult& aError)
> 
> Same comments here as for SetInnerWidth.

Done.

> Same issue here with double vs int.

Done.

> Same issue with double vs int here.

Done.

> And if this does not go away, please fix the spelling error in "length"?

It's gone.

> This is returning device pixels.  We should document that somewhere.

Done.

> Also, for this function's return value nsIntPoint would make a lot more
> sense than nsIntSize, I would think.

Done.

> Again, not clear to me that we should switch to returning double here.

Done.

> It looks like this patch is changing us to only do the !aCallback
> deprecation warning on the XPCOM version of this method.  That's wrong.  We
> want to do it on both.  And we _definitely_ want to bail out of the
> non-XPCOM version on null callback.
> 
> So I think the XPCOM MozRequestAnimationFrame should call the non-XPCOM one,
> which should do the warning-and-bail dance, as well as forwarding to the
> inner and whatnot.

The WebIDL binding ensures a non-null callback already.

> What if it throws?  I think you want more like:
> 
>   bool fullScreen = false;
>   aResult = window->GetFullScreen(&fullScreen);
>   return fullScreen;

Done.

>   aError.Throw(rv);

Done.

>   aError.Throw(NS_ERROR_NOT_AVAILABLE);

Done.

>   aError.Throw(rv);

Done.

> Probably better to default it to false, in case the callees throw without
> setting it.  Bad things can happen if clang sees an uninitialized bool.  :(

Done.

>   aError.Throw(rv);

Done.

> This should be cssPos or something, since those are css pixels.

Done.

> This comment should presumably come before devPos/cssPos is declared.  :(

Done.

> And this should be a new nsIntSize.  That way we're not mixing different
> kinds of units in the same object.

Done.

> aTransfer should become a JS::Handle<JS::Value>.

Done.

> Should that check maybe be in the IDL?

Depends, do we want to hide it or disable it? I've kept this as is.

> Why can we assert that VariantToJS didn't fail?  I don't see why we can.

Done.

> We should get a followup bug filed to wean this stuff off variants....

> And this should maybe be in the IDL too.

See above, kept a is.

> Should initialize didFind to false or something, so we can't return
> uninitialized values.

Done.

> So if !docShell we can return null.  But the WebIDL says this isn't nullable.
> 
> We need to either throw if !docShell or make the IDL say this is nullable.

Done.

> >+nsGlobalWindow::GetLocation(nsIDOMLocation ** aLocation)
> >+  nsCOMPtr<nsIDOMLocation> location = GetLocation(rv);
> >+  location.forget(aLocation);
> 
> Fwiw, this could also be:
> 
>   *aLocation = GetLocation(rv).get();

It could if the other GetLocation returned an already_AddRefed, but it doesn't.

> Again, should the pref check be in the IDL?

>   aError.Throw(rv);

Done.

> >+nsGlobalWindow::GetLocalStorage(ErrorResult& aError)
> >   if (!Preferences::GetBool(kStorageEnabled)) {
> 
> And here, pref in IDL?  Followup probably ok for all of these pref things.

>   aError.Throw(rv);

Done.

> >+  // This needs to forward to the inner window, but since the current
> >+  // inner may not be the inner in the calling scope, we need to treat
> >+  // this specially here as we don't want timeouts registered in a
> >+  // dying inner window to get registered and run on the current inner
> >+  // window.
> 
> Hmm.  This needs a spec issue, I believe; the spec doesn't have this sort of
> thing going on, so if we think it's needed the spec needs to change.

Will file a bug.

> I know that's what the comment used to say, but it means "forward to the
> caller inner", as far as I can tell.

Right, changed.

> How does it change the value of aTimeout?  I don't see how....

Yeah, doesn't anymore.

> I know we used to do this, but again, the spec has nothing like this. 
> Please either raise a spec issue or file a bug to remove this special case?

Will do.

> Why is that not using aError?  If there is a good reason, it at least
> deserves a comment.

Done.

> The old code is ... odd.  It returns NS_OK after sometimes setting an
> exception on the JSContext and sometimes not.  Then CallMethodHelper::Call
> will notice the exception on the context and throw even though the C++
> returned NS_OK.

Good point.

> I guess it's doing that to avoid clobbering the exception it reported?  But
> would we really clobber it nowadays, or do we check for an existing
> exception?

We check afaict.

> As in, can we just propagate the error return here in cases when it's an
> error return?  And can we just change the CSP-blocked case to return null
> without throwing, if it's trying to avoid throwing an exception?

Yeah, done. A little ugly because I need a bool* argument to be able to signal succes but not allowed, but oh well.

> Can GetDoc() never return null?
> 
> If so, a followup to get rid of it and make callers use Document()?  If it
> can return null, though, need to mark it so in the IDL.

Yeah, marked.

> The IDL says this never returns null, but I see nothing enforcing that,
> offhand. The IDL needs changing here, I think.

Done.

> The idl says this never returns null.  Is that true in practice?

Marked in the id.

> This can return null, but the IDL says otherwise.  Needs fixing.

Done.

> This can also return null, but the IDL says otherwise.

Done.

> This can also return null, but the IDL says otherwise...

Done.

> I really wish we had a type we could use for the return value here to make
> returning nullptr fail to compile.  :(

> You know the drill.  This can return null, but the IDL says it can't.

Done.

> This can be null, just like GetIndexedDB().  Please fix the IDL.

Done.

> I'd think this can also return null; the IDL says it can't.

Done.

> If GetTop() can return null, so can this one.  Please double-check!

Not really, it explicitly checks for non-null after checking for error.

> !mExpr is meaningless now.  Please reword accordingly?

Done.

> s/or object//, right?

Done.

> That's doing an infallible allocation.  The old code did a fallible one, and
> erorred out on failure.
> 
> Given the ease of passing in arrays of arbitrary length here, I think we
> should be doing a fallible allocation.

Done, made the constructor swap and NS_CreateJSTimeoutHandler creates a new fallible array and copies. It'd be sort of nice to be able to take over the storage of the Sequence since the binding doesn't need it anymore.

> If this is being done here, then we should _definitely_ be able to pass in
> aError in the caller instead of the temporary on-stack rv we actually pass
> in.

Done.

> >+    'wantsQI': False,
> 
> Hmm.  Why not true?

I don't think we want to quickstub-install QI. We should probably make Window implement LegacyQueryInterface when we switch completely though.

> This line needs to be in Window.webidl.

Done.

> I would actually prefer mozIndexedDB on a partial interface of IDBEnvironment
> here, I think....

Done.

> I believe this is replaceable in our code.  If it's not replaceable in the
> spec and in other UAs, we should change our current code to do that too
> before we flip the WebIDL switch, to minimize risk.

Made it replaceable for now, will file a bug.

> I believe this is wrong, as in a bug in the spec, which I have already
> raised.  The indexed getter actually needs to live on the WindowProxy, not
> the Window, and we do that already.  Please comment out this line, with a
> note about why.

Done.

> It would be good to link to the defining spec before every one of these
> snippets.

Done.

> Same thing for all the partial interfaces.

Done.

> And document which ones are Mozilla-extension-specific?

The Mozilla extensions are all in a separate partial interface.

> Again, these are currently replaceable.  We should change that before
> flipping the switch.

Made them Replaceable for now.

> This one is _not_ currently replaceable.  Should it be?

Done.

> Also, this this IDL comes from
> http://w3c-test.org/webperf/specs/NavigationTiming/#sec-window.performance-
> attribute but that's not mentioned anywhere, as far as I can tell.

Linked to https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html.

> This is in the "Mozilla extensions" section but isn't one, actually.  We
> should try to break this up per-spec...

Done.

> This should actually be [Replaceable].

Done.

> These are currently replaceable.  They should stay that way or get flipped
> to not be replaceable now...

Done.

> Might be worth putting stuff like this that we want to move off Window on a
> separate partial interface and file a bug to move it.

Well, maybe this should just be ChromeOnly in WebIDL?

> As far as I can tell, we lose onmouseenter and onmouseleave on Window in
> this patch.  Where did they go?

They live on GlobalEventHandlers, like in the spec.

> This is currently replaceable.  It should probably stay that way.

Done.

> No, this is wrong.  For one thing, those are all conditional.  What you want
> instead is:
> 
>   Window implements TouchEventHandlers;

Done.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 7 • 11 years ago

Landed as: <https://hg.mozilla.org/integration/mozilla-inbound/rev/2e466ccc7bd0>.

Broke a mozRequestAnimationFrame call; I think the one in <https://mxr.mozilla.org/mozilla-central/source/browser/devtools/shadereditor/test/head.js#136>: <https://tbpl.mozilla.org/php/getParsedLog.php?id=29710069&tree=Mozilla-Inbound>.

Backed out as: <https://hg.mozilla.org/integration/mozilla-inbound/rev/389dbd870f32>.

	Peter Van der Beken [:peterv] Assignee
	Comment 8 • 11 years ago

Thanks, looks like a recent regression because a try run on yesterday was green :-(. You did find the broken mozRequestAnimationFrame call, it looks like we get a security exception when unwrapping.

	Peter Van der Beken [:peterv] Assignee
	Comment 9 • 11 years ago

We're not going through XPCConvert for new DOM binding (or QS) unwrapping, so we didn't get the benfit of the code in attachment 713965 [details] [diff] [review]. Some of the tests added in bug 910955 end up hitting this :-/.

	Peter Van der Beken [:peterv] Assignee
	Comment 10 • 11 years ago

Relanded with a test fix instead (requestAnimationFrame instead of mozRequestAnimationFrame). I'll move attachment 822310 [details] to a separate bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/508288a2b62c

	Peter Van der Beken [:peterv] Assignee
	Comment 11 • 11 years ago

Comment on attachment 822810 [details] [diff] [review]
Port hack from XPCConvert unwrapping to XPCQuickStubs

Moved to bug 931467.

	Phil Ringnalda (:philor)
	Comment 12 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/508288a2b62c

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

I filed the following bugs, per comment 3 and comment 5:

* https://www.w3.org/Bugs/Public/show_bug.cgi?id=24159
* https://www.w3.org/Bugs/Public/show_bug.cgi?id=24160
* Bug 952892.
* 

> Depends, do we want to hide it or disable it? I've kept this as is.

If we disabled it by no-opping it, that would make sense.  But for both showModalDialog() and find() we disable by throwing, at which point I think we should just not have the method present at all.

Peter, did you ever raise a spec issue or bug on our end about the InnerForSetTimeoutOrInterval stuff?  What about the 0-delay interval thing?  What about window.parent being replaceable?  What about scrollX/scrollY being replaceable?  What about scrollMaxX/Y?  

>Well, maybe this should just be ChromeOnly in WebIDL?

I would be OK with that.