I do agree it's annoying, and I wonder if there's another way to make it better?

I think that any IWYU tool we create in the future add those includes back. IOW including those seems correct whether required or not, given our current header structure?

danakj@: You're worried that if we create an IWYU tool in the future, it will add back the includes that I plan to mass-remove?

I seems correct to include those headers even when they aren't required.

If we do create a tool in the future, we can have a way to annotate exceptions like this (and also content_client.h).

Let's not let the possible future stop us now though. Moving from message_loop.h to a task runner header is a good change. But having two always include it in conjunction with location.h is an annoyance.

It's more than IWYU states you should include the header of things you use. And we are using SingleThreadTaskRunner, so we should include the header. Making shortcuts doesn't change that really.

I suppose we could make automated exceptions for automation, it's just that we're explicitly trying to not include what we use here, which seems a bit weird?

jam: Do you feel that we should not include what we use here explicitly? Perhaps the code could be restructured somehow so you don't have to use so many separate things to post instead?

I guess I am unclear what motivates treating location.h as being so exceptional. There's lots of places where we construct object A to pass to object B, and we include A.h and B.h as a result. That seems like it is working as intended to me as far as C++ goes.

regarding what makes task runner special: There are 4 methods on that class. If cs' reference count is somewhat accurate, 82% of the calls to that class are to the 3 methods which take FROM_HERE. Of course, some subset of the remaining 18% are also in files that call the other methods but which I didn't further investigate.

Basically this header is not usable without location.h.

Stepping back, the purpose of IWYU is to avoid slowing down the build by including unnecessary headers. In this case, the headers are always necessary and will always be included by the source files anyways. So I don't see a negative.

> the purpose of IWYU is to avoid slowing down the build by including unnecessary headers.

I thought that is doubly intended to do both what you said, but also to prevent depending on transitive includes, which cause changes to a file to ripple through the codebase in terrible ways.

I guess in this case, location.h is both (almost) always needed, and also isn't use for other things than the call to PostTask. So it benefits from either case.

Thanks that's a really reasonable argument for why this is exceptional. LGTM

(so it does not benefit from either case, I mean)

I'm not so sure that in the case of the SingleThreadTaskRunner it is not likely to be used outside of the call to ThreadTaskRunnerHandle::Get()->*(). But perhaps it is close enough.

I (naively?) wish that looked more like SingleThreadTaskRunner::Get()->*() instead, which would mean it is in the right header to start with. Also you wouldn't have to remember so many names.

That's a good question. Why do we have thread_task_runner_handle.h instead of putting it as static methods in single_thread_task_runner.h?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d08c589d78f1e23d8813ba8bb5f265be22ee33e

commit 1d08c589d78f1e23d8813ba8bb5f265be22ee33e
Author: fdoray <fdoray@chromium.org>
Date: Thu Jun 09 22:55:06 2016

Replace forward declarations with includes in TaskRunner APIs.

This CL:
- Adds #include "base/location.h" in base/task_runner.h
- Adds #include "base/single_thread_task_runner.h"
      in base/threading/thread_task_runner_handle.h
- Adds #include "base/sequenced_task_runner.h"
      in base/threading/sequenced_task_runner_handle.h

This will be followed by a mass removal of includes that
are no longer necessary.

BUG= 618793 

