scheduler: Move the Blink scheduler into Blink

scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org

Committed: https://crrev.com/529caa299dee30c413f64ebdd699e5361821f49d
Cr-Commit-Position: refs/heads/master@{#411074}

[Sami, 2016-07-05 17:53:40]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Sami, 2016-07-05 17:55:08]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Sami, 2016-07-05 17:55:09]

skyostil@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[Sami, 2016-07-05 18:01:32]

This isn't at all polished yet, but I wanted to get some feedback from you guys
first:

- How do you feel about the layout under public/platform/scheduler and
Source/platform/scheduler?
- Does the public API look okay?
- Should I move everything into a blink::scheduler namespace (my preference) or
just plain blink?
- Anything else I missed?

I've removed a few plumbing layers here, but I'll leave larger changes for
future patches.

(In case you want to download the patch and Rietvelt won't let you, here's a
link:
https://drive.google.com/open?id=0ByyxMXB38gLDOWlFSUcyREY0Ync)

[haraken, 2016-07-06 01:05:11]

From my point of view...

> - How do you feel about the layout under public/platform/scheduler and
> Source/platform/scheduler?

Looks good.

> - Does the public API look okay?

Looks good.

> - Should I move everything into a blink::scheduler namespace (my preference)
or
> just plain blink?

Yeah, I'd also prefer using a dedicated namespace like blink::scheduler.

> - Anything else I missed?

I'm not sure if we want to introduce too much base/ stuff such as scoped_refptr,
base::Lock to Blink's platform. Should we aim at replacing them with WTF's
equivalents (in a follow-up CL)? Regarding coding style, we can just keep the
Chromium's style as is since Blink's coding style will be gone in the future.

[Sami, 2016-07-06 09:17:13]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org

[Sami, 2016-07-06 17:21:12]

Thanks for looking.

On 2016/07/06 01:05:11, haraken wrote:
> Yeah, I'd also prefer using a dedicated namespace like blink::scheduler.

Great, done.

> I'm not sure if we want to introduce too much base/ stuff such as
scoped_refptr,
> base::Lock to Blink's platform. Should we aim at replacing them with WTF's
> equivalents (in a follow-up CL)?

Right, I'd like to understand the best way to unify things like these going
forward. For example much of the scheduler is built around the base::TaskRunner
interface. It's not compatible with WTF::bind(), so Blink code needs to use it
via a wrapper (WebTaskRunner). The scheduler exports these task runners to
content too, so if we switched to WebTaskRunner then content would need a
wrapper. I'd love to find some common primitive that could be used directly from
all sides.

Re: base::Lock and others, I think we can migrate over time to whichever pattern
other Blink code prefers. One caveat here is that the scheduler needs to be
brought up before WTF is initialized, so that outrules some WTF APIs.

For now I've added DEPS files which whitelist the exact base/ APIs we're using.

[haraken, 2016-07-07 01:21:04]

On 2016/07/06 17:21:12, Sami wrote:
> Thanks for looking.
> 
> On 2016/07/06 01:05:11, haraken wrote:
> > Yeah, I'd also prefer using a dedicated namespace like blink::scheduler.
> 
> Great, done.
> 
> > I'm not sure if we want to introduce too much base/ stuff such as
> scoped_refptr,
> > base::Lock to Blink's platform. Should we aim at replacing them with WTF's
> > equivalents (in a follow-up CL)?
> 
> Right, I'd like to understand the best way to unify things like these going
> forward. For example much of the scheduler is built around the
base::TaskRunner
> interface. It's not compatible with WTF::bind(), so Blink code needs to use it
> via a wrapper (WebTaskRunner). The scheduler exports these task runners to
> content too, so if we switched to WebTaskRunner then content would need a
> wrapper. I'd love to find some common primitive that could be used directly
from
> all sides.
> 
> Re: base::Lock and others, I think we can migrate over time to whichever
pattern
> other Blink code prefers. One caveat here is that the scheduler needs to be
> brought up before WTF is initialized, so that outrules some WTF APIs.
> 
> For now I've added DEPS files which whitelist the exact base/ APIs we're
using.

Yeah, makes sense. If you plan to remove dependency on base/ things that have
equivalents in WTF, this CL overall looks good to me.

Elliott should take a look at this.

[Sami, 2016-07-08 15:51:56]

Bots seem reasonably happy now. Elliott, could you have a look please?

[esprehn, 2016-07-16 01:09:04]

On 2016/07/08 at 15:51:56, skyostil wrote:
> Bots seem reasonably happy now. Elliott, could you have a look please?

Yeah I'll look first thing next week, thanks for working on this!

[esprehn, 2016-07-22 06:12:11]

Wow this is amazing, the layering thing is the most important thing to sort out.
Then we need to figure out what to do about the scoped_refptrs in core. We can
switch the code to WTF in platform/ slowly, but the stuff that sticks into core
would need fixing in this patch.

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right):

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:62:
scoped_refptr<base::SingleThreadTaskRunner> taskRunner() override { return
nullptr; }
you can't use scoped_refptr in core/, this would need to be a RefPtr. Does the
ref counting need to be exposed here?

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/child/web_task_runner_impl.h
(right):

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/web_task_runner_impl.h:48:
scoped_refptr<TaskQueue> task_queue_;
this and others should be RefPtr and WTF::RefCounted, but we can deal with that
after the migration

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/blink_headers.gypi (right):

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/blink_headers.gypi:318:
"platform/scheduler/child/webthread_impl_for_worker_scheduler.h",
I'm not sure the renderer vs child distinction makes sense in blink, but we can
sort that out later

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/scheduler/DEPS (right):

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/scheduler/DEPS:6:
"+base/memory/ref_counted.h",
we'll want to switch to WTF stuff after moving the code, but all of this is fine
to get started.

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/scheduler/DEPS:11:
"+public/web/WebInputEvent.h",
platform actually can't depend on web/

