Split user agent code out of the 'glue' target

Split user agent code out of the 'glue' target

This creates a new user_agent component target for the user agent handling code. This makes it straightforward for iOS to build only the user-agent part of glue. It should also make incremental dismantling of webkit/ easier, since the dependencies on user agent code are now explicit.

The code that was in user_agent.cc/h (the webkit_user_agent target) has been moved to user_agent_util.cc/h, and folded into the user_agent component target. All user-agent related code has been moved to a new webkit/user_agent/ directory.

BUG=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155695

[stuartmorgan, 2012-08-27 14:32:53]

Per the email thread last week.

[tony, 2012-08-27 17:58:36]

LGTM

http://codereview.chromium.org/10869073/diff/11001/webkit/glue/user_agent_uni...
File webkit/glue/user_agent_unittest.cc (right):

http://codereview.chromium.org/10869073/diff/11001/webkit/glue/user_agent_uni...
webkit/glue/user_agent_unittest.cc:15: class WebkitGlueUserAgentTest : public
TestShellTest {
Nit: If you wanted to put this into content_unittests instead, that would help
crbug.com/126514 .  Not a big deal, someone else can also move it later.

[stuartmorgan, 2012-08-28 08:02:12]

+jam for content/ and chrome/ OWNERS (changes there are trivial)

http://codereview.chromium.org/10869073/diff/11001/webkit/glue/user_agent_uni...
File webkit/glue/user_agent_unittest.cc (right):

http://codereview.chromium.org/10869073/diff/11001/webkit/glue/user_agent_uni...
webkit/glue/user_agent_unittest.cc:15: class WebkitGlueUserAgentTest : public
TestShellTest {
On 2012/08/27 17:58:36, tony wrote:
> Nit: If you wanted to put this into content_unittests instead, that would help
> crbug.com/126514 .  Not a big deal, someone else can also move it later.

The test currently relies on the user agent being set by TestShellTest (see
IsSpoofedUserAgent), and presumably we don't want content_unittests running the
test shell setup and teardown. Since it's a logic change in the test, I think
it's probably best done separately.

[jam, 2012-08-28 16:08:42]

lgtm

[commit-bot: I haz the power, 2012-08-28 17:36:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10869073/11001

[commit-bot: I haz the power, 2012-08-28 18:01:00]

Try job failure for 10869073-11001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[stuartmorgan, 2012-08-29 07:09:38]

Sigh; component builds. +dpranke and thakis, who seem from blame to be the
experts here, since I'm not sure how to handle this component build failure.

This is failing because the webkit_user_agent target, which the UA code is now
part of, isn't defining WEBKIT_GLUE_EXPORT correctly because
WEBKIT_GLUE_IMPLEMENTATION isn't set. Setting that in the webkit_user_agent
target would be wrong presumably, because it's a static library target that a
bunch of things link.

There's also the question of what happens to g_user_agent if it ends up in
multiple components in a component build. I'm guessing nothing good.

I'm new to this structure, so I'm not sure what the solution here is. Make a new
component target just for user agent, with its own EXPORT? Move this code not to
user_agent.{h,cc}, but to some new other file that stays in the glue target?
Abandon this CL (dooming the file to have ifdefs for iOS)?

[stuartmorgan, 2012-08-29 07:10:26]

Sigh, +dpranke and thakis for real. Please see previous comment for questions.

[Nico, 2012-08-29 15:52:50]

On 2012/08/29 07:09:38, stuartmorgan wrote:
> This is failing because the webkit_user_agent target, which the UA code is now
> part of, isn't defining WEBKIT_GLUE_EXPORT correctly because
> WEBKIT_GLUE_IMPLEMENTATION isn't set. Setting that in the webkit_user_agent
> target would be wrong presumably, because it's a static library target that a
> bunch of things link.

I think it'd also do the wrong thing if you happen to include a header from the
webkit_glue target (it'd expand to the _IMPL version, even though that's not
true when building the user_agent target)

> 
> There's also the question of what happens to g_user_agent if it ends up in
> multiple components in a component build. I'm guessing nothing good.
> 
> I'm new to this structure, so I'm not sure what the solution here is. Make a
new
> component target just for user agent, with its own EXPORT? Move this code not
to
> user_agent.{h,cc}, but to some new other file that stays in the glue target?
> Abandon this CL (dooming the file to have ifdefs for iOS)?

From what I can tell, webkit_user_agent is already its own target (tommi created
it in http://codereview.chromium.org/3225003 for chrome frame, and piman tweaked
it in a minor way in http://codereview.chromium.org/8417017 ). Making it type
<(component) and giving it its own export macro sounds reasonable to me.

I think rvargas and darin came up with the initial components build, so you can
ask them if you want a second opinion. I just ported the thing to mac after it
was done.

[Dirk Pranke, 2012-08-29 16:38:40]

On 2012/08/29 15:52:50, Nico wrote:
> On 2012/08/29 07:09:38, stuartmorgan wrote:
> > This is failing because the webkit_user_agent target, which the UA code is
now
> > part of, isn't defining WEBKIT_GLUE_EXPORT correctly because
> > WEBKIT_GLUE_IMPLEMENTATION isn't set. Setting that in the webkit_user_agent
> > target would be wrong presumably, because it's a static library target that
a
> > bunch of things link.
> 
> I think it'd also do the wrong thing if you happen to include a header from
the
> webkit_glue target (it'd expand to the _IMPL version, even though that's not
> true when building the user_agent target)
> 
> > 
> > There's also the question of what happens to g_user_agent if it ends up in
> > multiple components in a component build. I'm guessing nothing good.
> > 
> > I'm new to this structure, so I'm not sure what the solution here is. Make a
> new
> > component target just for user agent, with its own EXPORT? Move this code
not
> to
> > user_agent.{h,cc}, but to some new other file that stays in the glue target?
> > Abandon this CL (dooming the file to have ifdefs for iOS)?
> 
> From what I can tell, webkit_user_agent is already its own target (tommi
created
> it in http://codereview.chromium.org/3225003 for chrome frame, and piman
tweaked
> it in a minor way in http://codereview.chromium.org/8417017 ). Making it type
> <(component) and giving it its own export macro sounds reasonable to me.
> 

Yes, this is the Right Thing To Do. Every target needs its own export macro or
much unhappiness will follow :)

[stuartmorgan, 2012-09-05 11:34:52]

Tony, John, please take another look. This is now a very different CL,
unfortunately.

The user agent code has a global, and exports methods, so needs to live in a
component. I tried making webkit_user_agent a component, but that target is
actually used by third_party/WebKit, so putting more code into it and making it
a component isn't really workable.

So now instead of a simple code move, this CL makes a new component target
called user_agent, which is separate from webkit_user_agent, and has just the
user agent code that was in webkit_glue.cc. Since there are now more user agent
files, and it's a component, I make a new webkit/user_agent directory for them
to match all the other components (this will also help as we tighten down DEPS
rules in chrome/ and content/).

I think this is still worth doing; it will be very helpful for iOS, and I think
having fewer things in the catch-all "glue" target will make incremental work on
removing code from webkit/ easier (especially since it seems likely that user
agent code will take longer to move out due to the DRT/TST tie-ins).

[tony, 2012-09-05 17:45:24]

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
File webkit/user_agent/user_agent.cc (right):

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
webkit/user_agent/user_agent.cc:116: BuildUserAgentFromProduct("Version/5.1.1
Safari/534.51.22");
It looks like this symbol is from user_agent_util.h, but this target doesn't
depend on that?

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
webkit/user_agent/user_agent.cc:122: webkit_glue::BuildOSCpuInfo().c_str());
Ditto.

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
File webkit/user_agent/webkit_user_agent.gypi (right):

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
webkit/user_agent/webkit_user_agent.gypi:52: 'target_name': 'webkit_user_agent',
I'm confused by this target. In the description, you say this is used by
third_party/WebKit targets, but I see user_agent_util.h being included in
src/chrome.

I see that webkit_unit_tests depends on webkit_user_agent, but I think we can
drop that dependency (links for me on Linux in both components build and
non-components build).

Could we merge this into user_agent and export the functions?

[stuartmorgan, 2012-09-05 19:58:14]

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
File webkit/user_agent/user_agent.cc (right):

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
webkit/user_agent/user_agent.cc:116: BuildUserAgentFromProduct("Version/5.1.1
Safari/534.51.22");
On 2012/09/05 17:45:24, tony wrote:
> It looks like this symbol is from user_agent_util.h, but this target doesn't
> depend on that?

Whoops, meant to add that. Good catch; I'll spin a new version tomorrow with
that dependency.

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
File webkit/user_agent/webkit_user_agent.gypi (right):

https://chromiumcodereview.appspot.com/10869073/diff/24005/webkit/user_agent/...
webkit/user_agent/webkit_user_agent.gypi:52: 'target_name': 'webkit_user_agent',
On 2012/09/05 17:45:24, tony wrote:
> I'm confused by this target. In the description, you say this is used by
> third_party/WebKit targets, but I see user_agent_util.h being included in
> src/chrome.

Sorry, I should have explained better. It's included by *both* chrome/ and
third_party/WebKit (specifically, both
src/third_party/WebKit/Source/WebKit/chromium/WebKitUnitTests.gyp and
src/third_party/WebKit/Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp
depend directly on it).

> Could we merge this into user_agent and export the functions?

I'm not sure how/if the component build system interacts with
third_party/WebKit. AFAICT, nothing in third_party/WebKit currently depends on a
<(component) target.

[tony, 2012-09-05 20:30:26]

On 2012/09/05 19:58:14, stuartmorgan wrote:
> Sorry, I should have explained better. It's included by *both* chrome/ and
> third_party/WebKit (specifically, both
> src/third_party/WebKit/Source/WebKit/chromium/WebKitUnitTests.gyp and
>
src/third_party/WebKit/Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp
> depend directly on it).

Ah, I see.  I think we can remove the dependencies from WebKitUnitTests.gyp and
DumpRenderTree.gyp.  That avoids the need for third_party/WebKit depending on a
<(component) target.

I'll try to get a patch in for that today so you can move forward on this
tomorrow.

[stuartmorgan, 2012-09-06 13:34:30]

Awesome; new version up that eliminates webkit_user_agent, and folds the code
into user_agent as exported functions. If you want me to fold the files
together, let me know.

[tony, 2012-09-06 19:14:15]

LGTM.  Looks like you'll have to wait for webkit deps to roll in r127691 before
you can use the trybots.

[stuartmorgan, 2012-09-07 07:06:43]

On 2012/09/06 19:14:15, tony wrote:
> LGTM.  Looks like you'll have to wait for webkit deps to roll in r127691
before
> you can use the trybots.

Yep, I realized my mistake when I got the flurry of failure emails :)

[commit-bot: I haz the power, 2012-09-07 07:09:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10869073/28009

[commit-bot: I haz the power, 2012-09-07 07:34:28]

Try job failure for 10869073-28009 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-07 08:30:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10869073/26050

[commit-bot: I haz the power, 2012-09-07 08:46:30]

Try job failure for 10869073-26050 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[stuartmorgan, 2012-09-07 09:04:26]

I thought I had all the component dependencies worked out, but apparently not (I
was missing base, and then it looks like dynamic annotations). Working through
another try cycle.

[stuartmorgan, 2012-09-07 11:40:31]

My local Mac component build is happy; fingers crossed.

(webkit_support had been implicitly depending on the transitive dependency from
webkit_user_agent to base_i18n; changing the user agent code to a shared library
exposed that bug.)

[commit-bot: I haz the power, 2012-09-07 11:40:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10869073/35006

[commit-bot: I haz the power, 2012-09-07 13:23:51]

Try job failure for 10869073-35006 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-07 13:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10869073/35006

[commit-bot: I haz the power, 2012-09-07 15:31:55]

Try job failure for 10869073-35006 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-07 15:36:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10869073/35006

[commit-bot: I haz the power, 2012-09-07 18:11:48]

Failed to apply patch for content/content_common.gypi:
While running patch -p1 --forward --force;
patching file content/content_common.gypi
Hunk #1 FAILED at 23.
1 out of 1 hunk FAILED -- saving rejects to file content/content_common.gypi.rej

[commit-bot: I haz the power, 2012-09-10 07:56:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10869073/36105

[commit-bot: I haz the power, 2012-09-10 10:01:09]

Change committed as 155695