Share precompiled header setup for blink between Windows and Mac.

Share precompiled header setup for blink between Windows and Mac.

BUG=

Review-Url: https://codereview.chromium.org/2702363002
Cr-Commit-Position: refs/heads/master@{#459430}
Committed: https://chromium.googlesource.com/chromium/src/+/ba244ac715ad0cc3e981a25076fd728b9e490e59

[matthewtff.asm, 2017-02-21 09:04:55]

I've fixed usage of pch in blink while gn arg enable_precompiled_headers equals
false, olso unified it with windows and gained so about 3% compile time
decrease(wile enable_precompiled_headers=true).

Please, take a look

[matthewtff.asm, 2017-02-22 08:14:26]

Description was changed from

==========
[Mac] Fix precompiled headers usage in blink

BUG=
==========

to

==========
[Mac] Fix precompiled headers usage in blink

BUG=
==========

[matthewtff.asm, 2017-02-22 08:14:26]

matthewtff@yandex-team.ru changed reviewers:
+ dpranke@chromium.org

[matthewtff.asm, 2017-03-02 09:05:04]

Dirk, could you take a look, please?

[Dirk Pranke, 2017-03-02 16:40:07]

dpranke@chromium.org changed reviewers:
+ thakis@chromium.org

[Dirk Pranke, 2017-03-02 16:40:08]

thakis@ is probably a better reviewer for this ...

[Nico, 2017-03-02 16:59:14]

Cool, thanks. CL description could be better ("Share precompiled header setup
for blink between Windows and Mac." or something like that – with "fix" it's not
clear what exactly it's fixing.)

https://codereview.chromium.org/2702363002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mac/Precompile-platform.h (right):

https://codereview.chromium.org/2702363002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mac/Precompile-platform.h:5: #if
defined(PrecompilePlatform_h_)
Can we make it so that we have a platform/Precompile-platform.h header only, and
not one copy per platform? It could look like

#if defined...
...

#if defined(_MSC_VER)
#include "build/win/Precompile.h"
#elif defined(__APPLE__)
#include "build/mac/Prefix.h"
#else
#error implement
#endif

...

[matthewtff.asm, 2017-03-02 17:10:54]

On 2017/03/02 16:59:14, Nico wrote:
> Cool, thanks. CL description could be better ("Share precompiled header setup
> for blink between Windows and Mac." or something like that – with "fix" it's
not
> clear what exactly it's fixing.)
> 
>
https://codereview.chromium.org/2702363002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/mac/Precompile-platform.h (right):
> 
>
https://codereview.chromium.org/2702363002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/mac/Precompile-platform.h:5: #if
> defined(PrecompilePlatform_h_)
> Can we make it so that we have a platform/Precompile-platform.h header only,
and
> not one copy per platform? It could look like
> 
> #if defined...
> ...
> 
> #if defined(_MSC_VER)
> #include "build/win/Precompile.h"
> #elif defined(__APPLE__)
> #include "build/mac/Prefix.h"
> #else
> #error implement
> #endif
> 
> ...

Ofc we could :) I'll change the description and create a single as you suggest.

[matthewtff.asm, 2017-03-02 17:27:32]

Description was changed from

==========
[Mac] Fix precompiled headers usage in blink

BUG=
==========

to

==========
Share precompiled header setup for blink between Windows and Mac.

BUG=
==========

[matthewtff.asm, 2017-03-02 17:32:42]

Description was changed from

==========
Share precompiled header setup for blink between Windows and Mac.

BUG=
==========

to

==========
Share precompiled header setup for blink between Windows and Mac.

BUG=
==========

[matthewtff.asm, 2017-03-03 10:13:36]

On 2017/03/02 16:59:14, Nico wrote:
> Cool, thanks. CL description could be better ("Share precompiled header setup
> for blink between Windows and Mac." or something like that – with "fix" it's
not
> clear what exactly it's fixing.)
> 
>
https://codereview.chromium.org/2702363002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/mac/Precompile-platform.h (right):
> 
>
https://codereview.chromium.org/2702363002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/mac/Precompile-platform.h:5: #if
> defined(PrecompilePlatform_h_)
> Can we make it so that we have a platform/Precompile-platform.h header only,
and
> not one copy per platform? It could look like
> 
> #if defined...
> ...
> 
> #if defined(_MSC_VER)
> #include "build/win/Precompile.h"
> #elif defined(__APPLE__)
> #include "build/mac/Prefix.h"
> #else
> #error implement
> #endif
> 
> ...

I've moved files from ./win/ to ./ in core/ and platform/. Also changed the
header inclusion as you suggested. Take a look, please.

[Nico, 2017-03-03 16:04:35]

Thanks! If you're in a rush, lgtm as is. I think it'd be nicer with two changes:

1. Try removing the rebase_file callse for all the precompiled_header
assignments in the non-win case

2. Keep the Precompiled-foo.cpp file in /win

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/BUILD.gn (right):

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/BUILD.gn:73: precompiled_header =
rebase_path("build/mac/Prefix.h", root_build_dir)
I don't think you need the rebase_path call here. Maybe try removing it and see
if that hurts. (I'm aware you're just moving this around)

(`gn help precompiled_header` is pretty confusing though, I read through gn's
code to understand how this is used)

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/BUILD.gn (right):

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/BUILD.gn:70: precompiled_header =
rebase_path("Precompile-core.h", root_build_dir)
likewise

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/Precompile-core.cpp (right):

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/Precompile-core.cpp:5: // Precompiled header
generator for Windows builds. No include is needed
This file is still win-only and could arguably stay in the old /win location

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:183: precompiled_header =
rebase_path("Precompile-platform.h", root_build_dir)
likewise

[Nico, 2017-03-03 19:28:39]

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/BUILD.gn (right):

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/BUILD.gn:73: precompiled_header =
rebase_path("build/mac/Prefix.h", root_build_dir)
On 2017/03/03 16:04:35, Nico wrote:
> I don't think you need the rebase_path call here. Maybe try removing it and
see
> if that hurts. (I'm aware you're just moving this around)
> 
> (`gn help precompiled_header` is pretty confusing though, I read through gn's
> code to understand how this is used)

Looking at this more, maybe precompiled_header isn't used at all with gcc-style
pchs -- does removing this line completely make any difference?

[Nico, 2017-03-03 20:30:31]

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/BUILD.gn (right):

https://codereview.chromium.org/2702363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/BUILD.gn:73: precompiled_header =
rebase_path("build/mac/Prefix.h", root_build_dir)
On 2017/03/03 19:28:38, Nico wrote:
> On 2017/03/03 16:04:35, Nico wrote:
> > I don't think you need the rebase_path call here. Maybe try removing it and
> see
> > if that hurts. (I'm aware you're just moving this around)
> > 
> > (`gn help precompiled_header` is pretty confusing though, I read through
gn's
> > code to understand how this is used)
> 
> Looking at this more, maybe precompiled_header isn't used at all with
gcc-style
> pchs -- does removing this line completely make any difference?

I think you can in fact remove this line.
https://codereview.chromium.org/2734523002/
https://codereview.chromium.org/2731753003/

[matthewtff.asm, 2017-03-06 15:13:59]

The CQ bit was checked by matthewtff@yandex-team.ru

[matthewtff.asm, 2017-03-06 15:14:00]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2702363002/#ps40001
(title: "Share precompiled header setup for blink between Windows and Mac.")

[commit-bot: I haz the power, 2017-03-06 15:14:24]

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, 2017-03-06 15:20:58]

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

[commit-bot: I haz the power, 2017-03-06 15:20:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dirk Pranke, 2017-03-07 00:21:58]

On 2017/03/06 15:20:59, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

argh ... stupid owners bug. I'll try to land that fix now, and then you can try
again.

[Dirk Pranke, 2017-03-07 00:37:06]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2017-03-07 00:37:54]

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, 2017-03-07 01:03:59]

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