https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/DEPS?...

it's a layering violation since web is above platform, that said we should
actually move WebInputEvent into platform/ it belongs in here. I would do that
first. To make it easier so you don't need to update several hundred files you
can leave a proxy header in web that just includes the one from platform.

[dtapuska, 2016-07-22 14:40:14]

dtapuska@chromium.org changed reviewers:
+ dtapuska@chromium.org

[dtapuska, 2016-07-22 14:40:18]

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/scheduler/DEPS (right):

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/scheduler/DEPS:11:
"+public/web/WebInputEvent.h",
On 2016/07/22 06:12:11, esprehn wrote:
> platform actually can't depend on web/
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/DEPS?...
> 
> it's a layering violation since web is above platform, that said we should
> actually move WebInputEvent into platform/ it belongs in here. I would do that
> first. To make it easier so you don't need to update several hundred files you
> can leave a proxy header in web that just includes the one from platform.

See: https://codereview.chromium.org/2173073003 I've had this in my backlog for
a while.

[Sami, 2016-07-29 17:03:27]

I know it's pain to review this, so thanks a lot for your comments!

I've now removed the public => web dependency thanks to Dave's patch and also by
adding a small interface for logging console warnings about interventions. I was
also able to avoid adding scoped_refptrs in core since the underlying
base::SingleThreadTaskRunner doesn't need to be used there.

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right):

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:62:
scoped_refptr<base::SingleThreadTaskRunner> taskRunner() override { return
nullptr; }
On 2016/07/22 06:12:11, esprehn wrote:
> you can't use scoped_refptr in core/, this would need to be a RefPtr. Does the
> ref counting need to be exposed here?

Turns out it's not needed: I've now made it a raw pointer. Over time I'd like to
somehow merge these into a single object, but I guess that can wait.

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/blink_headers.gypi (right):

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/blink_headers.gypi:318:
"platform/scheduler/child/webthread_impl_for_worker_scheduler.h",
On 2016/07/22 06:12:11, esprehn wrote:
> I'm not sure the renderer vs child distinction makes sense in blink, but we
can
> sort that out later

Agreed that it's a little weird now. Let's figure out what makes more sense once
the dust settles.

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/scheduler/DEPS (right):

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/scheduler/DEPS:6:
"+base/memory/ref_counted.h",
On 2016/07/22 06:12:11, esprehn wrote:
> we'll want to switch to WTF stuff after moving the code, but all of this is
fine
> to get started.

Acknowledged.

