Bug 203526

Created attachment 382122 [details] Patch

Created attachment 382129 [details] Fix release builds

Created attachment 382136 [details] GTK+ / WPE build fix

Ping reviewers.

Comment on attachment 382136 [details] GTK+ / WPE build fix View in context: https://bugs.webkit.org/attachment.cgi?id=382136&action=review > Source/WebCore/dom/Document.h:1065 > + AbstractEventLoop& eventLoop(); It looks like this is an override? If so, why isn't is marked as final? Also, using the tighter WindowEventLoop typing in the override should work and would actually be beneficial. > Source/WebCore/workers/WorkerGlobalScope.cpp:109 > +{ We should likely assert that this is only called from the context thread. > Source/WebCore/workers/WorkerGlobalScope.h:69 > + AbstractEventLoop& eventLoop() final; Can use WorkerEventLoop return type. > Source/WebCore/worklets/WorkletGlobalScope.h:61 > + AbstractEventLoop& eventLoop() final; Can use tighter type.

(In reply to Chris Dumez from comment #5) > Comment on attachment 382136 [details] > GTK+ / WPE build fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382136&action=review > > > Source/WebCore/dom/Document.h:1065 > > + AbstractEventLoop& eventLoop(); > > It looks like this is an override? If so, why isn't is marked as final? Good point. > Also, using the tighter WindowEventLoop typing in the override should work > and would actually be beneficial. There is no need for that now but I guess we can do that. > > > Source/WebCore/workers/WorkerGlobalScope.cpp:109 > > +{ > > We should likely assert that this is only called from the context thread. Added. > > Source/WebCore/workers/WorkerGlobalScope.h:69 > > + AbstractEventLoop& eventLoop() final; > > Can use WorkerEventLoop return type. Sure. Again, there isn't any need for that. Specific type isn't necessarily better if it's not useful. In fact, I'd argue that it's bad in this case because it would encourage people to add new functions to WorkerEventLoop and not to AbstractEventLoop. > > Source/WebCore/worklets/WorkletGlobalScope.h:61 > > + AbstractEventLoop& eventLoop() final; > > Can use tighter type. Fixed for now but again, I don't think this is a good idea.

(In reply to Ryosuke Niwa from comment #6) > (In reply to Chris Dumez from comment #5) > > Comment on attachment 382136 [details] > > GTK+ / WPE build fix > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=382136&action=review > > > > > Source/WebCore/dom/Document.h:1065 > > > + AbstractEventLoop& eventLoop(); > > > > It looks like this is an override? If so, why isn't is marked as final? > > Good point. > > > Also, using the tighter WindowEventLoop typing in the override should work > > and would actually be beneficial. > > There is no need for that now but I guess we can do that. > > > > > > Source/WebCore/workers/WorkerGlobalScope.cpp:109 > > > +{ > > > > We should likely assert that this is only called from the context thread. > > Added. > > > > Source/WebCore/workers/WorkerGlobalScope.h:69 > > > + AbstractEventLoop& eventLoop() final; > > > > Can use WorkerEventLoop return type. > > Sure. Again, there isn't any need for that. Specific type isn't necessarily > better if it's not useful. In fact, I'd argue that it's bad in this case > because it would encourage people to add new functions to WorkerEventLoop > and not to AbstractEventLoop. > > > > Source/WebCore/worklets/WorkletGlobalScope.h:61 > > > + AbstractEventLoop& eventLoop() final; > > > > Can use tighter type. > > Fixed for now but again, I don't think this is a good idea. One reason I think it is beneficial is perf. If you do document().eventLoop().queueTask() then no virtual table lookup since you get a WindowEventLoop and WindowEventLoop::queueTask() is marked as final.

(In reply to Chris Dumez from comment #7) > > One reason I think it is beneficial is perf. If you do > document().eventLoop().queueTask() then no virtual table lookup since you > get a WindowEventLoop and WindowEventLoop::queueTask() is marked as final. I guess that's a good point although I don't think it really matters. Ugh... now I remember why I went with AbstractEventLoop&. It's because if I used WindowEventLoop then I'd have to include WindowEventLoop.h in Document.h which isn't great (since otherwise the complier can't tell WindowEventLoop is covariant with AbstractEventLoop).

(In reply to Ryosuke Niwa from comment #8) > (In reply to Chris Dumez from comment #7) > > > > One reason I think it is beneficial is perf. If you do > > document().eventLoop().queueTask() then no virtual table lookup since you > > get a WindowEventLoop and WindowEventLoop::queueTask() is marked as final. > > I guess that's a good point although I don't think it really matters. > > Ugh... now I remember why I went with AbstractEventLoop&. It's because if I > used WindowEventLoop then I'd have to include WindowEventLoop.h in > Document.h which isn't great (since otherwise the complier can't tell > WindowEventLoop is covariant with AbstractEventLoop). This would also mean we'd have to export WindowEventLoop.h to WebKit layer as well. I don't think we want to do that. I'm revering back to AbstractEventLoop. It's not worth the trouble & slower compilation time.

Created attachment 382265 [details] Updated per review comments

(In reply to Ryosuke Niwa from comment #10) > Created attachment 382265 [details] > Updated per review comments I don’t see the threading assertion?

(In reply to Chris Dumez from comment #11) > (In reply to Ryosuke Niwa from comment #10) > > Created attachment 382265 [details] > > Updated per review comments > > I don’t see the threading assertion? The isContextThread() one I asked for in queueTask(). Or did I miss it?

(In reply to Chris Dumez from comment #12) > (In reply to Chris Dumez from comment #11) > > (In reply to Ryosuke Niwa from comment #10) > > > Created attachment 382265 [details] > > > Updated per review comments > > > > I don’t see the threading assertion? > > The isContextThread() one I asked for in queueTask(). Or did I miss it? Oh, sorry, I misunderstood what you were asking.

I'm gonna keep the assertion in WorkerGlobalScope::eventLoop too though since it's not safe to call that member function in a wrong thread.

Created attachment 382274 [details] Added an assertion

Comment on attachment 382274 [details] Added an assertion View in context: https://bugs.webkit.org/attachment.cgi?id=382274&action=review r=me with comments > Source/WebCore/dom/Document.cpp:6132 > { We likely want a isMainThread() assertion here too. > Source/WebCore/workers/WorkerEventLoop.cpp:61 > +bool WorkerEventLoop::hasPendingActivity() const Why do we need to override this. Presumably, the default implementation already returns false if you don’t use makePendingActivity / setPendingActivity. > Source/WebCore/workers/WorkerEventLoop.cpp:71 > +void WorkerEventLoop::suspend(ReasonForSuspension) We don’t suspend / resume ActiveDOMObjects on workers. Instead, we suspend the whole thread. As a result, I don’t think the suspend / resume methods do anything here. > Source/WebCore/workers/WorkerEventLoop.h:48 > + WorkerEventLoop(ScriptExecutionContext&); Explicit

(In reply to Chris Dumez from comment #16) > Comment on attachment 382274 [details] > Added an assertion > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382274&action=review > > r=me with comments > > > Source/WebCore/dom/Document.cpp:6132 > > { > > We likely want a isMainThread() assertion here too. Good catch. Added. > > Source/WebCore/workers/WorkerEventLoop.cpp:61 > > +bool WorkerEventLoop::hasPendingActivity() const > > Why do we need to override this. Presumably, the default implementation > already returns false if you don’t use makePendingActivity / > setPendingActivity. Yeah, I don't recall why I added there. Removed. > > Source/WebCore/workers/WorkerEventLoop.cpp:71 > > +void WorkerEventLoop::suspend(ReasonForSuspension) > > We don’t suspend / resume ActiveDOMObjects on workers. Instead, we suspend > the whole thread. As a result, I don’t think the suspend / resume methods do > anything here. Oh, sure. I wrote this patch before your patch was landed so I forgot to reflect that. I'd replace the implementation with ASSERT_NOT_REACHED(). > > Source/WebCore/workers/WorkerEventLoop.h:48 > > + WorkerEventLoop(ScriptExecutionContext&); > > Explicit Done.

Created attachment 382281 [details] Patch for landing

Comment on attachment 382281 [details] Patch for landing Wait for EWS

Comment on attachment 382281 [details] Patch for landing Clearing flags on attachment: 382281 Committed r251792: <https://trac.webkit.org/changeset/251792>

All reviewed patches have been landed. Closing bug.

<rdar://problem/56754436>

Comment on attachment 382281 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=382281&action=review > Source/WebCore/workers/WorkerEventLoop.cpp:36 > + return adoptRef(*new WorkerEventLoop(context)); This fails to call suspendIfNeeded(). > Source/WebCore/workers/WorkerEventLoop.cpp:42 > + return adoptRef(*new WorkerEventLoop(context)); This fails to call suspendIfNeeded().

(In reply to Chris Dumez from comment #23) > Comment on attachment 382281 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382281&action=review > > > Source/WebCore/workers/WorkerEventLoop.cpp:36 > > + return adoptRef(*new WorkerEventLoop(context)); > > This fails to call suspendIfNeeded(). > > > Source/WebCore/workers/WorkerEventLoop.cpp:42 > > + return adoptRef(*new WorkerEventLoop(context)); > > This fails to call suspendIfNeeded(). This got fixed in https://trac.webkit.org/changeset/251934.