TaskScheduler [3/9] Task and Sequence

base/task_scheduler/sequence_sort_key.h

+// 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.
+
+#ifndef BASE_TASK_SCHEDULER_SEQUENCE_SORT_KEY_H_
+#define BASE_TASK_SCHEDULER_SEQUENCE_SORT_KEY_H_
+
+#include "base/base_export.h"
+#include "base/gtest_prod_util.h"
+#include "base/task_scheduler/task_traits.h"
+#include "base/time/time.h"
+
+namespace base {
+namespace internal {
+
+// A SequenceSortKey determines the position of a Sequence in a PriorityQueue.
+// Contrary to a Sequence, which can be modified when it is inside a
+// PriorityQueue, a SequenceSortKey is immutable.
+class BASE_EXPORT SequenceSortKey {
+ public:
+  SequenceSortKey();
+  SequenceSortKey(TaskPriority priority, TimeTicks next_task_post_time);

[gab, 2016/02/18 03:00:46]

Add comments on constructors to highlight differences between the two.

[fdoray, 2016/02/18 14:56:12]

Default constructor is not needed.

+
+  // Returns true if the the sequence associated with this sort key must run
+  // after the sequence associated with |other|.

[fdoray, 2016/02/18 01:46:11]

Or, more simply:

Indicates whether the priority of this sort key is less than the priority of
|other|.

[gab, 2016/02/18 03:00:46]

Or more simply: no comment as this is implicit from the definition of the class?

[fdoray, 2016/02/18 14:56:12]

Done.

+  bool operator<(const SequenceSortKey& other) const;
+
+  // Used for testing.

[gab, 2016/02/18 03:00:46]

Wouldn't bother with this comment.

[fdoray, 2016/02/18 14:56:12]

Done.

+  bool operator==(const SequenceSortKey& other) const;
+
+  TaskPriority priority() const { return priority_; }
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(TaskSchedulerSequenceTest, GetSequenceSortKey);

[gab, 2016/02/18 03:00:46]

Let's make this a struct with public members, doesn't matter since it's an
immutable structure with const members anyways.

Then we don't need getters, nor friending.

(note style says that struct members don't end in with a _ suffix -- just in
case to avoid a second nit round on that)

[fdoray, 2016/02/18 14:56:12]

As explained below, the members can't be const. I don't want non-const public
members, because it would no longer be guaranteed that a SequenceSortKey is
never modified.

+
+  // Highest task priority in the sequence at the time this sort key is created.

[fdoray, 2016/02/18 01:46:11]

*was created?

[gab, 2016/02/18 03:00:46]

Yes.

[fdoray, 2016/02/18 14:56:13]

Done.

+  TaskPriority priority_;

[gab, 2016/02/18 03:00:46]

const

[fdoray, 2016/02/18 14:56:12]

Sadly, std::priority_queue needs operator=, which can't exist if there is const
members.

[gab, 2016/02/18 20:22:34]

Ugh :-(... really?! It does in place replacements instead of moving things
around?! Feels we should use pointers in std::priority_queue instead then? I
guess using pointers to avoid copy of a uint8_t and a uint64_t is not required
for performance, but it feels wrong as-is...

g2g and can't think of something right now, but I'd much prefer these being
const and having DISALLOW_COPY_AND_ASSIGN as usual if at all possible..

[gab, 2016/02/18 21:21:16]

Resurfacing this.

[fdoray, 2016/02/18 22:19:53]

Done.

+
+  // Post time of the next task to run in the sequence.

[fdoray, 2016/02/18 01:46:11]

... at the time this sort key was created.

[fdoray, 2016/02/18 14:56:12]

Done.

+  TimeTicks next_task_post_time_;

[gab, 2016/02/18 03:00:46]

const

[fdoray, 2016/02/18 14:56:12]

Same comment as above.

+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_TASK_SCHEDULER_SEQUENCE_SORT_KEY_H_