Bob Owen (:bobowen) Assignee
	Description • 10 years ago

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.

	Bob Owen (:bobowen) Assignee
	Comment 1 • 10 years ago

Looking fairly green on my try push, so I'll start uploading these:
https://tbpl.mozilla.org/?tree=Try&rev=790dc14d8cc7

Given the clear after the structured clone read, doesn't look like we'd need legacy error reporting there.
The mBackingStore->Put doesn't directly use the context and I think it eventually uses the one set up in the CallbackObject::CallSetup.

	Bob Owen (:bobowen) Assignee
	Comment 2 • 10 years ago

Virtually the same as Part 1.

I meant to mention this before ...
The main difference with my replacement code (apart from just using the SafeJSContext) is that if something goes wrong with the AutoJSAPI Init, then I am throwing an error and returning.
Whereas before, if the JSContext set up failed, it asserted.

	Bob Owen (:bobowen) Assignee
	Comment 3 • 10 years ago

Given the JS_ReportPendingException the legacy reporting looks like the safest option.

	Bob Owen (:bobowen) Assignee
	Comment 4 • 10 years ago

Pretty much the same as Part 3.

	Bob Owen (:bobowen) Assignee
	Comment 5 • 10 years ago

Had to do this one before the last one in RuntimeService, so I could rip out the nsIScriptContext pararmeter.

Apart from that, this is pretty similar to the last two.

	Bob Owen (:bobowen) Assignee
	Comment 6 • 10 years ago

Now the scx parameter has gone this is another along the same vein.

	Bob Owen (:bobowen) Assignee
	Comment 7 • 10 years ago

Given the xpc::Throw I've not looked into this too deeply and just gone for legacy reporting.

	Bob Owen (:bobowen) Assignee
	Comment 8 • 10 years ago

Assuming that the current code is correct, where we are using the passed in aCx for the rooted dictionaries, it seems a bit odd that we are pushing the contexts where we are.

So, given the contexts are only being used to report errors when something has gone wrong, it seems to make sense to move them into those blocks.

	Bobby Holley (:bholley)
	Comment 9 • 10 years ago

Comment on attachment 8448199 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in SynchronizeAndResumeRunnable::Run v1

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

r=bholley with that.

::: dom/workers/WorkerPrivate.h
@@ +341,3 @@
>  
>    bool
>    Suspend(JSContext* aCx, nsPIDOMWindow* aWindow);

Can you add a comment above these 3 methods stating that we can assume the existence of a Window because this stuff only applies to globals going in and out of the bfcache? In general, Worker stuff needs to work with non-Window globals as well, so this threw me for a loop.

	Bobby Holley (:bholley)
	Comment 10 • 10 years ago

Comment on attachment 8448223 [details] [diff] [review]
Part 8: Replace AutoCxPusher in WorkerPrivateParent<Derived>::DispatchMessageEventToMessagePort v1

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

Instead of doing this, can we just make a version of Throw() that takes a DOM Window? That'll be less AutoJSAPI-ing and make it easier to eventually transition away from JSContexts entirely here.

	Bob Owen (:bobowen) Assignee
	Comment 11 • 10 years ago

