Delay bookmarks load while the profile is loading.

base/deferred_sequenced_task_runner.cc

+// Copyright (c) 2013 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 "base/deferred_sequenced_task_runner.h"
+
+#include "base/bind.h"
+#include "base/threading/thread_checker.h"

[erikwright (departed), 2013/04/15 17:56:25]

not needed?

[msarda, 2013/04/17 09:53:55]

Done.

+
+namespace base {
+
+DeferredSequencedTaskRunner::DeferredTask::DeferredTask() {
+}
+
+DeferredSequencedTaskRunner::DeferredTask::~DeferredTask() {
+}
+
+DeferredSequencedTaskRunner::DeferredSequencedTaskRunner(
+    const scoped_refptr<SequencedTaskRunner>& target_task_runner)
+    : started_(false),
+      target_task_runner_(target_task_runner) {
+}
+
+DeferredSequencedTaskRunner::~DeferredSequencedTaskRunner() {
+}
+
+bool DeferredSequencedTaskRunner::PostDelayedTask(
+    const tracked_objects::Location& from_here,
+    const Closure& task,
+    TimeDelta delay) {
+  AutoLock lock(lock_);

[erikwright (departed), 2013/04/15 17:56:25]

presumably the same header that has Lock also has AutoLock. Otherwise you need
to add an #include for this too.

[msarda, 2013/04/17 09:53:55]

base/synchronization/lock.h has both Lock and AutoLock

+  if (started_) {
+    DCHECK(deferred_tasks_queue_.empty());

[erikwright (departed), 2013/04/15 17:56:25]

logging.h IIRC

[msarda, 2013/04/17 09:53:55]

Done.

+    return target_task_runner_->PostDelayedTask(from_here, task, delay);
+  }
+
+  QueueDeferredTask(from_here, task, delay, false /* is_non_nestable */);
+  return true;
+}
+
+bool DeferredSequencedTaskRunner::RunsTasksOnCurrentThread() const {
+  return target_task_runner_->RunsTasksOnCurrentThread();
+}
+
+bool DeferredSequencedTaskRunner::PostNonNestableDelayedTask(
+    const tracked_objects::Location& from_here,
+    const Closure& task,
+    TimeDelta delay) {
+  AutoLock lock(lock_);
+  if (started_) {
+    DCHECK(deferred_tasks_queue_.empty());
+    return target_task_runner_->PostDelayedTask(from_here, task, delay);

[erikwright (departed), 2013/04/15 17:56:25]

PostNonNestableDelayedTask?

[msarda, 2013/04/17 09:53:55]

Done.

+  }
+  QueueDeferredTask(from_here, task, delay, true /* is_non_nestable */);
+  return true;
+}
+
+void DeferredSequencedTaskRunner::QueueDeferredTask(
+    const tracked_objects::Location& from_here,
+    const Closure& task,
+    TimeDelta delay,
+    bool is_non_nestable) {
+  DeferredTask deferred_task;
+  deferred_task.posted_from = from_here;
+  deferred_task.task = task;
+  deferred_task.delay = delay;
+  deferred_task.is_non_nestable = is_non_nestable;
+  deferred_tasks_queue_.push_back(deferred_task);
+}
+
+
+void DeferredSequencedTaskRunner::Start() {
+  AutoLock lock(lock_);
+  DCHECK(!started_);

[erikwright (departed), 2013/04/15 17:56:25]

I don't really see any reason to DCHECK in this scenario. Since it could still
be called in production I'd rather have it not DCHECK and see a test that
verifies that it behaves sensibly (i.e., a no-op).

[msarda, 2013/04/17 09:53:55]

Start is a one-time method as per previous review from willchan.

[erikwright (departed), 2013/04/17 14:41:19]

I agree it is a one-time method, but since there is nothing stopping callers
from calling it twice, it would be nice to be clear about what happens and to be
able to test it.

If someone does call it twice, right now it will be a no-op and it will be fine
(unless a test happens to trigger the double-call). But if you later change the
implementation of this method to behave less sensibly when called twice, that
bug will only be caught in production because you don't (can't) have any tests
verifying that the second call is a no-op.

So if you remove the DCHECK, you can add a simple test that verifies the
behaviour.

I'd be perfectly happy to return to the original state ("if (started) { return;
}").

I looked through but I couldn't find willchan's comment on this subject.

[msarda, 2013/04/17 15:29:10]

This is exactly what I did before (for example
https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ...
). I had to change it following willchan's review (see
https://chromiumcodereview.appspot.com/12952005/diff/34002/base/deferred_sequ...
, comment at line 45). The DCHECK ensures that nobody calls it twice now, right?

willchan: I think this is your call - let me know what you prefer here (DCHECK
vs redundant calls).

[willchan no longer on Chromium, 2013/04/17 15:40:19]

My preference hasn't changed, but I also said before that it was personal style
and I wouldn't mandate it. This is one of those places where I don't think it
matters too much and I believe back and forth (especially cross-Atlantic) costs
more in developer velocity than it leads to any benefit in code quality.

[msarda, 2013/04/17 16:24:39]

I'll keep it as is then.

On 2013/04/17 15:40:19, willchan wrote:

[erikwright (departed), 2013/04/17 17:21:17]

I agree that this is far from a critical point in this CL. The code in this case
is visibly safe even if the DCHECK fails.

+  started_ = true;
+  for (std::vector<DeferredTask>::iterator i = deferred_tasks_queue_.begin();
+      i != deferred_tasks_queue_.end();
+      i = deferred_tasks_queue_.erase(i)) {
+    const DeferredTask& task = *i;
+    if (task.is_non_nestable) {
+      target_task_runner_->PostNonNestableDelayedTask(task.posted_from,
+                                                      task.task,
+                                                      task.delay);
+    } else {
+      target_task_runner_->PostDelayedTask(task.posted_from,
+                                           task.task,
+                                           task.delay);
+    }
+  }
+  DCHECK(deferred_tasks_queue_.empty());
+}
+
+}  // namespace base