Add OILPAN string to developer build chromium's webkit revision

Add OILPAN string to developer build chromium's webkit revision

We often build non-oilpan chromium for measuring oilpan performance, and find the mistake after runnining benchmarks for a few days.
So we need the way to find whether a given chromium is oilpan-build or not.
This change adds "-OIPLAN" to webkit revision. So about:version tells us information about oilpan or not-oilpan.

BUG=420515

[tasak, 2015-01-14 04:38:27]

tasak@google.com changed reviewers:
+ haraken@chromium.org, kouhei@chromium.org

[kouhei (in TOK), 2015-01-14 04:42:05]

lgtm

[haraken, 2015-01-14 05:27:52]

LGTM

[tasak, 2015-01-14 12:18:30]

Thank you for reivewing.

I found that the BUILD.gn's change is wrong. I'm now trying to fix gn build
failures.

[tasak, 2015-01-15 04:25:53]

tasak@google.com changed reviewers:
+ brettw@chromium.org, jochen@chromium.org

[tasak, 2015-01-15 04:25:54]

jochen, brettw,
Would you review this CL?

https://codereview.chromium.org/800543006/diff/40001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/800543006/diff/40001/content/common/BUILD.gn#...
content/common/BUILD.gn:9: import("//third_party/WebKit/Source/config.gni")
I need "declare_args" in //third_party/WebKit/Source/config.gni.
However, I'm not sure whether it is allowed to import the config.gni here or
not.

[jochen (gone - plz use gerrit), 2015-01-15 15:04:09]

everything you include from blink should be in third_party/WebKit/public

[brettw, 2015-01-20 19:24:03]

Personally, I'd prefer to have the blink target add the oilpan define to the
direct dependent configs on blink so that random targets don't have to duplicate
this.

I'm not certain if all of the feature defines should work like this, or if only
a subset should. If it's all feature defines, we can just add
  direct_dependent_configs = [ "//third_party/WebKit/Source:features" ]
in the public blink targets. Otherwise we should make a new "public_featyres"
config or something.

[tasak, 2015-01-21 11:22:55]

Thank you for review.

I see.
I added third_party/WebKit/public/features.gni for public declare_args:
https://codereview.chromium.org/861083003/

[brettw, 2015-01-21 18:09:52]

The change you linked to is fine but not all of what I was suggesting.

I don't think we should be duplicating the define in content build files. If
blink defines a config (you can see there are some configs that set defines in
the existing Source/BUILD.gn file), a target can say

  public_configs = [ ":my_config" ]

and then any targets that depend on this one get that config applied to them. I
think this mechanism should be used to set the ENABLE_OILPAN define on content
(since it depends on blink). We will want this for the various variants of the
blink targets (all, minimal).

[tasak, 2015-01-22 07:03:22]

On 2015/01/21 18:09:52, brettw wrote:
> The change you linked to is fine but not all of what I was suggesting.
> 
> I don't think we should be duplicating the define in content build files. If
> blink defines a config (you can see there are some configs that set defines in
> the existing Source/BUILD.gn file), a target can say
> 
>   public_configs = [ ":my_config" ]
> 
> and then any targets that depend on this one get that config applied to them.
I
> think this mechanism should be used to set the ENABLE_OILPAN define on content
> (since it depends on blink). We will want this for the various variants of the
> blink targets (all, minimal).

I would like to confirm my understanding.

I've created the following patch:
https://codereview.chromium.org/864723003/

So the patch looks ok?

[tasak, 2015-01-22 07:10:43]

On 2015/01/22 07:03:22, tasak wrote:
> On 2015/01/21 18:09:52, brettw wrote:
> > The change you linked to is fine but not all of what I was suggesting.
> > 
> > I don't think we should be duplicating the define in content build files. If
> > blink defines a config (you can see there are some configs that set defines
in
> > the existing Source/BUILD.gn file), a target can say
> > 
> >   public_configs = [ ":my_config" ]
> > 
> > and then any targets that depend on this one get that config applied to
them.
> I
> > think this mechanism should be used to set the ENABLE_OILPAN define on
content
> > (since it depends on blink). We will want this for the various variants of
the
> > blink targets (all, minimal).
> 
> I would like to confirm my understanding.
> 
> I've created the following patch:
> https://codereview.chromium.org/864723003/
> 
> So the patch looks ok?

