Make ui/gfx/range a component

Make ui/gfx/range a component

In a long chain of issues to address to get mojo to serialize
ui/gfx types for the gpu_host channel in Mus, I've found
myself needing to decouple skia parts of ui/gfx from non-skia
parts. This CL is one step along the way.

BUG=586390
Committed: https://crrev.com/6f6dea38646e3fc4860fe478bd6f99fec70cdb4e
Cr-Commit-Position: refs/heads/master@{#390183}

[Fady Samuel, 2016-04-27 02:14:49]

Description was changed from

==========
Make ui/gfx/range a component

BUG=586390
==========

to

==========
Make ui/gfx/range a component

In a long chain of issues to address to get mojo to serialize
ui/gfx types for the gpu_host channel in Mus, I've found
myself needing to decouple skia parts of ui/gfx from non-skia
parts. This CL is one step along the way.

BUG=586390
==========

[Fady Samuel, 2016-04-27 02:15:00]

fsamuel@chromium.org changed reviewers:
+ danakj@chromium.org

[Fady Samuel, 2016-04-27 02:15:04]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 02:15:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922373002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922373002/20001

[commit-bot: I haz the power, 2016-04-27 02:27:31]

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

[commit-bot: I haz the power, 2016-04-27 02:27:32]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Fady Samuel, 2016-04-27 14:13:22]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 14:13:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922373002/40001

[commit-bot: I haz the power, 2016-04-27 14:42:59]

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

[commit-bot: I haz the power, 2016-04-27 14:43:00]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Fady Samuel, 2016-04-27 16:36:37]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 16:36:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922373002/40002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922373002/40002

[Fady Samuel, 2016-04-27 16:37:58]

fsamuel@chromium.org changed reviewers:
+ sky@chromium.org
- danakj@chromium.org

[Fady Samuel, 2016-04-27 16:37:58]

This touches a lot of build files across ui/ so danakj@ to cc and sky@ for ui/
review.

[sky, 2016-04-27 16:51:30]

https://codereview.chromium.org/1922373002/diff/40002/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1922373002/diff/40002/ui/gfx/gfx.gyp#newcode77
ui/gfx/gfx.gyp:77: 'GFX_IMPLEMENTATION',
Don't you need a separate macro for the range files? Otherwise if something from
ui/gfx happens to include ui/gfx/range then you'll get the wrong define set.

[Fady Samuel, 2016-04-27 17:16:49]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[Fady Samuel, 2016-04-27 17:16:59]

PTAL Scott! Thanks!

https://codereview.chromium.org/1922373002/diff/40002/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1922373002/diff/40002/ui/gfx/gfx.gyp#newcode77
ui/gfx/gfx.gyp:77: 'GFX_IMPLEMENTATION',
On 2016/04/27 16:51:30, sky wrote:
> Don't you need a separate macro for the range files? Otherwise if something
from
> ui/gfx happens to include ui/gfx/range then you'll get the wrong define set.

Done.

[commit-bot: I haz the power, 2016-04-27 17:17:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922373002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922373002/70001

[sky, 2016-04-27 17:50:39]

LGTM

https://codereview.chromium.org/1922373002/diff/70001/ui/gfx/range/gfx_range_...
File ui/gfx/range/gfx_range_export.h (right):

https://codereview.chromium.org/1922373002/diff/70001/ui/gfx/range/gfx_range_...
ui/gfx/range/gfx_range_export.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
nit: 2016

[Fady Samuel, 2016-04-27 18:11:09]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-04-27 18:11:09]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1922373002/#ps90001
(title: "Fixed windows issues + addressed nit")