https://codereview.chromium.org/2118903002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/scheduler/DEPS:11:
"+public/web/WebInputEvent.h",
On 2016/07/22 14:40:18, dtapuska wrote:
> On 2016/07/22 06:12:11, esprehn wrote:
> > platform actually can't depend on web/
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/DEPS?...
> > 
> > it's a layering violation since web is above platform, that said we should
> > actually move WebInputEvent into platform/ it belongs in here. I would do
that
> > first. To make it easier so you don't need to update several hundred files
you
> > can leave a proxy header in web that just includes the one from platform.
> 
> See: https://codereview.chromium.org/2173073003 I've had this in my backlog
for
> a while.

Thanks a lot Dave! I've now removed this dep.

[Sami, 2016-07-29 17:03:54]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 17:04:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 17:04:19]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-29 17:05:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-29 17:05:51]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED, no build URL)

[Sami, 2016-08-01 13:30:18]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 13:30:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 13:30:37]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-01 13:31:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 13:31:47]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED, no build URL)

[Sami, 2016-08-01 16:46:25]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[Sami, 2016-08-01 18:06:07]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 18:06:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 18:06:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 18:06:40]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[Sami, 2016-08-02 18:13:00]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 18:13:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 18:14:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 18:14:02]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[Sami, 2016-08-02 18:27:59]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 18:28:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 18:28:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 18:28:45]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[Sami, 2016-08-02 18:33:29]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
CQ_INCLUDE_TRYBOTS=linux_precise_blink_rel:linux_blink_rel
==========

[Sami, 2016-08-02 18:33:49]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 18:34:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 18:34:26]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     linux_precise_blink_rel
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-02 18:35:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 18:35:15]

Dry run: CQ has no permission to schedule in bucket
master.linux_precise_blink_rel

[Sami, 2016-08-03 10:38:57]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 10:39:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 10:39:13]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     linux_precise_blink_rel
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-03 10:39:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 10:39:37]

Dry run: CQ has no permission to schedule in bucket
master.linux_precise_blink_rel

[Sami, 2016-08-03 14:42:11]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 14:42:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 14:42:37]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     linux_precise_blink_rel
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-03 14:43:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 14:43:02]

Dry run: CQ has no permission to schedule in bucket
master.linux_precise_blink_rel

[Sami, 2016-08-03 16:50:13]

It appears the bots are finally content. Elliott, another look please?

[haraken, 2016-08-04 01:11:38]

LGTM on my side.

I might want to replace the following base things with WTF's equivalents though.

- std::vector
- std::map
- std::set
- scoped_refptr
- base::Lock
- base::WeakPtr
- base::WeakPtrFactory
- base::ThreadChecker
- base::Closure
- base::trace_event::BlameContext
- base::PlatformThreadId
- base::Thread

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/base/cancelable_closure_holder.h
(right):

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/cancelable_closure_holder.h:5:
#ifndef
THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_CANCELABLE_CLOSURE_HOLDER_H_