BUILD.gn's change is:
+  defines += blink_public_defines_list

[brettw, 2015-01-22 18:17:08]

Ideally GYP and GN should work the same.

In GN, in WebKit/public/BUILD.gn we would add:

  # Comment about how this is different than Source:features
  config("features") {
    defines = public_feature_defines_list
  }

In Source/wtf/BUILD.gn apply the public:features config in the same way as the
existing features one is (this will put the defines together for the internal
WebKit files).

For dependent targets, in public/BUILD.gn add to all the targets the line:
  public_configs = [ ":features" ]
This will apply those defines to any target that depends on :blink and friends.
Then you should have to make no changes to content and never duplicate the
define.

[brettw, 2015-01-22 18:22:29]

For GYP, the defines are set by wtf/wtf.gyp:wtf_config. The wtf target forwards
these to its dependents by listing wtf_config in its 'export_dependent_settings'
section. You can make a wtf_public_config target and hook it up like wtf_config
is in there.

To forward these outside of the public blink targets, add a dependency on
wtf_public_config from all the targets in public/blink, and then list
wtf_public_config in each target's export_dependent_settings list. This will
apply those defines to targets that depend on it.

[tasak, 2015-01-23 13:14:55]

Thank you for comments.

On 2015/01/22 18:22:29, brettw wrote:
> For GYP, the defines are set by wtf/wtf.gyp:wtf_config. The wtf target
forwards
> these to its dependents by listing wtf_config in its
'export_dependent_settings'
> section. You can make a wtf_public_config target and hook it up like
wtf_config
> is in there.
> 
> To forward these outside of the public blink targets, add a dependency on
> wtf_public_config from all the targets in public/blink, and then list
> wtf_public_config in each target's export_dependent_settings list. This will
> apply those defines to targets that depend on it.

I see.
So how about https://codereview.chromium.org/864723003/#ps60001 ?

Added direct_depedent_settings to blink_minimal.

Talking about GN, added pkg_config to blink and blink_minimal.

[tasak, 2015-01-23 13:16:59]

On 2015/01/23 13:14:55, tasak wrote:
> Thank you for comments.
> 
> On 2015/01/22 18:22:29, brettw wrote:
> > For GYP, the defines are set by wtf/wtf.gyp:wtf_config. The wtf target
> forwards
> > these to its dependents by listing wtf_config in its
> 'export_dependent_settings'
> > section. You can make a wtf_public_config target and hook it up like
> wtf_config
> > is in there.
> > 
> > To forward these outside of the public blink targets, add a dependency on
> > wtf_public_config from all the targets in public/blink, and then list
> > wtf_public_config in each target's export_dependent_settings list. This will
> > apply those defines to targets that depend on it.
> 
> I see.
> So how about https://codereview.chromium.org/864723003/#ps60001 ?
> 
> Added direct_depedent_settings to blink_minimal.
> 
> Talking about GN, added pkg_config to blink and blink_minimal.

I would like to discuss in https://codereview.chromium.org/864723003/

[tasak, 2015-02-06 11:36:29]

Would you review this CL?

Since the patch: https://codereview.chromium.org/864723003/ is committed:
https://src.chromium.org/viewvc/blink?view=rev&revision=189560, now we only need
to modify content/common/user_agent.cc.

[brettw, 2015-02-07 00:17:18]

https://codereview.chromium.org/800543006/diff/120001/content/common/user_age...
File content/common/user_agent.cc (right):

https://codereview.chromium.org/800543006/diff/120001/content/common/user_age...
content/common/user_agent.cc:41: #if defined(ENABLE_OILPAN) && ENABLE_OILPAN &&
!defined(OFFICIAL_BUILD)
You should do either "defined" style or check the value, but not both. Almost
all code does the defined style, and in fact there is no build code to set
ENABLE_OILPAN to 0. So just remove the ENABLE_OILPAN part of this.

Second, I don't understand the OFFICIAL_BUILD condition here. Shouldn't this
just key off of the value? You can key the ENABLE_OILPAN value off of whether
the build is official or not.