Review-Url: https://codereview.chromium.org/2053503004
Cr-Commit-Position: refs/heads/master@{#399044}

[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/task_runner.h
[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/threading/sequenced_task_runner_handle.cc
[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/threading/sequenced_task_runner_handle.h
[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/threading/thread_task_runner_handle.cc
[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/threading/thread_task_runner_handle.h

Of some possible relevance (am reading it): https://docs.google.com/document/d/1A_LRKyTOCzhRPOY4Q3RsePuw4UCsvxuFYx6D18BaYk0/edit#

+gab re #10

Interesting, yes I'm currently thinking about changing TaskRunnerHandles semantics (looks like the doc was linked from somewhere else and found by a few before I intended to send it but it's mostly ready so all good :-)).

I did think about moving the getters to static *TaskRunner::Get() and could still be convinced that's the right thing. The reason that's not my current proposal is that (1) it would mean we would either have the scoped handle in a different header (problematic for anonymous TLS state, now need a public getter to be called by the TaskRunner getters...) or we would pollute the *TaskRunner headers with a setter not relevant to most (plus a setter implies heap memory instead of stack in the TLS, unless we expose a Handle object from TaskRunners directly as a "setter" ala TaskRunnerHandle) and (2) devs are already used to the TaskRunnerHandle flow so it's not much of a burden to keep it separate.

Note on (1): and even then we end up with TLS in 3 different places whereas my proposal merges it all in one file which I think is cleaner (though I guess w.r.t to this bug would mean we'd have to include all 3 *_task_runner.h files instead of fwd-decling them and letting users include the one they actually care about..?).

IMO we should optimize for the common case, which is needing to get the current task runner and post a task. This will be orders of magnitude more common that needing to call the setter.

re 1), can you expand on what you mean by anonymous TLS state? why not just have private methods and a friend declaration to let the scoped class be able to access it?

Having all task runner handles in the same header (cf. design doc) makes users aware of all available options and forces them to choose the most appropriate one for their use case. We don't want someone to use a SingleThreadTaskRunner just because they know how to get it if a TaskRunner would have been sufficient.

I'm curious why the strong belief that people will look at the header and see that there are other choices rather than copypaste calls to the GetSingleThread function. I have opened the thread_task_runner_handle.h file very few times, which makes me wonder.

For people reading the header, it certainly is obvious though. Do you think that we could get (near?) that level of visibility out of comments on SingleThreadTaskRunner::Get() and friends?

One of my goals with the new TaskRunnerHandle in a new header is exactly to have devs and reviewers learn this new paradigm and question the choices in reviews.

In the same line of thought I don't intend to mass migrate ThreadTaskRunnerHandle::Get() to TaskRunnerHandle::GetSingleThreaded() because I want callers of the latter to have truly made the decision to require thread affinity (as opposed to ThreadTaskRunnerHandle::Get() which used to be the only choice and is often over specific).

And I think devs/reviewers are more likely to notice there are multiple choices if it's all in the same header.

Re. private+friend for TLS: that works but then the TLS holds heap memory instead of stack as today's TaskRunnerHandles do.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d08c589d78f1e23d8813ba8bb5f265be22ee33e

commit 1d08c589d78f1e23d8813ba8bb5f265be22ee33e
Author: fdoray <fdoray@chromium.org>
Date: Thu Jun 09 22:55:06 2016

Replace forward declarations with includes in TaskRunner APIs.

This CL:
- Adds #include "base/location.h" in base/task_runner.h
- Adds #include "base/single_thread_task_runner.h"
      in base/threading/thread_task_runner_handle.h
- Adds #include "base/sequenced_task_runner.h"
      in base/threading/sequenced_task_runner_handle.h

This will be followed by a mass removal of includes that
are no longer necessary.

BUG= 618793 

Review-Url: https://codereview.chromium.org/2053503004
Cr-Commit-Position: refs/heads/master@{#399044}

[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/task_runner.h
[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/threading/sequenced_task_runner_handle.cc
[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/threading/sequenced_task_runner_handle.h
[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/threading/thread_task_runner_handle.cc
[modify] https://crrev.com/1d08c589d78f1e23d8813ba8bb5f265be22ee33e/base/threading/thread_task_runner_handle.h

- Add #include "base/location.h" in base/task_runner.h: Done
- Add #include "base/single_thread_task_runner.h" in base/threading/thread_task_runner_handle.h: Done
- Add #include "base/sequenced_task_runner.h" in base/threading/sequenced_task_runner_handle.h: Done
- Do a mass removal or #includes that are no longer required: WontFix

I don't think that spending time to do a mass removal of #includes that are no longer required + making sure that people don't continue to include headers that they don't need is worth it.