Closed Bug 787961 Opened 7 years ago Closed 7 years ago

Remove mDocument from WorkerPrivate

Categories

(Core :: DOM: Workers, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Unassigned)

Details

Attachments

(2 files)

Attached patch patchSplinter Review
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)
Attachment #657876 - Flags: review?(bent.mozilla)
>+  MOZ_ASSERT(!mWindow || mWindow->IsInnerWindow());
MOZ_ASSERT_IF(mWindow, mWindow->IsInnerWindow());
I prefer to not use _IF. It tends to make code harder to read.
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.
Attachment #657876 - Flags: review?(bent.mozilla) → review+
Attached patch patchSplinter Review
https://hg.mozilla.org/mozilla-central/rev/eb201b1e55fd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.