787961 - Remove mDocument from WorkerPrivate

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

I'm not at all familiar with this code, but I don't really understand the need for
keeping document alive. Having pointer to the inner window should be enough.
https://tbpl.mozilla.org/?tree=Try&rev=c94f127254d1


(This does not fix https://bugzilla.mozilla.org/show_bug.cgi?id=785248)

Comment 1

[Masatoshi Kimura [:emk]]

>+  MOZ_ASSERT(!mWindow || mWindow->IsInnerWindow());
MOZ_ASSERT_IF(mWindow, mWindow->IsInnerWindow());

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I prefer to not use _IF. It tends to make code harder to read.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 657876 [details] [diff] [review]
patch

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

::: dom/workers/WorkerPrivate.cpp
@@ +2274,5 @@
>  uint64_t
>  WorkerPrivateParent<Derived>::GetInnerWindowId()
>  {
>    AssertIsOnMainThread();
> +  MOZ_ASSERT(!mWindow || mWindow->IsInnerWindow());

NIT: Please use NS_ASSERTION for now. At some point we need to go change all of these, but having a mix of both is awful (especially since you get no stack traces on tinderbox for MOZ_ASSERT at the moment).

::: dom/workers/WorkerPrivate.h
@@ +22,5 @@
>  #include "nsStringGlue.h"
>  #include "nsTArray.h"
>  #include "nsTPriorityQueue.h"
>  #include "StructuredCloneTags.h"
> +#include "nsPIDOMWindow.h"

Nit: Please move this into the other list of includes (for interfaces, above), in alphabetical order.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/eb201b1e55fd