Is there any reason you wanted to add the
THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_ prefix?

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h (right):

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:14: namespace
internal {

I'm not sure if we need to add the blink namespace. The three level of nesting
(blink => scheduler => internal) looks a bit too verbose to me.

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h
(right):

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h:48:
base::MessageLoop* message_loop_;

I'm not sure if we want to expose MessageLoop to Blink.

[alex clarke (OOO till 29th), 2016-08-04 09:07:28]

On 2016/08/04 01:11:38, haraken wrote:
> LGTM on my side.
> 
> I might want to replace the following base things with WTF's equivalents
though.
> 
> - std::vector
> - std::map
> - std::set
We can't replace all uses of std::set with wtf::HashSet because a number of them
rely on it being a tree rather than a hash set.

> - scoped_refptr
> - base::Lock
> - base::WeakPtr
> - base::WeakPtrFactory
> - base::ThreadChecker
> - base::Closure
> - base::trace_event::BlameContext
> - base::PlatformThreadId
> - base::Thread
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/platform/scheduler/base/cancelable_closure_holder.h
> (right):
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/scheduler/base/cancelable_closure_holder.h:5:
> #ifndef
> THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_CANCELABLE_CLOSURE_HOLDER_H_
> 
> Is there any reason you wanted to add the
> THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_ prefix?
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h
(right):
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:14:
namespace
> internal {
> 
> I'm not sure if we need to add the blink namespace. The three level of nesting
> (blink => scheduler => internal) looks a bit too verbose to me.
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h
> (right):
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h:48:
> base::MessageLoop* message_loop_;
> 
> I'm not sure if we want to expose MessageLoop to Blink.

[Sami, 2016-08-04 10:12:41]

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/base/cancelable_closure_holder.h
(right):

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/cancelable_closure_holder.h:5:
#ifndef
THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_CANCELABLE_CLOSURE_HOLDER_H_
On 2016/08/04 01:11:38, haraken wrote:
> 
> Is there any reason you wanted to add the
> THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_ prefix?

Assuming we're following Chromium style here I think that's what the include
guard should be. Other (although not all) third party modules follow this
convention:

https://cs.chromium.org/search/?q=ifndef+THIRD_PARTY&sq=package:chromium&type=cs

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h (right):

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:14: namespace
internal {
On 2016/08/04 01:11:38, haraken wrote:
> 
> I'm not sure if we need to add the blink namespace. The three level of nesting
> (blink => scheduler => internal) looks a bit too verbose to me.
> 

These things are internal to the "base" part of the scheduler and because all
things referring to them are already in the blink::scheduler namespace, we never
use the full blink::scheduler::internal notation but instead just internal::*.
Does that alleviate your concern?

I'm not sure what the alternative would be. I wouldn't want to move internal
things to the scheduler namespace since other code might accidentally use them.
Leaving out blink would also be awkward since now we'd have two different
scheduler namespaces (one in blink:: and one in the global scope).

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h
(right):

https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h:48:
base::MessageLoop* message_loop_;
On 2016/08/04 01:11:38, haraken wrote:
> 
> I'm not sure if we want to expose MessageLoop to Blink.

MessageLoop itself is only used inside platform/scheduler, and it would be hard
to factor it out because the message loop is pretty integral to scheduling.

The rest of Blink doesn't need to see or use the MessageLoop. Technically there
are some objects in public/platform/scheduler that expose methods with
MessageLoop::TaskObserver, but no code in Blink needs to use them.

Do you have ideas for improving this?

[haraken, 2016-08-04 13:58:55]

On 2016/08/04 10:12:41, Sami wrote:
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/platform/scheduler/base/cancelable_closure_holder.h
> (right):
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/scheduler/base/cancelable_closure_holder.h:5:
> #ifndef
> THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_CANCELABLE_CLOSURE_HOLDER_H_
> On 2016/08/04 01:11:38, haraken wrote:
> > 
> > Is there any reason you wanted to add the
> > THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_ prefix?
> 
> Assuming we're following Chromium style here I think that's what the include
> guard should be. Other (although not all) third party modules follow this
> convention:
> 
>
https://cs.chromium.org/search/?q=ifndef+THIRD_PARTY&sq=package:chromium&type=cs
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h
(right):
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:14:
namespace
> internal {
> On 2016/08/04 01:11:38, haraken wrote:
> > 
> > I'm not sure if we need to add the blink namespace. The three level of
nesting
> > (blink => scheduler => internal) looks a bit too verbose to me.
> > 
> 
> These things are internal to the "base" part of the scheduler and because all
> things referring to them are already in the blink::scheduler namespace, we
never
> use the full blink::scheduler::internal notation but instead just internal::*.
> Does that alleviate your concern?
> 
> I'm not sure what the alternative would be. I wouldn't want to move internal
> things to the scheduler namespace since other code might accidentally use
them.
> Leaving out blink would also be awkward since now we'd have two different
> scheduler namespaces (one in blink:: and one in the global scope).
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h
> (right):
> 
>
https://codereview.chromium.org/2118903002/diff/180001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.h:48:
> base::MessageLoop* message_loop_;
> On 2016/08/04 01:11:38, haraken wrote:
> > 
> > I'm not sure if we want to expose MessageLoop to Blink.
> 
> MessageLoop itself is only used inside platform/scheduler, and it would be
hard
> to factor it out because the message loop is pretty integral to scheduling.
> 
> The rest of Blink doesn't need to see or use the MessageLoop. Technically
there
> are some objects in public/platform/scheduler that expose methods with
> MessageLoop::TaskObserver, but no code in Blink needs to use them.
> 
> Do you have ideas for improving this?

Yeah, all your points make sense.

[esprehn, 2016-08-06 02:00:23]

lgtm

[Sami, 2016-08-08 09:24:38]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
CQ_INCLUDE_TRYBOTS=linux_precise_blink_rel:linux_blink_rel
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org
CQ_INCLUDE_TRYBOTS=linux_precise_blink_rel:linux_blink_rel
==========

[Sami, 2016-08-08 09:24:38]

skyostil@chromium.org changed reviewers:
+ chcunningham@chromium.org, jochen@chromium.org, sievers@chromium.org

[Sami, 2016-08-08 09:29:08]

Thanks. Adding some owners:

content/: sievers@
media/: chcunningham@
components/: jochen@ (TBR'd since he's out and the changes are trivial)

PTAL.

[no sievers, 2016-08-08 18:45:54]

https://codereview.chromium.org/2118903002/diff/180001/content/child/resource...
File content/child/resource_scheduling_filter.cc (right):

https://codereview.chromium.org/2118903002/diff/180001/content/child/resource...
content/child/resource_scheduling_filter.cc:57:
scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
nit: 'const scoped_refptr<>&'

https://codereview.chromium.org/2118903002/diff/180001/content/child/resource...
content/child/resource_scheduling_filter.cc:60: std::make_pair(id,
std::move(task_runner)));
and probably didn't intend to move the refptr here

https://codereview.chromium.org/2118903002/diff/180001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2118903002/diff/180001/content/child/web_url_...
content/child/web_url_loader_impl.cc:318: void
SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> task_runner);
nit: const-ref here too

https://codereview.chromium.org/2118903002/diff/180001/content/child/web_url_...
content/child/web_url_loader_impl.cc:564: task_runner_ = std::move(task_runner);
here too

[chcunningham, 2016-08-08 23:10:16]

media LGTM

https://codereview.chromium.org/2118903002/diff/180001/media/blink/run_all_un...
File media/blink/run_all_unittests.cc (right):

https://codereview.chromium.org/2118903002/diff/180001/media/blink/run_all_un...
media/blink/run_all_unittests.cc:27: class TestBlinkPlatformSupport :
NON_EXPORTED_BASE(public blink::Platform) {
Is NON_EXPORTED_BASE still needed? We don't seem to export the derived class.

[Sami, 2016-08-09 10:09:42]

Thanks, all addressed.

https://codereview.chromium.org/2118903002/diff/180001/content/child/resource...
File content/child/resource_scheduling_filter.cc (right):

https://codereview.chromium.org/2118903002/diff/180001/content/child/resource...
content/child/resource_scheduling_filter.cc:57:
scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
On 2016/08/08 18:45:54, sievers wrote:
> nit: 'const scoped_refptr<>&' 

Done.

https://codereview.chromium.org/2118903002/diff/180001/content/child/resource...
content/child/resource_scheduling_filter.cc:60: std::make_pair(id,
std::move(task_runner)));
On 2016/08/08 18:45:54, sievers wrote:
> and probably didn't intend to move the refptr here

Done.

https://codereview.chromium.org/2118903002/diff/180001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2118903002/diff/180001/content/child/web_url_...
content/child/web_url_loader_impl.cc:318: void
SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2016/08/08 18:45:54, sievers wrote:
> nit: const-ref here too

Done.

https://codereview.chromium.org/2118903002/diff/180001/content/child/web_url_...
content/child/web_url_loader_impl.cc:564: task_runner_ = std::move(task_runner);
On 2016/08/08 18:45:54, sievers wrote:
> here too

Done.

https://codereview.chromium.org/2118903002/diff/180001/media/blink/run_all_un...
File media/blink/run_all_unittests.cc (right):

https://codereview.chromium.org/2118903002/diff/180001/media/blink/run_all_un...
media/blink/run_all_unittests.cc:27: class TestBlinkPlatformSupport :
NON_EXPORTED_BASE(public blink::Platform) {
On 2016/08/08 23:10:15, chcunningham wrote:
> Is NON_EXPORTED_BASE still needed? We don't seem to export the derived class. 

Good catch, removed.

[Sami, 2016-08-09 10:09:46]

The CQ bit was checked by skyostil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 10:10:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 10:10:24]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     linux_precise_blink_rel
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-09 10:11:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 10:11:03]

Dry run: CQ has no permission to schedule in bucket
master.linux_precise_blink_rel

[no sievers, 2016-08-09 18:22:02]

content lgtm

[Sami, 2016-08-09 18:52:16]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-08-09 18:52:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, chcunningham@chromium.org, haraken@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2118903002/#ps210001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-09 18:53:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 18:53:15]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     linux_precise_blink_rel
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-09 18:54:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 18:54:04]

