Figure out how to deal with mainthread-only atoms in worker events

RESOLVED FIXED in mozilla26

Status

()

Core
DOM: Workers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla26
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Should we have per-thread atom tables? And perhaps store the thread info in atom, and
if for some reason comparing atom from another thread, do slow string
comparison.
(Assignee)

Comment 1

5 years ago
Created attachment 789185 [details] [diff] [review]
v3
Assignee: nobody → bugs
(Assignee)

Comment 3

5 years ago
Comment on attachment 789185 [details] [diff] [review]
v3

Let non-main-thread to deal with strings, not atoms.
This is not super pretty, but we agreed to use strings in workers, for now.
Attachment #789185 - Flags: review?(khuey)
(Assignee)

Comment 4

5 years ago
Note, I assume the setup may need some tweaking while landing more stuff to support
DOM events in workers.
(Assignee)

Updated

5 years ago
Blocks: 853893
No longer blocks: 887239
Comment on attachment 789185 [details] [diff] [review]
v3

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

::: content/events/src/EventTarget.cpp
@@ +40,5 @@
> +  }
> +  return SetEventHandler(nullptr,
> +                         Substring(aType, 2), // Remove "on"
> +                         aHandler, rv);
> +}

Yuck.  Have you done perf testing of code that goes through this method?

::: content/events/src/nsDOMEvent.cpp
@@ +57,5 @@
>                              nsPresContext* aPresContext, nsEvent* aEvent)
>  {
>    SetIsDOMBinding();
>    SetOwner(aOwner);
> +  mIsMainThreadEvent = mOwner || NS_IsMainThread();

Is this going to be true for long?  Why wouldn't worker thread events have owners?
Attachment #789185 - Flags: review?(khuey) → review+
(Assignee)

Comment 6

5 years ago
That mOwner is nsPIDOMWindow, so certainly we won't have that in the workers.
Also, the owner is there just for parent object. In workers we should be able to return null
as parent.
(Assignee)

Comment 7

5 years ago
And that SetEventHandler is for chrome only JS event targets to implement onfoos.
https://hg.mozilla.org/mozilla-central/rev/17c36f363203
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.