Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc

content/browser/dom_storage/dom_storage_task_runner.cc

Index: content/browser/dom_storage/dom_storage_task_runner.cc
diff --git a/content/browser/dom_storage/dom_storage_task_runner.cc b/content/browser/dom_storage/dom_storage_task_runner.cc
index ba2842802d4318a6f5a4d4099d756c3d7b41a1bc..a98c431c93e07de4ed188970d0e7c5183dfd7450 100644
--- a/content/browser/dom_storage/dom_storage_task_runner.cc
+++ b/content/browser/dom_storage/dom_storage_task_runner.cc
@@ -7,16 +7,11 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/tracked_objects.h"
 
 namespace content {
 
-// DOMStorageTaskRunner
-
-bool DOMStorageTaskRunner::RunsTasksOnCurrentThread() const {
-  return IsRunningOnSequence(PRIMARY_SEQUENCE);
-}
-
 // DOMStorageWorkerPoolTaskRunner
 
 DOMStorageWorkerPoolTaskRunner::DOMStorageWorkerPoolTaskRunner(
@@ -28,11 +23,17 @@ DOMStorageWorkerPoolTaskRunner::DOMStorageWorkerPoolTaskRunner(
       sequenced_worker_pool_(sequenced_worker_pool),
       primary_sequence_token_(primary_sequence_token),
       commit_sequence_token_(commit_sequence_token) {
+  primary_sequence_checker_.DetachFromSequence();
+  commit_sequence_checker_.DetachFromSequence();
 }
 
 DOMStorageWorkerPoolTaskRunner::~DOMStorageWorkerPoolTaskRunner() {
 }
 
+bool DOMStorageWorkerPoolTaskRunner::RunsTasksOnCurrentThread() const {
+  return sequenced_worker_pool_->RunsTasksOnCurrentThread();

[michaeln, 2016/09/12 22:49:17]

i think you've changed the semantics of this method? it's supposed to be
equivalent to IsRunningOnPrimarySequence()

[fdoray, 2016/09/13 14:24:13]

This method is part of the TaskRunner interface
https://cs.chromium.org/chromium/src/base/task_runner.h?l=82 "Returns true if
the current thread is a thread on which a task may be run, and false if no task
will be run on the current thread."

Derived classes shouldn't have a different description for this method. Can you
point me to a call site where having an implementation that complies with the
description in task_runner.h doesn't work?

[michaeln, 2016/09/14 00:35:12]

The base class is too abstract to provide a full description of its various
concrete impls.

hmmm... if we can't provide a useful impl for this concrete class, maybe just
  return true;

Given that infrastructure no longer provides the ability to check if we're
actually running on the desired sequence in a release build, i suppose we have
no choice but to alter the semantics of this method of this class. Since the
comment in the base class says its valid to always return true, i'd vote to do
that so its more clear its not actually a useful method of this implementation
of taskrunner. Fortunately, I don't see any dom_storage related callsites where
it matters.

You asked for a ptr to a call site where a more discerning impl would matter,
here's one outside of the dom_storage use case...
  base_session_service.cc line 37

If the method always returns true (a fully description compliant impl), that
callsite will never branch.

If the runner is backed by a logical sequence and the method returns true when
on a different sequence (which i think is the best we can do know based on what
you said), this code would do the wrong thing.

[fdoray, 2016/09/14 13:23:53]

Done
As code is migrated to base/task_scheduler, more tasks will run on sequences
rather than specific thread. In that world, I believe that
TaskRunner::RunsTasksOnCurrentThread (with its current description) isn't
helpful and should be removed.

It could be replaced with:

// Returns true iff the current task can't run at the same
// time as another task posted to this TaskRunner. In
// particular, will return true if the current task was posted
// to this TaskRunner.
bool IsSequencedWithCurrentTask() const = 0;

But all of this won't be done immediately.

+}
+
 bool DOMStorageWorkerPoolTaskRunner::PostDelayedTask(
     const tracked_objects::Location& from_here,
     const base::Closure& task,
@@ -62,10 +63,12 @@ bool DOMStorageWorkerPoolTaskRunner::PostShutdownBlockingTask(
       base::SequencedWorkerPool::BLOCK_SHUTDOWN);
 }
 
-bool DOMStorageWorkerPoolTaskRunner::IsRunningOnSequence(
-    SequenceID sequence_id) const {
-  return sequenced_worker_pool_->IsRunningSequenceOnCurrentThread(

[michaeln, 2016/09/12 22:49:17]

wdyt about changing the impl w/o changing the public facing api of this class?
seems like you could accomplish the switchover to sequence checker entirely in
this method body?

if (id == PRI)
  return primary_checker.CalledOnValidThread();
return commit_checker.CalledOnValidThread();

[fdoray, 2016/09/13 14:24:13]

SequenceChecker::CalledOnValidThread() always returns true in non-DCHECK builds.
I prefer having AssertIsRunningOn(Primary|Commit)Sequence and no
IsRunningOn(Primary|Commit)Sequence to prevent such use cases:

if (dom_storage_task_runner->IsRunningOnPrimarySequence()) {
  // This will always run in non-DCHECK builds !!!
}

[michaeln, 2016/09/14 00:35:12]

Surprising? Ok, I agree given the lack of the actual functionality, we need to
take away the methods that promise to provide that functionality. I see the
Checker class is a debug build only kind of utility.
Please remove this comment from the CL description, maybe mention that
SequenceChecker only works properly in builds with DCHECKs enabled.

[fdoray, 2016/09/14 13:23:53]

Done. I rewrote the last paragraph of the CL description.

-      IDtoToken(sequence_id));
+void DOMStorageWorkerPoolTaskRunner::AssertIsRunningOnPrimarySequence() const {
+  DCHECK(primary_sequence_checker_.CalledOnValidSequence());
+}
+
+void DOMStorageWorkerPoolTaskRunner::AssertIsRunningOnCommitSequence() const {
+  DCHECK(commit_sequence_checker_.CalledOnValidSequence());
 }
 
 scoped_refptr<base::SequencedTaskRunner>
@@ -91,6 +94,10 @@ MockDOMStorageTaskRunner::MockDOMStorageTaskRunner(
 MockDOMStorageTaskRunner::~MockDOMStorageTaskRunner() {
 }
 
+bool MockDOMStorageTaskRunner::RunsTasksOnCurrentThread() const {
+  return task_runner_->RunsTasksOnCurrentThread();
+}
+
 bool MockDOMStorageTaskRunner::PostDelayedTask(
     const tracked_objects::Location& from_here,
     const base::Closure& task,
@@ -105,8 +112,12 @@ bool MockDOMStorageTaskRunner::PostShutdownBlockingTask(
   return task_runner_->PostTask(from_here, task);
 }
 
-bool MockDOMStorageTaskRunner::IsRunningOnSequence(SequenceID) const {
-  return task_runner_->RunsTasksOnCurrentThread();
+void MockDOMStorageTaskRunner::AssertIsRunningOnPrimarySequence() const {
+  DCHECK(RunsTasksOnCurrentThread());
+}
+
+void MockDOMStorageTaskRunner::AssertIsRunningOnCommitSequence() const {
+  DCHECK(RunsTasksOnCurrentThread());
 }
 
 scoped_refptr<base::SequencedTaskRunner>