CQ has no permission to schedule in bucket master.linux_precise_blink_rel

[Sami, 2016-08-10 08:57:50]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org
CQ_INCLUDE_TRYBOTS=linux_precise_blink_rel:linux_blink_rel
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org
CQ_INCLUDE_TRYBOTS=linux_blink_rel
==========

[Sami, 2016-08-10 08:59:16]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-08-10 08:59:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 08:59:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 08:59:43]

Your CL can not be processed by CQ because of:
 * Failed to parse additional trybots

[Sami, 2016-08-10 09:13:37]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org
CQ_INCLUDE_TRYBOTS=linux_blink_rel
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org
==========

[Sami, 2016-08-10 09:14:52]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-08-10 09:15:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 14:16:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 14:16:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sami, 2016-08-10 14:20:44]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-08-10 14:21:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 17:45:09]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org
==========

[commit-bot: I haz the power, 2016-08-10 17:45:14]

Committed patchset #22 (id:210001)

[commit-bot: I haz the power, 2016-08-10 17:47:04]

Description was changed from

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org
==========

to

==========
scheduler: Move the Blink scheduler into Blink

Thanks to the Onion Soup effort it is now feasible to implement the Blink
scheduler inside Blink. This helps to reduce the redundant plumbing of
scheduling data over the content-blink boundary.

