Pass per-frame task runners to Workers (when possible) [2nd try]

third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/workers/ParentFrameTaskRunners.h"
+
+#include "core/dom/ContextLifecycleObserver.h"
+#include "core/dom/Document.h"
+#include "wtf/Assertions.h"
+
+namespace blink {
+
+class ParentFrameTaskRunners::Observer final : public GarbageCollected<Observer>, public ContextLifecycleObserver {

[haraken, 2016/08/11 14:57:43]

I'd use LocalFrameLifecycleObserver instead, since ParentFrameTaskRunners is
associated with a frame.

Just help me understand: Why can't you simply make ParentFrameTaskRunners an
observer of frame/document?

[kinuko, 2016/08/11 23:21:37]

Wanted to make it an observer only in in-process cases.  Done for simplicity.

+    USING_GARBAGE_COLLECTED_MIXIN(Observer);
+public:
+    Observer(ParentFrameTaskRunners* owner, Document* document)
+        : ContextLifecycleObserver(document)
+        , m_owner(owner)
+    {
+    }
+
+    void contextDestroyed() override
+    {
+        m_owner->contextDestroyed();
+    }
+
+    DEFINE_INLINE_VIRTUAL_TRACE()
+    {
+        ContextLifecycleObserver::trace(visitor);
+    }
+
+    ParentFrameTaskRunners* m_owner;
+};
+
+ParentFrameTaskRunners::~ParentFrameTaskRunners()
+{
+}
+
+WebTaskRunner* ParentFrameTaskRunners::get(TaskType type)

[haraken, 2016/08/11 14:57:43]

Nit: I'd rename this to ParentFrameTaskRunnerHelper or WorkerTaskRunnerHelper
(for consistency with TaskRunnerHelper)

[kinuko, 2016/08/11 23:21:37]

I slightly prefer the current name as this class actually stores pointers to
task runners while TaskRunnerHelper only provides static methods.  (Could be
convinced if you feel strongly)

+{
+    // For now we only support most basic three task types for workers.
+    // (Probably some unthrottled tasks should be moved to Internal type)

[haraken, 2016/08/11 14:57:42]

Yeah, our plan is to limit the unthrottled task runner to only very special
tasks. Maybe would it make more sense to expose the internal task runner instead
of the unthrottled task runner?

[kinuko, 2016/08/11 23:21:37]

I agree, but wanted to avoid mixing up additional behavioral changes in one
patch.  Done for try.

+    switch (type) {
+    case TaskType::Networking:
+        return m_loadingTaskRunner;
+    case TaskType::Unthrottled:
+        return m_unthrottledTaskRunner;
+    case TaskType::Timer:
+        return m_timerTaskRunner;
+    default:
+        NOTREACHED();
+    }
+    return nullptr;
+}
+
+ParentFrameTaskRunners::ParentFrameTaskRunners(Document* document)

[haraken, 2016/08/11 14:57:43]

Would it make more sense to change the parameter from Document to LocalFrame?

[kinuko, 2016/08/11 23:21:37]

For out-of-process workers taking LocalFrame feels a bit weird, but we just pass
nullptr for those workers for now anyways... so might be fine.  Done.

+    : m_unthrottledTaskRunner(TaskRunnerHelper::getUnthrottledTaskRunner(document))
+    , m_timerTaskRunner(TaskRunnerHelper::getTimerTaskRunner(document))
+    , m_loadingTaskRunner(TaskRunnerHelper::getLoadingTaskRunner(document))
+{
+    if (!document)
+        return;
+
+    m_observer = new Observer(this, document);
+    DCHECK(document->isContextThread());
+}
+
+void ParentFrameTaskRunners::contextDestroyed()

[haraken, 2016/08/11 14:57:43]

Wouldn't contextDestroyed() (called by the main thread) cause a threading race
with get() (called by the worker thread)?

[kinuko, 2016/08/11 23:21:36]

Done.

+{
+    Document* document = nullptr;
+    m_unthrottledTaskRunner = TaskRunnerHelper::getUnthrottledTaskRunner(document);

[haraken, 2016/08/11 14:57:43]

Note that these APIs are deprecated. Use TaskRunnerHelper::get() instead.

[kinuko, 2016/08/11 23:21:37]

Yep, I cut corners before I get initial feedback.

+    m_timerTaskRunner = TaskRunnerHelper::getTimerTaskRunner(document);
+    m_loadingTaskRunner = TaskRunnerHelper::getLoadingTaskRunner(document);
+}
+
+} // namespace blink