[tasak, 2015-02-10 05:51:25]

Thank you for comment.

https://codereview.chromium.org/800543006/diff/120001/content/common/user_age...
File content/common/user_agent.cc (right):

https://codereview.chromium.org/800543006/diff/120001/content/common/user_age...
content/common/user_agent.cc:41: #if defined(ENABLE_OILPAN) && ENABLE_OILPAN &&
!defined(OFFICIAL_BUILD)
On 2015/02/07 00:17:17, brettw wrote:

> Second, I don't understand the OFFICIAL_BUILD condition here. Shouldn't this
> just key off of the value? You can key the ENABLE_OILPAN value off of whether
> the build is official or not.

Because we don't need to add the "OILPAN" string to official build.

- Before shipping, we should not enable oilpan for official builds.
- After shipping, oilpan is always enabled. We don't need to check whether
oilpan build or not.

So I want to explicitly specify "!defined(OFFICIAL_BUILD)" here.
I also added FIXME comment here.

[tasak, 2015-02-10 12:44:53]

I'm sorry.

I will close this issue, because:
- this change depends on r189560, and
- r189560 causes build problem: when changing enable_oilpan flag's value, we
need to recompile +2000 files.

So I need another way to confirm whether a given binary is oilpan-build or not.

[brettw, 2015-02-10 22:51:38]

https://codereview.chromium.org/800543006/diff/120001/content/common/user_age...
File content/common/user_agent.cc (right):

https://codereview.chromium.org/800543006/diff/120001/content/common/user_age...
content/common/user_agent.cc:41: #if defined(ENABLE_OILPAN) && ENABLE_OILPAN &&
!defined(OFFICIAL_BUILD)
On 2015/02/10 05:51:24, tasak wrote:
> On 2015/02/07 00:17:17, brettw wrote:
> 
> > Second, I don't understand the OFFICIAL_BUILD condition here. Shouldn't this
> > just key off of the value? You can key the ENABLE_OILPAN value off of
whether
> > the build is official or not.
> 
> Because we don't need to add the "OILPAN" string to official build.
> 
> - Before shipping, we should not enable oilpan for official builds.
> - After shipping, oilpan is always enabled. We don't need to check whether
> oilpan build or not.
> 
> So I want to explicitly specify "!defined(OFFICIAL_BUILD)" here.
> I also added FIXME comment here.

But shouldn'e the ENABLE_OILPAN flag always be set correctly? If you've got an
official build with the ENABLE_OILPAN flag set and you don't want oilpan
enabled, or vice versa, something is wrong.

[haraken, 2015-02-10 23:38:01]

On 2015/02/10 22:51:38, brettw wrote:
>
https://codereview.chromium.org/800543006/diff/120001/content/common/user_age...
> File content/common/user_agent.cc (right):
> 
>
https://codereview.chromium.org/800543006/diff/120001/content/common/user_age...
> content/common/user_agent.cc:41: #if defined(ENABLE_OILPAN) && ENABLE_OILPAN
&&
> !defined(OFFICIAL_BUILD)
> On 2015/02/10 05:51:24, tasak wrote:
> > On 2015/02/07 00:17:17, brettw wrote:
> > 
> > > Second, I don't understand the OFFICIAL_BUILD condition here. Shouldn't
this
> > > just key off of the value? You can key the ENABLE_OILPAN value off of
> whether
> > > the build is official or not.
> > 
> > Because we don't need to add the "OILPAN" string to official build.
> > 
> > - Before shipping, we should not enable oilpan for official builds.
> > - After shipping, oilpan is always enabled. We don't need to check whether
> > oilpan build or not.
> > 
> > So I want to explicitly specify "!defined(OFFICIAL_BUILD)" here.
> > I also added FIXME comment here.
> 
> But shouldn'e the ENABLE_OILPAN flag always be set correctly? If you've got an
> official build with the ENABLE_OILPAN flag set and you don't want oilpan
> enabled, or vice versa, something is wrong.

Actually this is just a hack to move our temporary development faster, so we
don't want to spend a lot of time on this. We'll look for another way.