(In reply to Bobby Holley (:bholley) (PTO 6/25-6/29) from comment #10)
> Comment on attachment 8448223 [details] [diff] [review]
> Part 8: Replace AutoCxPusher in
> WorkerPrivateParent<Derived>::DispatchMessageEventToMessagePort v1
> 
> Review of attachment 8448223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Instead of doing this, can we just make a version of Throw() that takes a
> DOM Window? That'll be less AutoJSAPI-ing and make it easier to eventually
> transition away from JSContexts entirely here.

Makes sense, I think it might apply in some other places already as well.
Anyway, time for sleep.

	Bob Owen (:bobowen) Assignee
	Comment 12 • 10 years ago

r=bholley - from comment 9.

Added requested comment.

	Bob Owen (:bobowen) Assignee
	Comment 13 • 10 years ago

Added a ThrowAndReport method that takes a window, as the exceptions were reported straight afterwards.

It's a bit odd that the Throw always returns false.

	Bob Owen (:bobowen) Assignee
	Comment 14 • 10 years ago

Whoops, forgot the try push:
https://tbpl.mozilla.org/?tree=Try&rev=2a18ae261bfc

	Bobby Holley (:bholley)
	Comment 15 • 10 years ago

Comment on attachment 8448857 [details] [diff] [review]
Part 8: Replace AutoCxPusher in WorkerPrivateParent<Derived>::DispatchMessageEventToMessagePort v2

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

::: dom/bindings/Exceptions.cpp
@@ +132,5 @@
> +  if (NS_WARN_IF(!jsapi.InitWithLegacyErrorReporting(aWindow))) {
> +    return false;
> +  }
> +
> +  // This version of Throw always returns false for some reason.

so that people can just do "return Throw(..)". I don't necessarily agree with this design pattern, but it's common enough that I don't think we need this comment.

@@ +134,5 @@
> +  }
> +
> +  // This version of Throw always returns false for some reason.
> +  Throw(jsapi.cx(), aRv, aMessage);
> +  return JS_ReportPendingException(jsapi.cx());

I don't think the return value of JS_ReportPendingException is all that useful. I think this function should return void and the last line should explicitly cast the result of JS_ReportPendingException to void.

	Bob Owen (:bobowen) Assignee
	Comment 16 • 10 years ago

r=bholley - from comment 15

(In reply to Bobby Holley (:bholley) from comment #15)

> > +  // This version of Throw always returns false for some reason.
> 
> so that people can just do "return Throw(..)". I don't necessarily agree
> with this design pattern, but it's common enough that I don't think we need
> this comment.

Ah, I see, comment removed.
I've not seen that before, probably because most of my experience of this style of coding has been with "proper" exception handling.

I suppose it saves a line ... but if I were using it, I'd call the function ThrowAndReturnFalse (or something), to make it explicitly clear what was going on.
Particularly when the other Throws (albeit with different names) don't use this pattern.
 
> > +  return JS_ReportPendingException(jsapi.cx());
> 
> I don't think the return value of JS_ReportPendingException is all that
> useful. I think this function should return void and the last line should
> explicitly cast the result of JS_ReportPendingException to void.

OK, I'm guessing that this is a last ditch attempt to report the error and if it failed there's not much we could do anyway.

	Bob Owen (:bobowen) Assignee
	Comment 17 • 10 years ago

r=bholley - from comment 15 with points addressed in comment 16.

I noticed, in one of your patches for Bug 1032317, that C-style casting seems to be the norm when deliberately ignoring a return value.

Sorry for the spam.

	Bob Owen (:bobowen) Assignee
	Comment 18 • 10 years ago

Fairly full try push before reviews:
https://tbpl.mozilla.org/?tree=Try&rev=790dc14d8cc7

Smaller try push after first changes to Part 8:
https://tbpl.mozilla.org/?tree=Try&rev=2a18ae261bfc

Compiled and tested locally after the final changes, but I think they're small enough to not warrant another try push ... famous last words I'm sure. :-)

Landing in patch number order please.
Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb721c3b80f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bceb1dd6129
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdcaaf2af7bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/49a7c1aa4ae1
https://hg.mozilla.org/integration/mozilla-inbound/rev/98bdfcb393d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0edda6d34b
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a7bc7215e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b66860fde6d

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/6eb721c3b80f
https://hg.mozilla.org/mozilla-central/rev/2bceb1dd6129
https://hg.mozilla.org/mozilla-central/rev/bdcaaf2af7bb
https://hg.mozilla.org/mozilla-central/rev/49a7c1aa4ae1
https://hg.mozilla.org/mozilla-central/rev/98bdfcb393d0
https://hg.mozilla.org/mozilla-central/rev/9c0edda6d34b
https://hg.mozilla.org/mozilla-central/rev/86a7bc7215e7
https://hg.mozilla.org/mozilla-central/rev/4b66860fde6d