[commit-bot: I haz the power, 2017-03-07 01:04:00]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[matthewtff.asm, 2017-03-07 06:35:33]

On 2017/03/07 00:21:58, Dirk Pranke wrote:
> On 2017/03/06 15:20:59, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> argh ... stupid owners bug. I'll try to land that fix now, and then you can
try
> again.

There's also a little bug in code - several headers aren't included where they
should be. It's due to unconditional usage of precompiled headers in blink.
I'll add changes to fix these.

I should be honest to you guys - I've done this CL just to show and fix these
errors in upstream :)

[Nico, 2017-03-07 14:27:08]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2017-03-07 14:27:42]

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, 2017-03-07 14:52:14]

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

[commit-bot: I haz the power, 2017-03-07 14:52:15]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Nico, 2017-03-23 18:03:24]

Are you still working on landing this? I'd be interested in seeing this get in.
(Ran into https://bugs.chromium.org/p/chromium/issues/detail?id=704620 today)

[matthewtff.asm, 2017-03-24 12:57:10]

The CQ bit was checked by matthewtff@yandex-team.ru

[matthewtff.asm, 2017-03-24 12:57:11]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2702363002/#ps60001
(title: "Added missing Cocoa imports")

[commit-bot: I haz the power, 2017-03-24 12:57:27]

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, 2017-03-24 15:37:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490360230820000,
"parent_rev": "715e56aaf43c1997448ba967558157f6bed291d0", "commit_rev":
"ba244ac715ad0cc3e981a25076fd728b9e490e59"}

[commit-bot: I haz the power, 2017-03-24 15:38:00]

Description was changed from

==========
Share precompiled header setup for blink between Windows and Mac.

BUG=
==========

to

==========
Share precompiled header setup for blink between Windows and Mac.

BUG=

Review-Url: https://codereview.chromium.org/2702363002
Cr-Commit-Position: refs/heads/master@{#459430}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba244ac715ad0cc3e981a25076fd...
==========

[commit-bot: I haz the power, 2017-03-24 15:38:01]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ba244ac715ad0cc3e981a25076fd...

[Nico, 2017-03-25 16:18:16]

On 2017/03/24 15:38:01, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:60001) as
>
https://chromium.googlesource.com/chromium/src/+/ba244ac715ad0cc3e981a25076fd...

Thanks! I saw you added a few Cocoa.h imports -- since this is an extremely
heavy header, we often try to not import it in .H files but instead do `#ifdef
__OBJC__ @class NSColor; #else class NSColor;` -- do you think that's possible
here?

[matthewtff.asm, 2017-03-25 22:38:50]

On 2017/03/25 16:18:16, Nico wrote:
> On 2017/03/24 15:38:01, commit-bot: I haz the power wrote:
> > Committed patchset #4 (id:60001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/ba244ac715ad0cc3e981a25076fd...
> 
> Thanks! I saw you added a few Cocoa.h imports -- since this is an extremely
> heavy header, we often try to not import it in .H files but instead do `#ifdef
> __OBJC__ @class NSColor; #else class NSColor;` -- do you think that's possible
> here?

Agh, should've wait for your reply after updating CL. I'll check for if it's
possible on Monday and create a follow up CL.