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

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) 2009 Adam Barth. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
@@ skipping 33 matching lines @@
 #include "core/loader/DocumentLoader.h"
 #include "core/loader/FormSubmission.h"
 #include "core/loader/FrameLoadRequest.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/loader/FrameLoaderStateMachine.h"
 #include "core/page/Page.h"
 #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"
 #include "wtf/CurrentTime.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 namespace blink {
 
 namespace {
@@ skipping 271 matching lines @@
       : ScheduledNavigation(0, document, replacesCurrentItem, true),
         m_submission(submission) {
     DCHECK(m_submission->form());
   }
 
   Member<FormSubmission> m_submission;
 };
 
 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);
   }
 }
 
 bool NavigationScheduler::locationChangePending() {
   return m_redirect && m_redirect->isLocationChange();
 }
 
 bool NavigationScheduler::isNavigationScheduledWithin(double interval) const {
@@ skipping 153 matching lines @@
   cancel();
   m_redirect = redirect;
   startTimer();
 }
 
 void NavigationScheduler::startTimer() {
   if (!m_redirect)
     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();
 }
 
 DEFINE_TRACE(NavigationScheduler) {
   visitor->trace(m_frame);
   visitor->trace(m_redirect);
 }
 
 }  // namespace blink