[commit-bot: I haz the power, 2016-04-27 18:11:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922373002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922373002/90001

[Fady Samuel, 2016-04-27 18:14:47]

fsamuel@chromium.org changed reviewers:
+ jam@chromium.org

[Fady Samuel, 2016-04-27 18:14:48]

+jam@ for content/ and other things not covered by sky@'s review.

[Fady Samuel, 2016-04-27 18:14:51]

+jam@ for content/ and other things not covered by sky@'s review.

[commit-bot: I haz the power, 2016-04-27 18:20:25]

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

[commit-bot: I haz the power, 2016-04-27 18:20:26]

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

[danakj, 2016-04-27 18:21:31]

On 2016/04/27 17:50:39, sky wrote:
> LGTM

sky@ I'd like to get your thoughts on the direction to take ui/gfx/ in the
future. The reason behind this patch is to prevent ui/gfx/ipc/ from depending on
ui/gfx/ because we need to be able to depend on the parts of ui/gfx/ that don't
depend on skia from NaCl code.

Long term, as we add more things to the ipc/ dir, do you see us moving
everything in the root ui/gfx/ into components so that ui/gfx/ipc/ can avoid a
transitive skia dependency?

Put another way I can see at least 2 ways to consider ui/gfx/
1. ui/gfx/ depends on skia. subcomponents may avoid deps on skia.
2. ui/gfx/ does not depend on skia. subcomponents may dep on skia.

Do you have any thoughts here?

[jam, 2016-04-27 19:38:10]

lgtm

[Fady Samuel, 2016-04-27 19:55:01]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-04-27 19:55:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922373002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922373002/90001

[commit-bot: I haz the power, 2016-04-27 21:03:59]

Description was changed from

==========
Make ui/gfx/range a component

In a long chain of issues to address to get mojo to serialize
ui/gfx types for the gpu_host channel in Mus, I've found
myself needing to decouple skia parts of ui/gfx from non-skia
parts. This CL is one step along the way.

BUG=586390
==========

to

==========
Make ui/gfx/range a component

In a long chain of issues to address to get mojo to serialize
ui/gfx types for the gpu_host channel in Mus, I've found
myself needing to decouple skia parts of ui/gfx from non-skia
parts. This CL is one step along the way.

BUG=586390
==========

[commit-bot: I haz the power, 2016-04-27 21:04:01]

Committed patchset #6 (id:90001)

[sky, 2016-04-27 21:07:38]

My guess is the parts of ui/gfx that use skia also use other parts of
ui/gfx that don't use skia. For example, ui/gfx/canvas makes use of
ui/gfx/geometry/rect. It seems like we should have a layer that has no
skia and what is gfx has a dep on the no-skia layer. Make sense? I
believe I'm saying 1.

  -Scott

On Wed, Apr 27, 2016 at 11:21 AM,  <danakj@chromium.org> wrote:
> On 2016/04/27 17:50:39, sky wrote:
>> LGTM
>
> sky@ I'd like to get your thoughts on the direction to take ui/gfx/ in the
> future. The reason behind this patch is to prevent ui/gfx/ipc/ from
> depending on
> ui/gfx/ because we need to be able to depend on the parts of ui/gfx/ that
> don't
> depend on skia from NaCl code.
>
> Long term, as we add more things to the ipc/ dir, do you see us moving
> everything in the root ui/gfx/ into components so that ui/gfx/ipc/ can avoid
> a
> transitive skia dependency?
>
> Put another way I can see at least 2 ways to consider ui/gfx/
> 1. ui/gfx/ depends on skia. subcomponents may avoid deps on skia.
> 2. ui/gfx/ does not depend on skia. subcomponents may dep on skia.
>
> Do you have any thoughts here?
>
> https://codereview.chromium.org/1922373002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2016-04-27 22:19:00]

On 2016/04/27 21:07:38, sky wrote:
> My guess is the parts of ui/gfx that use skia also use other parts of
> ui/gfx that don't use skia. For example, ui/gfx/canvas makes use of
> ui/gfx/geometry/rect. It seems like we should have a layer that has no
> skia and what is gfx has a dep on the no-skia layer. Make sense? I
> believe I'm saying 1.
> 
>   -Scott
> 
> On Wed, Apr 27, 2016 at 11:21 AM,  <mailto:danakj@chromium.org> wrote:
> > On 2016/04/27 17:50:39, sky wrote:
> >> LGTM
> >
> > sky@ I'd like to get your thoughts on the direction to take ui/gfx/ in the
> > future. The reason behind this patch is to prevent ui/gfx/ipc/ from
> > depending on
> > ui/gfx/ because we need to be able to depend on the parts of ui/gfx/ that
> > don't
> > depend on skia from NaCl code.
> >
> > Long term, as we add more things to the ipc/ dir, do you see us moving
> > everything in the root ui/gfx/ into components so that ui/gfx/ipc/ can avoid
> > a
> > transitive skia dependency?
> >
> > Put another way I can see at least 2 ways to consider ui/gfx/
> > 1. ui/gfx/ depends on skia. subcomponents may avoid deps on skia.
> > 2. ui/gfx/ does not depend on skia. subcomponents may dep on skia.
> >
> > Do you have any thoughts here?
> >
> > https://codereview.chromium.org/1922373002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

For the sake of sanity, I think it makes sense for ui/gfx/ipc to mirror the
directory structure of ui/gfx.

We can do this incrementally as needed. In the limit, ui/gfx/ipc/geometry for
ui/gfx/geometry, ui/gfx/ipc/range for ui/gfx/range, etc.

It makes no sense to depend on ui/gfx/ipc/geometry for example without depending
on ui/gfx/geometry, so these may end up all being public_deps in gn world.

WDYT?

[sky, 2016-04-27 23:47:56]

Are you sure you don't want an ipc directory for each directory? So,
instead of ui/gfx/ipc/geometry you would have ui/gfx/geometry/ipc.

On Wed, Apr 27, 2016 at 3:19 PM,  <fsamuel@chromium.org> wrote:
> On 2016/04/27 21:07:38, sky wrote:
>> My guess is the parts of ui/gfx that use skia also use other parts of
>> ui/gfx that don't use skia. For example, ui/gfx/canvas makes use of
>> ui/gfx/geometry/rect. It seems like we should have a layer that has no
>> skia and what is gfx has a dep on the no-skia layer. Make sense? I
>> believe I'm saying 1.
>>
>> -Scott
>>
>> On Wed, Apr 27, 2016 at 11:21 AM, <mailto:danakj@chromium.org> wrote:
>> > On 2016/04/27 17:50:39, sky wrote:
>> >> LGTM
>> >
>> > sky@ I'd like to get your thoughts on the direction to take ui/gfx/ in
>> > the
>> > future. The reason behind this patch is to prevent ui/gfx/ipc/ from
>> > depending on
>> > ui/gfx/ because we need to be able to depend on the parts of ui/gfx/
>> > that
>> > don't
>> > depend on skia from NaCl code.
>> >
>> > Long term, as we add more things to the ipc/ dir, do you see us moving
>> > everything in the root ui/gfx/ into components so that ui/gfx/ipc/ can
>> > avoid
>> > a
>> > transitive skia dependency?
>> >
>> > Put another way I can see at least 2 ways to consider ui/gfx/
>> > 1. ui/gfx/ depends on skia. subcomponents may avoid deps on skia.
>> > 2. ui/gfx/ does not depend on skia. subcomponents may dep on skia.
>> >
>> > Do you have any thoughts here?
>> >
>> > https://codereview.chromium.org/1922373002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> For the sake of sanity, I think it makes sense for ui/gfx/ipc to mirror the
> directory structure of ui/gfx.
>
> We can do this incrementally as needed. In the limit, ui/gfx/ipc/geometry
> for
> ui/gfx/geometry, ui/gfx/ipc/range for ui/gfx/range, etc.
>
> It makes no sense to depend on ui/gfx/ipc/geometry for example without
> depending
> on ui/gfx/geometry, so these may end up all being public_deps in gn world.
>
> WDYT?
>
> https://codereview.chromium.org/1922373002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-04-28 02:24:37]

On 2016/04/27 22:19:00, Fady Samuel wrote:
> On 2016/04/27 21:07:38, sky wrote:
> > My guess is the parts of ui/gfx that use skia also use other parts of
> > ui/gfx that don't use skia. For example, ui/gfx/canvas makes use of
> > ui/gfx/geometry/rect. It seems like we should have a layer that has no
> > skia and what is gfx has a dep on the no-skia layer. Make sense? I
> > believe I'm saying 1.
> > 
> >   -Scott
> > 
> > On Wed, Apr 27, 2016 at 11:21 AM,  <mailto:danakj@chromium.org> wrote:
> > > On 2016/04/27 17:50:39, sky wrote:
> > >> LGTM
> > >
> > > sky@ I'd like to get your thoughts on the direction to take ui/gfx/ in the
> > > future. The reason behind this patch is to prevent ui/gfx/ipc/ from
> > > depending on
> > > ui/gfx/ because we need to be able to depend on the parts of ui/gfx/ that
> > > don't
> > > depend on skia from NaCl code.
> > >
> > > Long term, as we add more things to the ipc/ dir, do you see us moving
> > > everything in the root ui/gfx/ into components so that ui/gfx/ipc/ can
avoid
> > > a
> > > transitive skia dependency?
> > >
> > > Put another way I can see at least 2 ways to consider ui/gfx/
> > > 1. ui/gfx/ depends on skia. subcomponents may avoid deps on skia.
> > > 2. ui/gfx/ does not depend on skia. subcomponents may dep on skia.
> > >
> > > Do you have any thoughts here?
> > >
> > > https://codereview.chromium.org/1922373002/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> For the sake of sanity, I think it makes sense for ui/gfx/ipc to mirror the
> directory structure of ui/gfx.
> 
> We can do this incrementally as needed. In the limit, ui/gfx/ipc/geometry for
> ui/gfx/geometry, ui/gfx/ipc/range for ui/gfx/range, etc.
> 
> It makes no sense to depend on ui/gfx/ipc/geometry for example without
depending
> on ui/gfx/geometry, so these may end up all being public_deps in gn world.
> 
> WDYT?

That sounds good. I was worried about these being opposites ya.

[danakj, 2016-04-28 02:25:59]

On 2016/04/27 23:47:56, sky wrote:
> Are you sure you don't want an ipc directory for each directory? So,
> instead of ui/gfx/ipc/geometry you would have ui/gfx/geometry/ipc.

Hm, not every directory is a separate component, so we'd have gfx/ipc refer to
some subdirs but not others. For that reason I mildly prefer gfx/ipc/<directory
structure that is subset of gfx/>. I'd call them about the same tho so whatever
works.

[sky, 2016-04-28 15:47:14]

I'm ok either way. I mildly prefer ipc per dir, but if that does't
make sense the single ipc dir is fine by me.

  -Scott

On Wed, Apr 27, 2016 at 7:25 PM,  <danakj@chromium.org> wrote:
> On 2016/04/27 23:47:56, sky wrote:
>> Are you sure you don't want an ipc directory for each directory? So,
>> instead of ui/gfx/ipc/geometry you would have ui/gfx/geometry/ipc.
>
> Hm, not every directory is a separate component, so we'd have gfx/ipc refer
> to
> some subdirs but not others. For that reason I mildly prefer
> gfx/ipc/<directory
> structure that is subset of gfx/>. I'd call them about the same tho so
> whatever
> works.
>
> https://codereview.chromium.org/1922373002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-04-30 17:13:05]

Patchset 6 (id:??) landed as
https://crrev.com/6f6dea38646e3fc4860fe478bd6f99fec70cdb4e
Cr-Commit-Position: refs/heads/master@{#390183}

[commit-bot: I haz the power, 2016-04-30 17:51:52]

Description was changed from

==========
Make ui/gfx/range a component

In a long chain of issues to address to get mojo to serialize
ui/gfx types for the gpu_host channel in Mus, I've found
myself needing to decouple skia parts of ui/gfx from non-skia
parts. This CL is one step along the way.

BUG=586390
==========

to

==========
Make ui/gfx/range a component

In a long chain of issues to address to get mojo to serialize
ui/gfx types for the gpu_host channel in Mus, I've found
myself needing to decouple skia parts of ui/gfx from non-skia
parts. This CL is one step along the way.

BUG=586390

Committed: https://crrev.com/6f6dea38646e3fc4860fe478bd6f99fec70cdb4e
Cr-Commit-Position: refs/heads/master@{#390183}
==========