Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2)

third_party/WebKit/Source/core/loader/NavigationScheduler.cpp

Index: third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
diff --git a/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp b/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
index 923e203a1bb4345eba76d38ebc0c295585e49cf5..993b7831dd8e2e0d003f5c4b00b8050040e9ceeb 100644
--- a/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
+++ b/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
@@ -51,7 +51,6 @@
 #include "platform/Histogram.h"
 #include "platform/SharedBuffer.h"
 #include "platform/UserGestureIndicator.h"
-#include "platform/scheduler/CancellableTaskFactory.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebCachePolicy.h"
 #include "public/platform/WebScheduler.h"
@@ -343,15 +342,12 @@ class ScheduledFormSubmission final : public ScheduledNavigation {
 
 NavigationScheduler::NavigationScheduler(LocalFrame* frame)
     : m_frame(frame),
-      m_navigateTaskFactory(
-          CancellableTaskFactory::create(this,
-                                         &NavigationScheduler::navigateTask)),
       m_frameType(m_frame->isMainFrame()
                       ? WebScheduler::NavigatingFrameType::kMainFrame
                       : WebScheduler::NavigatingFrameType::kChildFrame) {}
 
 NavigationScheduler::~NavigationScheduler() {
-  if (m_navigateTaskFactory->isPending()) {
+  if (m_navigateTaskHandle.isActive()) {
     Platform::current()->currentThread()->scheduler()->removePendingNavigation(
         m_frameType);
   }
@@ -525,28 +521,33 @@ void NavigationScheduler::startTimer() {
     return;
 
   DCHECK(m_frame->page());
-  if (m_navigateTaskFactory->isPending())
+  if (m_navigateTaskHandle.isActive())
     return;
   if (!m_redirect->shouldStartTimer(m_frame))
     return;
 
   WebScheduler* scheduler = Platform::current()->currentThread()->scheduler();
   scheduler->addPendingNavigation(m_frameType);

[nhiroki, 2016/11/16 13:49:32]

Question: addPendingNavigation and removePendingNavigation do nothing in the
current implementation. Are we able to remove them?

It seems that these were added for prioritizing navigation tasks[1]. I'm not
sure they're still useful after per-frame task scheduler and throttling
mechanism are enabled.

[1] https://codereview.chromium.org/1368643002

[haraken, 2016/11/17 05:59:09]

It is calling UpdatePolicy(), right?

However, WebThread::scheduler() should be replaced with the per-frame scheduler.
So if we need the logic, we need to move the logic to WebFrameScheduler.

+alex +sami

[nhiroki, 2016/11/17 06:15:59]

Are you seeing RendererScheduler::AddPendingNavigation? I meant
WebScheduler::addPendingNavigation that has an empty body. According to
codesearch, RendererScheduler(Impl)::AddPendingNavigation is called only from
unittests (RendererSchedulerImplTest), so we could remove them, too?

[haraken, 2016/11/17 06:35:19]

ah, got it.

-  scheduler->loadingTaskRunner()->postDelayedTask(
-      BLINK_FROM_HERE, m_navigateTaskFactory->cancelAndCreate(),
-      m_redirect->delay() * 1000.0);
+
+  // wrapWeakPersistent(this) is safe because a posted task is canceled when the
+  // task handle is destroyed on the dtor of this NavigationScheduler.
+  m_navigateTaskHandle =
+      scheduler->loadingTaskRunner()->postDelayedCancellableTask(

[haraken, 2016/11/16 11:37:20]

Nit: We should move this to TaskRunnerHelper::xxx in a follow up.

[nhiroki, 2016/11/16 13:49:32]

Acknowledged. I have a question related to this cleanup. Can you see my other
comment?

+          BLINK_FROM_HERE, WTF::bind(&NavigationScheduler::navigateTask,
+                                     wrapWeakPersistent(this)),
+          m_redirect->delay() * 1000.0);
 
   InspectorInstrumentation::frameScheduledNavigation(m_frame,
                                                      m_redirect->delay());
 }
 
 void NavigationScheduler::cancel() {
-  if (m_navigateTaskFactory->isPending()) {
+  if (m_navigateTaskHandle.isActive()) {
     Platform::current()->currentThread()->scheduler()->removePendingNavigation(
         m_frameType);
     InspectorInstrumentation::frameClearedScheduledNavigation(m_frame);
   }
-  m_navigateTaskFactory->cancel();
+  m_navigateTaskHandle.cancel();
   m_redirect.clear();
 }