Prevent static mojo builds from pulling //ui/gl into mojo_shell

Prevent static mojo builds from pulling //ui/gl into mojo_shell

BUG=

Committed: https://crrev.com/608e1a19df52b2c50015ace4fccbea0a3a4173cd
Cr-Commit-Position: refs/heads/master@{#297207}

[DaveMoore, 2014-09-25 19:46:50]

davemoore@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-09-25 20:09:44]

Almost but not quite.

https://codereview.chromium.org/603273003/diff/1/gpu/command_buffer/client/BU...
File gpu/command_buffer/client/BUILD.gn (left):

https://codereview.chromium.org/603273003/diff/1/gpu/command_buffer/client/BU...
gpu/command_buffer/client/BUILD.gn:126: "//ui/gl",
this isn't quite correct - gles2_implementation.cc #includes
ui/gl/gl_switches.h, so it should declare a dependency on something to match the
code.  Could you split the switches out into a separate target perhaps?

https://codereview.chromium.org/603273003/diff/1/mojo/shell/BUILD.gn
File mojo/shell/BUILD.gn (right):

https://codereview.chromium.org/603273003/diff/1/mojo/shell/BUILD.gn#newcode42
mojo/shell/BUILD.gn:42: "//net",
why do we need //net ?

https://codereview.chromium.org/603273003/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/603273003/diff/1/ui/gfx/BUILD.gn#newcode22
ui/gfx/BUILD.gn:22: source_set("switches") {
the switches are exported from the gfx component, so linking them in separately
won't work. if you want to split this you you need to visibility-restrict it
into the gfx component and you cannot link it separately.

you could make this a separate component if you want it to be separately
linkable

[jamesr, 2014-09-25 20:14:07]

https://codereview.chromium.org/603273003/diff/1/mojo/shell/BUILD.gn
File mojo/shell/BUILD.gn (right):

https://codereview.chromium.org/603273003/diff/1/mojo/shell/BUILD.gn#newcode14
mojo/shell/BUILD.gn:14: "//ui/gfx:switches",
for this since we just use the one constant honestly I'd just hardcode the
string constant in mojo_shell

[DaveMoore, 2014-09-26 18:15:59]

Copy switches to break dependencies

[DaveMoore, 2014-09-26 18:28:53]

No need to export

[DaveMoore, 2014-09-26 18:29:23]

https://codereview.chromium.org/603273003/diff/1/gpu/command_buffer/client/BU...
File gpu/command_buffer/client/BUILD.gn (left):

https://codereview.chromium.org/603273003/diff/1/gpu/command_buffer/client/BU...
gpu/command_buffer/client/BUILD.gn:126: "//ui/gl",
I created a new kEnableGPUServiceLoggingGPU flag in the new
client/gpu_switches.{cc,h} in a way that matches the use of
kEnableGPUServiceLoggingGPU in service/gpu_switches.{cc,h}

On 2014/09/25 20:09:44, jamesr wrote:
> this isn't quite correct - gles2_implementation.cc #includes
> ui/gl/gl_switches.h, so it should declare a dependency on something to match
the
> code.  Could you split the switches out into a separate target perhaps?

https://codereview.chromium.org/603273003/diff/1/mojo/shell/BUILD.gn
File mojo/shell/BUILD.gn (right):

https://codereview.chromium.org/603273003/diff/1/mojo/shell/BUILD.gn#newcode14
mojo/shell/BUILD.gn:14: "//ui/gfx:switches",
I hardcoded it.

On 2014/09/25 20:14:07, jamesr wrote:
> for this since we just use the one constant honestly I'd just hardcode the
> string constant in mojo_shell

https://codereview.chromium.org/603273003/diff/1/mojo/shell/BUILD.gn#newcode42
mojo/shell/BUILD.gn:42: "//net",
Accident. Removed.
On 2014/09/25 20:09:44, jamesr wrote:
> why do we need //net ?

https://codereview.chromium.org/603273003/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/603273003/diff/1/ui/gfx/BUILD.gn#newcode22
ui/gfx/BUILD.gn:22: source_set("switches") {
Copied switch instead.

On 2014/09/25 20:09:44, jamesr wrote:
> the switches are exported from the gfx component, so linking them in
separately
> won't work. if you want to split this you you need to visibility-restrict it
> into the gfx component and you cannot link it separately.
> 
> you could make this a separate component if you want it to be separately
> linkable

[jamesr, 2014-09-26 19:14:26]

https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_switches.cc (right):

https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_switches.cc:11: // gl_switches.cc. It's defined
here again to avoid dependencies between
can't you delete the version in gl_switches? you don't need two

https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_switches.h (right):

https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_switches.h:5: // Defines all the command-line
switches used by gpu/command_buffer/service/.
s/service/client/

https://codereview.chromium.org/603273003/diff/40001/ui/gl/gl_switches.cc
File ui/gl/gl_switches.cc (right):

https://codereview.chromium.org/603273003/diff/40001/ui/gl/gl_switches.cc#new...
ui/gl/gl_switches.cc:28: const char kEnableGPUClientLogging[]        =
"enable-gpu-client-logging";
you have to delete this or you have ODR violations on
switches::kEnableGPUClientLogging

[DaveMoore, 2014-09-26 20:10:16]

https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_switches.cc (right):

https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_switches.cc:11: // gl_switches.cc. It's defined
here again to avoid dependencies between
That version is used by render_process_host_impl.cc
On 2014/09/26 19:14:25, jamesr wrote:
> can't you delete the version in gl_switches? you don't need two

https://codereview.chromium.org/603273003/diff/40001/ui/gl/gl_switches.cc
File ui/gl/gl_switches.cc (right):

https://codereview.chromium.org/603273003/diff/40001/ui/gl/gl_switches.cc#new...
ui/gl/gl_switches.cc:28: const char kEnableGPUClientLogging[]        =
"enable-gpu-client-logging";
This one is kEnableGPUClientLogging
The new one is kEnableGPUClientLoggingGPU

This is the same approach used in gpu/command_buffer/service for
kEnableGPUServiceLoggingGPU
On 2014/09/26 19:14:25, jamesr wrote:
> you have to delete this or you have ODR violations on
> switches::kEnableGPUClientLogging

[DaveMoore, 2014-09-26 20:11:53]

Only define kEnableHarfBuzzRenderText if used

[jamesr, 2014-09-26 20:16:00]

On 2014/09/26 20:10:16, DaveMoore wrote:
>
https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
> File gpu/command_buffer/client/gpu_switches.cc (right):
> 
>
https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
> gpu/command_buffer/client/gpu_switches.cc:11: // gl_switches.cc. It's defined
> here again to avoid dependencies between
> That version is used by render_process_host_impl.cc

OK, but it can use the one in gpu/command_buffer/client/...


> On 2014/09/26 19:14:25, jamesr wrote:
> > can't you delete the version in gl_switches? you don't need two
> 
> https://codereview.chromium.org/603273003/diff/40001/ui/gl/gl_switches.cc
> File ui/gl/gl_switches.cc (right):
> 
>
https://codereview.chromium.org/603273003/diff/40001/ui/gl/gl_switches.cc#new...
> ui/gl/gl_switches.cc:28: const char kEnableGPUClientLogging[]        =
> "enable-gpu-client-logging";
> This one is kEnableGPUClientLogging
> The new one is kEnableGPUClientLoggingGPU

That's silly.

> 
> This is the same approach used in gpu/command_buffer/service for
> kEnableGPUServiceLoggingGPU
> On 2014/09/26 19:14:25, jamesr wrote:
> > you have to delete this or you have ODR violations on
> > switches::kEnableGPUClientLogging

[DaveMoore, 2014-09-26 22:29:54]

Share kEnableGPUClientLogging

[DaveMoore, 2014-09-26 22:31:08]

https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_switches.cc (right):

https://codereview.chromium.org/603273003/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_switches.cc:11: // gl_switches.cc. It's defined
here again to avoid dependencies between
On 2014/09/26 20:10:16, DaveMoore wrote:
> That version is used by render_process_host_impl.cc
> On 2014/09/26 19:14:25, jamesr wrote:
> > can't you delete the version in gl_switches? you don't need two
> 

Done.

https://codereview.chromium.org/603273003/diff/40001/ui/gl/gl_switches.cc
File ui/gl/gl_switches.cc (right):

https://codereview.chromium.org/603273003/diff/40001/ui/gl/gl_switches.cc#new...
ui/gl/gl_switches.cc:28: const char kEnableGPUClientLogging[]        =
"enable-gpu-client-logging";
On 2014/09/26 20:10:16, DaveMoore wrote:
> This one is kEnableGPUClientLogging
> The new one is kEnableGPUClientLoggingGPU
> 
> This is the same approach used in gpu/command_buffer/service for
> kEnableGPUServiceLoggingGPU
> On 2014/09/26 19:14:25, jamesr wrote:
> > you have to delete this or you have ODR violations on
> > switches::kEnableGPUClientLogging
> 

Done.

[jamesr, 2014-09-26 22:46:19]

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
File gpu/command_buffer/client/BUILD.gn (right):

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
gpu/command_buffer/client/BUILD.gn:114: defines = ["GLES2_IMPL_IMPLEMENTATION",
"GPU_IMPLEMENTATION" ]
this is not GPU_IMPLEMENTATION, it's a different component.  If you set the
export header for both you'll get inconsistent declspecs

[jamesr, 2014-09-26 22:49:20]

I don't think the build stuff in gpu/.. is set up correctly.

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_switches.h (right):

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_switches.h:5: // Defines all the command-line
switches used by gpu/command_buffer/service/.
fix the comment

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_switches.h:10: #include "gpu/gpu_export.h"
is gpu_export correct? that would be the case if you were linking this in to the
gpu component, but the build system doesn't look like it's doing that

[DaveMoore, 2014-09-26 22:51:18]

Forgot to remove an extra define in gn

[DaveMoore, 2014-09-26 22:52:07]

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
File gpu/command_buffer/client/BUILD.gn (right):

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
gpu/command_buffer/client/BUILD.gn:114: defines = ["GLES2_IMPL_IMPLEMENTATION",
"GPU_IMPLEMENTATION" ]
Urp. I realized that and moved the files to the client source_set, but forgot to
take out the extra define
On 2014/09/26 22:46:18, jamesr wrote:
> this is not GPU_IMPLEMENTATION, it's a different component.  If you set the
> export header for both you'll get inconsistent declspecs

[jamesr, 2014-09-27 23:29:40]

still not lgtm.  Could you let me know when you think you've addressed comments
so I can know when to look at a new patch instead of giving the exact same
review comments on patch set after patch set?  It's getting frustrating.

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_switches.cc (right):

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_switches.cc:11: // gl_switches.cc. It's defined
here again to avoid dependencies between
this comment isn't correct

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_switches.h (right):

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_switches.h:5: // Defines all the command-line
switches used by gpu/command_buffer/service/.
this still says "service"

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_switches.h:10: #include "gpu/gpu_export.h"
no. gpu_export is for things in the gpu component.  This is not in the gpu
component, it's linked into the gles2_impl component, so you want the
gles2_impl_export.h export header

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_switches.h:14: GPU_EXPORT extern const char
kEnableGPUClientLogging[];
still the wrong export macro

[DaveMoore, 2014-09-28 02:45:11]

Address James' commentsa

[DaveMoore, 2014-09-28 05:05:29]

I missed your second set of comments while fixing the first, and didn't notice
that you had added more. Sorry about that. I think this takes care of them.

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_switches.h (right):

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_switches.h:5: // Defines all the command-line
switches used by gpu/command_buffer/service/.
On 2014/09/26 22:49:20, jamesr wrote:
> fix the comment

Done.

https://codereview.chromium.org/603273003/diff/80001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_switches.h:10: #include "gpu/gpu_export.h"
Changes to use gles_impl_export.h.

Question: All the other users of this file import it with just
"gles_impl_export.h". Why would we not specify a full path?
On 2014/09/26 22:49:20, jamesr wrote:
> is gpu_export correct? that would be the case if you were linking this in to
the
> gpu component, but the build system doesn't look like it's doing that

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_switches.cc (right):

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_switches.cc:11: // gl_switches.cc. It's defined
here again to avoid dependencies between
On 2014/09/27 23:29:40, jamesr wrote:
> this comment isn't correct

Done.

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_switches.h (right):

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_switches.h:5: // Defines all the command-line
switches used by gpu/command_buffer/service/.
On 2014/09/27 23:29:40, jamesr wrote:
> this still says "service"

Done.

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_switches.h:10: #include "gpu/gpu_export.h"
On 2014/09/27 23:29:40, jamesr wrote:
> no. gpu_export is for things in the gpu component.  This is not in the gpu
> component, it's linked into the gles2_impl component, so you want the
> gles2_impl_export.h export header

Done.

https://codereview.chromium.org/603273003/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_switches.h:14: GPU_EXPORT extern const char
kEnableGPUClientLogging[];
On 2014/09/27 23:29:40, jamesr wrote:
> still the wrong export macro

Done.

[jamesr, 2014-09-28 19:09:47]

jamesr@chromium.org changed reviewers:
+ piman@chromium.org

[jamesr, 2014-09-28 19:09:48]

thanks, lgtm

+piman for ui/gl/ and gpu/ OWNERS

[piman, 2014-09-29 14:09:11]

LGTM, thanks for the cleanup!

[DaveMoore, 2014-09-29 15:47:05]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-09-29 15:47:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/603273003/120001

[commit-bot: I haz the power, 2014-09-29 18:13:38]

Committed patchset #7 (id:120001) as 2e37eab900670c1ca1a5300060fa93ae8f39bf18

[commit-bot: I haz the power, 2014-09-29 18:14:34]

Patchset 7 (id:??) landed as
https://crrev.com/608e1a19df52b2c50015ace4fccbea0a3a4173cd
Cr-Commit-Position: refs/heads/master@{#297207}