Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
657953004:
content: Add RendererTaskQueueSelector. (Closed)
Descriptioncontent: Add RendererTaskQueueSelector.
This CL introduces a RendererTaskQueueSelector, which will be used by
the upcoming RendererScheduler. A RendererTaskQueueSelector will be queried
by the RenderScheduler's TaskQueueManager whenever it needs to select the
next task to run. The RendererTaskQueueSelector chooses which queue the
TaskQueueManager should service based on both priority and task age.
Queues can be moved between priority levels or disabled at any time,
which will be employed by the RendererScheduler when triggered by external
signals.
Design doc: https://docs.google.com/a/chromium.org/document/d/16f_RIhZa47uEK_OdtTgzWdRU0RFMTQWMpEWyWXIpXUo/edit
BUG=391005
Committed: https://crrev.com/c5d76512415f21e9ae9b28acef97fa41efbed7ec
Cr-Commit-Position: refs/heads/master@{#302007}
Patch Set 1 : #
Total comments: 25
Patch Set 2 : Address review comments. #
Total comments: 27
Patch Set 3 : Address Daniel's comments, without file rename #Patch Set 4 : Update with files renamed #
Total comments: 2
Patch Set 5 : Fix comment #
Total comments: 2
Patch Set 6 : Fix unittest std::vector initialization #Patch Set 7 : Add CONTENT_EXPORT #Patch Set 8 : Add CONTENT_EXPORT to TaskQueueSelector. #Patch Set 9 : Use NON_EXPORTED_BASE #
Messages
Total messages: 43 (15 generated)
Patchset #1 (id:1) has been deleted
rmcilroy@chromium.org changed reviewers: + sievers@chromium.org
Daniel, this is the first part of the RendererScheduler, PTAL, thanks!
alexclarke@google.com changed reviewers: + alexclarke@google.com
https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:58: return queueB->front() < queueA->front(); +1 on this helper method. maybe add a comment explaining why this is correct. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:36: kFirstQueuePriority = kControlPriority, Is this used anymore? I had a look in https://codereview.chromium.org/664963002/ and the only reference seems to be in the header. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:53: virtual void RegisterWorkQueues( nit: the style guide says "For clarity, use exactly one of override, final, or virtual when declaring an override" http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:59: bool IsOlder(const base::TaskQueue* queueA, While I think you have reasonable test coverage, I'd note that if these methods were protected it would be possible to unit test these functions directly, which usually results in more thorough testing.
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
Thanks Ross, the test looks great! https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:27: queue_priorities_[kControlPriority].clear(); Should we just clear all the priorities before adding everything to normal so we don't have to list the other priorities explicitly here? https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:49: static const QueuePriority kPriorities[] = { It would be safer just to iterate from 0 to kQueuePriorityCount instead of having a separate list here. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:70: // Note: the comparison between |chosen_queue| and |queue_index| is correct This comment should probably be in IsOlder. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:108: normal_queue_starvation_count_ = 0; Do we want to reset this when running something from kBestEffortPriority too? If so, the variable name is a little misleading. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:42: // Set the priority of |queue_index| to |set_priority|. s/set_priority/priority/ https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:59: bool IsOlder(const base::TaskQueue* queueA, nit: could be static. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector_unittest.cc (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:10: #include "testing/gmock/include/gmock/gmock.h" nit: I keep hearing rumors that gmock is deprecated, but looks like new code is still using it. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:33: void PushTasks(std::vector<base::PendingTask> tasks, nit: |tasks| could be a const ref.
Thanks for the reviews! https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:27: queue_priorities_[kControlPriority].clear(); On 2014/10/27 11:23:02, Sami wrote: > Should we just clear all the priorities before adding everything to normal so we > don't have to list the other priorities explicitly here? Done. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:49: static const QueuePriority kPriorities[] = { On 2014/10/27 11:23:02, Sami wrote: > It would be safer just to iterate from 0 to kQueuePriorityCount instead of > having a separate list here. Done. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:58: return queueB->front() < queueA->front(); On 2014/10/27 10:40:51, alexclarke wrote: > +1 on this helper method. > > maybe add a comment explaining why this is correct. Done. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:70: // Note: the comparison between |chosen_queue| and |queue_index| is correct On 2014/10/27 11:23:02, Sami wrote: > This comment should probably be in IsOlder. Done. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:108: normal_queue_starvation_count_ = 0; On 2014/10/27 11:23:02, Sami wrote: > Do we want to reset this when running something from kBestEffortPriority too? If > so, the variable name is a little misleading. I think we want to reset this anytime we run something with lower than high priority (since it means we gave normal something lower a chance even if it didn't have anything to run). Renamed to starvation_count_ https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:36: kFirstQueuePriority = kControlPriority, On 2014/10/27 10:40:51, alexclarke wrote: > Is this used anymore? I had a look in > https://codereview.chromium.org/664963002/ and the only reference seems to be in > the header. It's now used in RendererSchedulerSelector::NextPriority() https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:42: // Set the priority of |queue_index| to |set_priority|. On 2014/10/27 11:23:02, Sami wrote: > s/set_priority/priority/ Done. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:53: virtual void RegisterWorkQueues( On 2014/10/27 10:40:51, alexclarke wrote: > nit: the style guide says "For clarity, use exactly one of override, final, or > virtual when declaring an override" > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Done. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:59: bool IsOlder(const base::TaskQueue* queueA, On 2014/10/27 11:23:02, Sami wrote: > nit: could be static. Done. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:59: bool IsOlder(const base::TaskQueue* queueA, On 2014/10/27 10:40:51, alexclarke wrote: > While I think you have reasonable test coverage, I'd note that if these methods > were protected it would be possible to unit test these functions directly, which > usually results in more thorough testing. I'd prefer not to test the internals like these - IMO that just makes it more difficult to change how things work internally and I think the testing covers these functions in any case. However, if you strongly disagree I'm happy to do this. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector_unittest.cc (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:10: #include "testing/gmock/include/gmock/gmock.h" On 2014/10/27 11:23:02, Sami wrote: > nit: I keep hearing rumors that gmock is deprecated, but looks like new code is > still using it. I'm only using this for testing::ElementsAre(). If you want me to remove this let me know. https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:33: void PushTasks(std::vector<base::PendingTask> tasks, On 2014/10/27 11:23:02, Sami wrote: > nit: |tasks| could be a const ref. Done.
Patchset #2 (id:40001) has been deleted
Thanks, lgtm! https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector_unittest.cc (right): https://codereview.chromium.org/657953004/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:10: #include "testing/gmock/include/gmock/gmock.h" On 2014/10/27 13:08:06, rmcilroy wrote: > On 2014/10/27 11:23:02, Sami wrote: > > nit: I keep hearing rumors that gmock is deprecated, but looks like new code > is > > still using it. > > I'm only using this for testing::ElementsAre(). If you want me to remove this > let me know. I think that's fine to use it here. I couldn't find any official position statement either way.
https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:76: for (int queue_index : queue_priorities_[priority]) { Regarding the implementation, isn't it simpler to just maintain one ordered list of tasks queue indices (ordered by priority)? https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:5: #ifndef CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_POLICY_H_ CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_POLICY_H_ ^^ SELECTOR https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:15: // A RendererSchedulerSelector is a TaskQueueSelector which is used by the nit: Why is it not called RendererTaskQueueSelector then? :) https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:17: // circumstances. nit: "priorities" rather than "circumstances"? https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:24: kControlPriority, nit: CONTROL_PRIORITY etc. (macro-style) for Chromium (http://www.chromium.org/developers/coding-style) https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:40: virtual ~RendererSchedulerSelector(); nit: ~RendererSchedulerSelector() override; https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:83: size_t starvation_count_; nit: DISALLOW_COPY_AND_ASSIGN(...) https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector_unittest.cc (right): https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:19: selector_(), nit: default constructor invocations for selector_ and tasks_queues_ are redundant https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:21: virtual ~RendererSchedulerSelectorTest() {} nit: ~RendererSchedulerSelectorTest() override {} https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:41: std::vector<size_t> SelectedQueueOrder() { nit: maybe rename this to PopTasks()? Then it would match the above but more importantly the side effect would be obvious and one would be less likely to add a bad test below (that wants to test disabled queues for example but really ends up repeatedly checking an already empty queue). https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:54: virtual void SetUp() { nit: void SetUp() override (or final) https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:55: std::vector<const base::TaskQueue*> const_task_queues_; nit: no trailing '_' i.e. 'const_task_queues' https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:58: make_scoped_ptr(new base::TaskQueue()); nit: scoped_ptr<base::TaskQueue> task_queue(new base::TaskQueue()); https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:65: size_t kTaskQueueCount = 5; nit: const
Thanks for the review Daniel. All comments addressed. I had to rename the files, so I uploaded patch set 3 with all the changes apart from the rename to make reviewing easier. Patch set 4 includes all of the changes and the renamed files. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:76: for (int queue_index : queue_priorities_[priority]) { On 2014/10/28 01:21:00, sievers wrote: > Regarding the implementation, isn't it simpler to just maintain one ordered list > of tasks queue indices (ordered by priority)? This wouldn't quite work as we want. We want to have multiple queues that have the same priority, and choose from a particular priority based on task age of all the queues with that priority. A simple ordered list would give each queue a different priority depending on it's position in the list, which would make it difficult to do this task-age choice for queue's which are meant to be of the same priority. We also want different starving semantics for each priority - control should starve everything, but high should not starve normal, and best effort is allowed to be starved by any other queue. Again I think this would be tricky to incorporate in a single ordered list. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:5: #ifndef CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_POLICY_H_ On 2014/10/28 01:21:00, sievers wrote: > CONTENT_RENDERER_SCHEDULER_RENDERER_SCHEDULER_POLICY_H_ > ^^ SELECTOR Changed to CONTENT_RENDERER_SCHEDULER_RENDERER_TASK_QUEUE_SELECTOR_H_ given your comment below. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:15: // A RendererSchedulerSelector is a TaskQueueSelector which is used by the On 2014/10/28 01:21:01, sievers wrote: > nit: Why is it not called RendererTaskQueueSelector then? :) Good point, done :). https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:17: // circumstances. On 2014/10/28 01:21:01, sievers wrote: > nit: "priorities" rather than "circumstances"? dropped "depending on circumstances" entirely https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:24: kControlPriority, On 2014/10/28 01:21:00, sievers wrote: > nit: CONTROL_PRIORITY etc. (macro-style) for Chromium > (http://www.chromium.org/developers/coding-style) Done. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:40: virtual ~RendererSchedulerSelector(); On 2014/10/28 01:21:00, sievers wrote: > nit: ~RendererSchedulerSelector() override; Done. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector_unittest.cc (right): https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:19: selector_(), On 2014/10/28 01:21:01, sievers wrote: > nit: default constructor invocations for selector_ and tasks_queues_ are > redundant Done. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:21: virtual ~RendererSchedulerSelectorTest() {} On 2014/10/28 01:21:01, sievers wrote: > nit: ~RendererSchedulerSelectorTest() override {} Done. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:41: std::vector<size_t> SelectedQueueOrder() { On 2014/10/28 01:21:01, sievers wrote: > nit: maybe rename this to PopTasks()? Then it would match the above but more > importantly the side effect would be obvious and one would be less likely to add > a bad test below (that wants to test disabled queues for example but really ends > up repeatedly checking an already empty queue). Good point, Done. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:54: virtual void SetUp() { On 2014/10/28 01:21:01, sievers wrote: > nit: void SetUp() override (or final) Done. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:55: std::vector<const base::TaskQueue*> const_task_queues_; On 2014/10/28 01:21:01, sievers wrote: > nit: no trailing '_' i.e. 'const_task_queues' Done. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:58: make_scoped_ptr(new base::TaskQueue()); On 2014/10/28 01:21:01, sievers wrote: > nit: scoped_ptr<base::TaskQueue> task_queue(new base::TaskQueue()); Done. https://codereview.chromium.org/657953004/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector_unittest.cc:65: size_t kTaskQueueCount = 5; On 2014/10/28 01:21:01, sievers wrote: > nit: const Done.
lgtm https://codereview.chromium.org/657953004/diff/100001/content/renderer/schedu... File content/renderer/scheduler/renderer_task_queue_selector.h (right): https://codereview.chromium.org/657953004/diff/100001/content/renderer/schedu... content/renderer/scheduler/renderer_task_queue_selector.h:15: // A RendererSchedulerSelector is a TaskQueueSelector which is used by the nit: RendererSchedulerSelector -> RendererTaskQueueSelector
On 2014/10/28 17:40:12, sievers wrote: > lgtm > > https://codereview.chromium.org/657953004/diff/100001/content/renderer/schedu... > File content/renderer/scheduler/renderer_task_queue_selector.h (right): > > https://codereview.chromium.org/657953004/diff/100001/content/renderer/schedu... > content/renderer/scheduler/renderer_task_queue_selector.h:15: // A > RendererSchedulerSelector is a TaskQueueSelector which is used by the > nit: RendererSchedulerSelector -> RendererTaskQueueSelector oh you might want to update the cl description also to reflect the class name change
Thanks for the review! > oh you might want to update the cl description also to reflect the class name change Done. https://codereview.chromium.org/657953004/diff/100001/content/renderer/schedu... File content/renderer/scheduler/renderer_task_queue_selector.h (right): https://codereview.chromium.org/657953004/diff/100001/content/renderer/schedu... content/renderer/scheduler/renderer_task_queue_selector.h:15: // A RendererSchedulerSelector is a TaskQueueSelector which is used by the On 2014/10/28 17:40:12, sievers wrote: > nit: RendererSchedulerSelector -> RendererTaskQueueSelector Oops, done, thanks!
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657953004/120001
The CQ bit was unchecked by rmcilroy@chromium.org
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657953004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
https://codereview.chromium.org/657953004/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_task_queue_selector_unittest.cc (right): https://codereview.chromium.org/657953004/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_task_queue_selector_unittest.cc:71: PushTasks(tasks, std::vector<size_t>{4, 3, 2, 1, 0}); Turns out we ended up banning uniform initializers for now :( The build error looks to be caused by them.
https://codereview.chromium.org/657953004/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_task_queue_selector_unittest.cc (right): https://codereview.chromium.org/657953004/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_task_queue_selector_unittest.cc:71: PushTasks(tasks, std::vector<size_t>{4, 3, 2, 1, 0}); On 2014/10/28 18:31:48, Sami wrote: > Turns out we ended up banning uniform initializers for now :( The build error > looks to be caused by them. Yeah I just noticed this, thanks. I'll update it tomorrow to replace them.
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657953004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657953004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2014/10/29 16:11:44, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_chromium_compile_dbg on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) Looks like we need to use NON_EXPORTED_BASE with the class that failed to build because the interface comes from Blink.
On 2014/10/29 18:17:42, Sami wrote: > On 2014/10/29 16:11:44, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > win_chromium_compile_dbg on tryserver.chromium.win > > > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > > Looks like we need to use NON_EXPORTED_BASE with the class that failed to build > because the interface comes from Blink. The class it fails to compile is subclassing TaskQueueSelector, which is in content, not in blink. So I think the correct solution is to add CONTENT_EXPORT on TaskQueueSelector (which seems like it should be given thet TaskQueueManager is CONTENT_EXPORT and can't be used without TaskQueueSelector). WDYT is this right?
On 2014/10/29 23:34:27, rmcilroy wrote: > On 2014/10/29 18:17:42, Sami wrote: > > On 2014/10/29 16:11:44, I haz the power (commit-bot) wrote: > > > Try jobs failed on following builders: > > > win_chromium_compile_dbg on tryserver.chromium.win > > > > > > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > > > > Looks like we need to use NON_EXPORTED_BASE with the class that failed to > build > > because the interface comes from Blink. > > The class it fails to compile is subclassing TaskQueueSelector, which is in > content, not in blink. So I think the correct solution is to add CONTENT_EXPORT > on TaskQueueSelector (which seems like it should be given thet TaskQueueManager > is CONTENT_EXPORT and can't be used without TaskQueueSelector). WDYT is this > right? If you can hide the subclass like Sami suggested that would be better. It's a bit unfortunate that RendererTaskQueueSelector has to be exported at all. It's just because of how we link those unit tests for the component build, right? (content being a dll that the test executable links against).
On 2014/10/29 23:43:11, sievers wrote: > On 2014/10/29 23:34:27, rmcilroy wrote: > > On 2014/10/29 18:17:42, Sami wrote: > > > On 2014/10/29 16:11:44, I haz the power (commit-bot) wrote: > > > > Try jobs failed on following builders: > > > > win_chromium_compile_dbg on tryserver.chromium.win > > > > > > > > > > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > > > > > > Looks like we need to use NON_EXPORTED_BASE with the class that failed to > > build > > > because the interface comes from Blink. > > > > The class it fails to compile is subclassing TaskQueueSelector, which is in > > content, not in blink. So I think the correct solution is to add > CONTENT_EXPORT > > on TaskQueueSelector (which seems like it should be given thet > TaskQueueManager > > is CONTENT_EXPORT and can't be used without TaskQueueSelector). WDYT is this > > right? > > If you can hide the subclass like Sami suggested that would be better. ^^ err hide the base class > It's a bit unfortunate that RendererTaskQueueSelector has to be exported at all. > It's just because of how we link those unit tests for the component build, > right? (content being a dll that the test executable links against).
Actually I want to extend this in the SchedulerFuzzer On 29 Oct 2014 23:43, <sievers@chromium.org> wrote: > On 2014/10/29 23:34:27, rmcilroy wrote: > >> On 2014/10/29 18:17:42, Sami wrote: >> > On 2014/10/29 16:11:44, I haz the power (commit-bot) wrote: >> > > Try jobs failed on following builders: >> > > win_chromium_compile_dbg on tryserver.chromium.win >> > > >> > >> > > (http://build.chromium.org/p/tryserver.chromium.win/ > builders/win_chromium_compile_dbg/builds/29432) > >> > >> > Looks like we need to use NON_EXPORTED_BASE with the class that failed >> to >> build >> > because the interface comes from Blink. >> > > The class it fails to compile is subclassing TaskQueueSelector, which is >> in >> content, not in blink. So I think the correct solution is to add >> > CONTENT_EXPORT > >> on TaskQueueSelector (which seems like it should be given thet >> > TaskQueueManager > >> is CONTENT_EXPORT and can't be used without TaskQueueSelector). WDYT is >> this >> right? >> > > If you can hide the subclass like Sami suggested that would be better. > It's a bit unfortunate that RendererTaskQueueSelector has to be exported > at all. > It's just because of how we link those unit tests for the component build, > right? (content being a dll that the test executable links against). > > > > https://codereview.chromium.org/657953004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> If you can hide the subclass like Sami suggested that would be better. > It's a bit unfortunate that RendererTaskQueueSelector has to be exported at all. > It's just because of how we link those unit tests for the component build, > right? (content being a dll that the test executable links against). Yes exactly, it's just for the component build unit tests. Ok sounds good, I've changed it to hide the base class. > Actually I want to extend this in the SchedulerFuzzer This shouldn't be an issue unless you are building the SchedulerFuzzer outside of content, right? You should be able to just use NON_EXPORTED_BASE as here I think?
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657953004/220001
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c5d76512415f21e9ae9b28acef97fa41efbed7ec Cr-Commit-Position: refs/heads/master@{#302007} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