BUG=624702
TBR=jochen@chromium.org

Committed: https://crrev.com/529caa299dee30c413f64ebdd699e5361821f49d
Cr-Commit-Position: refs/heads/master@{#411074}
==========

[commit-bot: I haz the power, 2016-08-10 17:47:08]

Patchset 22 (id:??) landed as
https://crrev.com/529caa299dee30c413f64ebdd699e5361821f49d
Cr-Commit-Position: refs/heads/master@{#411074}

[panicker, 2016-08-12 23:57:55]

Hey Sami,
Now that the scheduler is in Blink (awesome!) should it be switched to use
Blink's trace macros (i.e. third_party/WebKit/Source/platform/TraceEvent) as
opposed to (base/trace_event)?


For background, I'm running into compilation issues while trying to tweak long
task instrumentation in the scheduler with these errors about duplicate MACRO
definition:

../../third_party/WebKit/Source/platform/TraceEvent.h:82:9: error:
'TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET' macro redefined
[-Werror,-Wmacro-redefined]
#define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
        ^
../../base/trace_event/trace_event.h:58:9: note: previous definition is here
#define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
        ^

[haraken, 2016-08-13 01:37:12]

On 2016/08/12 23:57:55, Shubhie wrote:
> Hey Sami,
> Now that the scheduler is in Blink (awesome!) should it be switched to use
> Blink's trace macros (i.e. third_party/WebKit/Source/platform/TraceEvent) as
> opposed to (base/trace_event)?
> 
> 
> For background, I'm running into compilation issues while trying to tweak long
> task instrumentation in the scheduler with these errors about duplicate MACRO
> definition:
> 
> ../../third_party/WebKit/Source/platform/TraceEvent.h:82:9: error:
> 'TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET' macro redefined
> [-Werror,-Wmacro-redefined]
> #define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
>         ^
> ../../base/trace_event/trace_event.h:58:9: note: previous definition is here
> #define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
>         ^

platform/ can directly use base/, so it's allowed to use base/trace_event/. That
said, it also makes sense to use Source/platform/TraceEvent/ since they provide
macros for Blink (mainly for core/ and modules/ though).

[Sami, 2016-08-15 10:48:00]

On 2016/08/13 01:37:12, haraken wrote:
> On 2016/08/12 23:57:55, Shubhie wrote:
> > Hey Sami,
> > Now that the scheduler is in Blink (awesome!) should it be switched to use
> > Blink's trace macros (i.e. third_party/WebKit/Source/platform/TraceEvent) as
> > opposed to (base/trace_event)?
> > 
> > 
> > For background, I'm running into compilation issues while trying to tweak
long
> > task instrumentation in the scheduler with these errors about duplicate
MACRO
> > definition:
> > 
> > ../../third_party/WebKit/Source/platform/TraceEvent.h:82:9: error:
> > 'TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET' macro redefined
> > [-Werror,-Wmacro-redefined]
> > #define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
> >         ^
> > ../../base/trace_event/trace_event.h:58:9: note: previous definition is here
> > #define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
> >         ^
> 
> platform/ can directly use base/, so it's allowed to use base/trace_event/.
That
> said, it also makes sense to use Source/platform/TraceEvent/ since they
provide
> macros for Blink (mainly for core/ and modules/ though).

Is there a file somewhere that includes both tracing systems? If possible let's
try to use just one of them.

A longer term fix I think would be unifying the macros so that everything uses
base/trace_event.

[haraken, 2016-08-15 14:30:44]

On 2016/08/15 10:48:00, Sami wrote:
> On 2016/08/13 01:37:12, haraken wrote:
> > On 2016/08/12 23:57:55, Shubhie wrote:
> > > Hey Sami,
> > > Now that the scheduler is in Blink (awesome!) should it be switched to use
> > > Blink's trace macros (i.e. third_party/WebKit/Source/platform/TraceEvent)
as
> > > opposed to (base/trace_event)?
> > > 
> > > 
> > > For background, I'm running into compilation issues while trying to tweak
> long
> > > task instrumentation in the scheduler with these errors about duplicate
> MACRO
> > > definition:
> > > 
> > > ../../third_party/WebKit/Source/platform/TraceEvent.h:82:9: error:
> > > 'TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET' macro redefined
> > > [-Werror,-Wmacro-redefined]
> > > #define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
> > >         ^
> > > ../../base/trace_event/trace_event.h:58:9: note: previous definition is
here
> > > #define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
> > >         ^
> > 
> > platform/ can directly use base/, so it's allowed to use base/trace_event/.
> That
> > said, it also makes sense to use Source/platform/TraceEvent/ since they
> provide
> > macros for Blink (mainly for core/ and modules/ though).
> 
> Is there a file somewhere that includes both tracing systems? If possible
let's
> try to use just one of them.

Agreed. As far as I understand, we haven't yet started an effort to merge
platform/TraceEvent.h with base::trace_event. So there's no file that includes
both.

> 
> A longer term fix I think would be unifying the macros so that everything uses
> base/trace_event.

Yes, this sounds like a right direction.

primiano@ should know more about platform/TraceEvent.h v.s. base::trace_event.

[panicker, 2016-08-15 17:08:22]

On 2016/08/15 14:30:44, haraken wrote:
> On 2016/08/15 10:48:00, Sami wrote:
> > On 2016/08/13 01:37:12, haraken wrote:
> > > On 2016/08/12 23:57:55, Shubhie wrote:
> > > > Hey Sami,
> > > > Now that the scheduler is in Blink (awesome!) should it be switched to
use
> > > > Blink's trace macros (i.e.
third_party/WebKit/Source/platform/TraceEvent)
> as
> > > > opposed to (base/trace_event)?
> > > > 
> > > > 
> > > > For background, I'm running into compilation issues while trying to
tweak
> > long
> > > > task instrumentation in the scheduler with these errors about duplicate
> > MACRO
> > > > definition:
> > > > 
> > > > ../../third_party/WebKit/Source/platform/TraceEvent.h:82:9: error:
> > > > 'TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET' macro redefined
> > > > [-Werror,-Wmacro-redefined]
> > > > #define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
> > > >         ^
> > > > ../../base/trace_event/trace_event.h:58:9: note: previous definition is
> here
> > > > #define TRACE_EVENT_GET_SAMPLING_STATE_FOR_BUCKET(bucket_number) \
> > > >         ^
> > > 
> > > platform/ can directly use base/, so it's allowed to use
base/trace_event/.
> > That
> > > said, it also makes sense to use Source/platform/TraceEvent/ since they
> > provide
> > > macros for Blink (mainly for core/ and modules/ though).
> > 
> > Is there a file somewhere that includes both tracing systems? If possible
> let's
> > try to use just one of them.
> 
> Agreed. As far as I understand, we haven't yet started an effort to merge
> platform/TraceEvent.h with base::trace_event. So there's no file that includes
> both.
> 
> > 
> > A longer term fix I think would be unifying the macros so that everything
uses
> > base/trace_event.
> 
> Yes, this sounds like a right direction.
> 
> primiano@ should know more about platform/TraceEvent.h v.s. base::trace_event.

Thanks. I've moved this discussion to email thread to loop in tracing folks and
learn about the unification plan (if any)