Geometry plans

32 views
Skip to first unread message

Dana Jansens

unread,
Jun 29, 2017, 12:39:05 PM6/29/17
to platform-architecture-dev, Kentaro Hara, enne, piman, Daniel Cheng
Hiya team.

I looked at the Onion Soup 2.0 doc, and was happy to see "The wtf/ layer, the web/ layer, the public APIs and the Web types will be gone."  From the perspective of the graphics team, a major pain point to collapsing old abstractions are the geometry types. We have today..

In blink platform:
- Int{Rect,Size,Point}
- Float{Rect,Size,Point}
- FloatPoint3D
- FloatQuad

In public platform:
- Web{Rect,Size,Point}
- WebFloat{Size,Point,Point3D}
- WebDouble{Size,Point}

In gfx:
- Rect,Size,Point,Vector2d
- {Rect,Size,Point,Vector2d}F
- Point3F
- QuadF
- ScrollOffset

At one point the compositor lived in Source/WebCore/platform/graphics/chromium, and used blink's Int/Float geometry types. When we moved the compositor out, we audited the gfx geometry types and found them to be lacking in functionality and performance. Our initial plan was to reimplement the blink types in //cc as CCMath types, as seen in the initial proposal of this bug.

We had some strong support from piman@ at the time to instead change gfx types to work for us. We managed to convince the owners there to let us make changes to gfx types to provide the functionality, performance, and type-conversion-safety that we needed and had expected from the blink types.

One design decision we made while boiling the ocean, and this diverged us from the blink types, was to stop using Size or Point types to represent 2d vectors. There is some debate on the bug about this, which you may find interesting. I think this captures the reasoning behind the decision.

Because we no longer allowed size+size or point+point, this required non-mechanical changes, making logical decisions about what was a size and what was a vector. Mostly this was not difficult at all, and led to improved readability and typesafety in the code. It also led to our ability to disallow negative Sizes.

There's a long series of behaviour and API fixes that follow on that bug.

Since then, we also converted scrolling offsets away from Vector2d to be a separate ScrollOffset class, since we wanted the ability to use doubles for them. That bug shows the process at which we decided on the representation. There is a TODO about reconciling this with blink, which would be resolved if we switched FloatPoints to ScrollOffsets there as appropriate.

I wanted to give this history to help motivate moving blink to use gfx types. I think that most of the work has already been done here. Layout types appeared in blink since then, so we'd need Layout <-> gfx conversions instead of Layout <-> blinkgeom conversions. If we can achieve that, then the Web geometry types disappear, and we lose 

Has anyone on the onion soup team given this some thought yet? What do you think, given the above context?

Thanks,
Dana



Stuart Langley

unread,
Jun 30, 2017, 1:01:49 AM6/30/17
to Dana Jansens, platform-architecture-dev, Kentaro Hara, enne, piman, Daniel Cheng
Hi Dana,

Thanks for the mail. i think you might not have finished a sentence? "If we can achieve that, then the Web geometry types disappear, and we lose" - was there meant to be anything additional here?

In any case, I think I understand your advocating for replacing a bunch of types currently defined in platform/geometry/ with types currently defined in gfx/geometry/ and we'll delete the types in public/platform/ as part of onion soup. Is that correct?

I don't know why we'd want more than one geometry type, so this sounds reasonable to me. Perhaps people with more substantial history might be able to provide insights as to why we wouldn't want to do this.

But assuming we do - do you have any ideas on how to make this possible? Would we move these types to //base?

Cheers,
Stuart




--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHtyhaTz6A1WTcZh%2BXxjTh9OrCKLLSz27ezDwgHnm9v6egzOgQ%40mail.gmail.com.

Daniel Cheng

unread,
Jun 30, 2017, 1:18:10 AM6/30/17
to Stuart Langley, Dana Jansens, platform-architecture-dev, Kentaro Hara, enne, piman
On Thu, Jun 29, 2017 at 10:01 PM Stuart Langley <slan...@chromium.org> wrote:
Hi Dana,

Thanks for the mail. i think you might not have finished a sentence? "If we can achieve that, then the Web geometry types disappear, and we lose" - was there meant to be anything additional here?

In any case, I think I understand your advocating for replacing a bunch of types currently defined in platform/geometry/ with types currently defined in gfx/geometry/ and we'll delete the types in public/platform/ as part of onion soup. Is that correct?

I don't know why we'd want more than one geometry type, so this sounds reasonable to me. Perhaps people with more substantial history might be able to provide insights as to why we wouldn't want to do this.

But assuming we do - do you have any ideas on how to make this possible? Would we move these types to //base?

I think we should just allow //ui/gfx/geometry to be used directly in Blink, without having to move them to //base. It looks largely self-contained.

From the perspective of mojo IPC, cleaning this up would also be nice to make validation more uniform: today, the Blink size types allow negative widths, while the gfx ones do not, etc.

Daniel
 
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CALFY5Zz%3DPv6FkM7Xih1nhja00ovcwaunFkMokQnkdJS61czWiA%40mail.gmail.com.

Stuart Langley

unread,
Jun 30, 2017, 3:04:37 AM6/30/17
to Daniel Cheng, Dana Jansens, platform-architecture-dev, Kentaro Hara, enne, piman
I don't feel strongly about it, but if we believe that these types are generic enough then perhaps they should be homed somewhere besides //ui/... 

Perhaps the correct way forward is to open a bug and put this detail in there, and then we can iterate. Dana, would you like to do that?

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

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

Nico Weber

unread,
Jun 30, 2017, 8:03:18 AM6/30/17
to Stuart, Daniel Cheng, Antoine Labour, Kentaro Hara, platform-architecture-dev, enne, Dana Jansens


On Jun 30, 2017 3:04 AM, "Stuart Langley" <slan...@chromium.org> wrote:
I don't feel strongly about it, but if we believe that these types are generic enough then perhaps they should be homed somewhere besides //ui/... 

//ui/ contains various generic libraries useful when doing ui-y things. What's the problem with making blink depend on //ui/gfx?

(I don't feel terribly strongly either, but I don't understand the concern yet.)

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.

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

To post to this group, send email to platform-architecture-dev@chromium.org.

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

Jeremy Roman

unread,
Jun 30, 2017, 10:19:20 AM6/30/17
to Nico Weber, Stuart, Daniel Cheng, Antoine Labour, Kentaro Hara, platform-architecture-dev, enne, Dana Jansens
I think it's something that it'd be nice if it were done, but it's a little tricky:
- need to reconcile Layout* geometry types (probably want them to have a consistent interface with the integer and float geometry types used in Blink, at least)
- in principle it should be similar, but there's a bunch of perf-sensitive code that uses geometry types that we'd have to watch for regression (and the gfx ones do some checks that the Blink ones do not, IIRC)
- tons of uses that would need to be rewritten

It's less thorny than unifying with the Skia geometry types (because at least we're consistent about whether we store left/right or left/width, etc.), but it's mostly a lot of work and it's not apparent to me whether the benefits (compared to just having conversion operators to make it easy to go back and forth) are worth the effort. I'm not opposed to someone doing it if they wish to, though.

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

Dana Jansens

unread,
Jun 30, 2017, 11:49:26 AM6/30/17
to Jeremy Roman, Nico Weber, Stuart, Daniel Cheng, Antoine Labour, Kentaro Hara, platform-architecture-dev, enne
On Fri, Jun 30, 2017 at 10:19 AM, Jeremy Roman <jbr...@chromium.org> wrote:
I think it's something that it'd be nice if it were done, but it's a little tricky:
- need to reconcile Layout* geometry types (probably want them to have a consistent interface with the integer and float geometry types used in Blink, at least)
- in principle it should be similar, but there's a bunch of perf-sensitive code that uses geometry types that we'd have to watch for regression (and the gfx ones do some checks that the Blink ones do not, IIRC)
- tons of uses that would need to be rewritten

It's less thorny than unifying with the Skia geometry types (because at least we're consistent about whether we store left/right or left/width, etc.), but it's mostly a lot of work and it's not apparent to me whether the benefits (compared to just having conversion operators to make it easy to go back and forth) are worth the effort. I'm not opposed to someone doing it if they wish to, though.

By having different types, it forces us to have different APIs as well. You can't have a FooApi written in gfx::Rects and call that from blink, you have to have a blink API written in IntRects that converts to gfx::Rects and calls the other API. So this results in extra code and friction across the whole graphics stack. We'd lose this friction if we combined types.

We could get rid of Web geometry types, which forces a 3rd API layer in between the other two, before doing the full conversion by letting the blink geometry types include ui/gfx/geometry and allow conversions to gfx:: types directly.
 

On Fri, Jun 30, 2017 at 8:03 AM, 'Nico Weber' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:


On Jun 30, 2017 3:04 AM, "Stuart Langley" <slan...@chromium.org> wrote:
I don't feel strongly about it, but if we believe that these types are generic enough then perhaps they should be homed somewhere besides //ui/... 

//ui/ contains various generic libraries useful when doing ui-y things. What's the problem with making blink depend on //ui/gfx?

//ui/gfx/geometry/ is a separate component from //ui/gfx. I would actually not advocate for depending on //ui/gfx from blink as it has a bunch of conflicting things in there like another implementation of text layout for example.
 

(I don't feel terribly strongly either, but I don't understand the concern yet.)


Perhaps the correct way forward is to open a bug and put this detail in there, and then we can iterate. Dana, would you like to do that?

Thanks for the feedback, I have filed https://bugs.chromium.org/p/chromium/issues/detail?id=738465.
 

On Fri, Jun 30, 2017 at 3:17 PM, Daniel Cheng <dch...@chromium.org> wrote:
On Thu, Jun 29, 2017 at 10:01 PM Stuart Langley <slan...@chromium.org> wrote:
Hi Dana,

Thanks for the mail. i think you might not have finished a sentence? "If we can achieve that, then the Web geometry types disappear, and we lose" - was there meant to be anything additional here?

Woops, let me respond above with what I meant to say here, and somehow deleted.
 

In any case, I think I understand your advocating for replacing a bunch of types currently defined in platform/geometry/ with types currently defined in gfx/geometry/ and we'll delete the types in public/platform/ as part of onion soup. Is that correct?

That's right.
 

I don't know why we'd want more than one geometry type, so this sounds reasonable to me. Perhaps people with more substantial history might be able to provide insights as to why we wouldn't want to do this.

But assuming we do - do you have any ideas on how to make this possible? Would we move these types to //base?

I think we should just allow //ui/gfx/geometry to be used directly in Blink, without having to move them to //base. It looks largely self-contained.

I don't think they need to be in base. Like I said above, //ui/gfx/geometry is a component of its own. I've toyed with the idea of moving them to //geometry or something but I think that would be largely cosmetic and I'm not sure of that work's value.
 

Jeremy Roman

unread,
Jun 30, 2017, 1:24:49 PM6/30/17
to Dana Jansens, Nico Weber, Stuart, Daniel Cheng, Antoine Labour, Kentaro Hara, platform-architecture-dev, enne
On Fri, Jun 30, 2017 at 11:49 AM, Dana Jansens <dan...@google.com> wrote:
On Fri, Jun 30, 2017 at 10:19 AM, Jeremy Roman <jbr...@chromium.org> wrote:
I think it's something that it'd be nice if it were done, but it's a little tricky:
- need to reconcile Layout* geometry types (probably want them to have a consistent interface with the integer and float geometry types used in Blink, at least)
- in principle it should be similar, but there's a bunch of perf-sensitive code that uses geometry types that we'd have to watch for regression (and the gfx ones do some checks that the Blink ones do not, IIRC)
- tons of uses that would need to be rewritten

It's less thorny than unifying with the Skia geometry types (because at least we're consistent about whether we store left/right or left/width, etc.), but it's mostly a lot of work and it's not apparent to me whether the benefits (compared to just having conversion operators to make it easy to go back and forth) are worth the effort. I'm not opposed to someone doing it if they wish to, though.

By having different types, it forces us to have different APIs as well. You can't have a FooApi written in gfx::Rects and call that from blink, you have to have a blink API written in IntRects that converts to gfx::Rects and calls the other API. So this results in extra code and friction across the whole graphics stack. We'd lose this friction if we combined types.

Sure you can. Define "IntRect::operator gfx::Rect()" and "IntRect::IntRect(const gfx::Rect&)" to enable implicit conversions, and it should Just Work™ in 98% of cases. I'm not saying I prefer this to a unified world, but it's a middle ground that exists.

Dana Jansens

unread,
Jun 30, 2017, 1:43:55 PM6/30/17
to Jeremy Roman, Nico Weber, Stuart, Daniel Cheng, Antoine Labour, Kentaro Hara, platform-architecture-dev, enne
On Fri, Jun 30, 2017 at 1:24 PM, Jeremy Roman <jbr...@chromium.org> wrote:


On Fri, Jun 30, 2017 at 11:49 AM, Dana Jansens <dan...@google.com> wrote:
On Fri, Jun 30, 2017 at 10:19 AM, Jeremy Roman <jbr...@chromium.org> wrote:
I think it's something that it'd be nice if it were done, but it's a little tricky:
- need to reconcile Layout* geometry types (probably want them to have a consistent interface with the integer and float geometry types used in Blink, at least)
- in principle it should be similar, but there's a bunch of perf-sensitive code that uses geometry types that we'd have to watch for regression (and the gfx ones do some checks that the Blink ones do not, IIRC)
- tons of uses that would need to be rewritten

It's less thorny than unifying with the Skia geometry types (because at least we're consistent about whether we store left/right or left/width, etc.), but it's mostly a lot of work and it's not apparent to me whether the benefits (compared to just having conversion operators to make it easy to go back and forth) are worth the effort. I'm not opposed to someone doing it if they wish to, though.

By having different types, it forces us to have different APIs as well. You can't have a FooApi written in gfx::Rects and call that from blink, you have to have a blink API written in IntRects that converts to gfx::Rects and calls the other API. So this results in extra code and friction across the whole graphics stack. We'd lose this friction if we combined types.

Sure you can. Define "IntRect::operator gfx::Rect()" and "IntRect::IntRect(const gfx::Rect&)" to enable implicit conversions, and it should Just Work™ in 98% of cases. I'm not saying I prefer this to a unified world, but it's a middle ground that exists.

Yeah, I tried to mention that elsewhere, and it's a possible step in a migration plan, this would let us get rid of the Web type conversions in between blink and gfx types. But it would require access to ui/gfx/geometry from blink to do that.

Jeremy Roman

unread,
Jun 30, 2017, 1:47:43 PM6/30/17
to Dana Jansens, Nico Weber, Stuart, Daniel Cheng, Antoine Labour, Kentaro Hara, platform-architecture-dev, enne
On Fri, Jun 30, 2017 at 1:43 PM, Dana Jansens <dan...@google.com> wrote:
On Fri, Jun 30, 2017 at 1:24 PM, Jeremy Roman <jbr...@chromium.org> wrote:


On Fri, Jun 30, 2017 at 11:49 AM, Dana Jansens <dan...@google.com> wrote:
On Fri, Jun 30, 2017 at 10:19 AM, Jeremy Roman <jbr...@chromium.org> wrote:
I think it's something that it'd be nice if it were done, but it's a little tricky:
- need to reconcile Layout* geometry types (probably want them to have a consistent interface with the integer and float geometry types used in Blink, at least)
- in principle it should be similar, but there's a bunch of perf-sensitive code that uses geometry types that we'd have to watch for regression (and the gfx ones do some checks that the Blink ones do not, IIRC)
- tons of uses that would need to be rewritten

It's less thorny than unifying with the Skia geometry types (because at least we're consistent about whether we store left/right or left/width, etc.), but it's mostly a lot of work and it's not apparent to me whether the benefits (compared to just having conversion operators to make it easy to go back and forth) are worth the effort. I'm not opposed to someone doing it if they wish to, though.

By having different types, it forces us to have different APIs as well. You can't have a FooApi written in gfx::Rects and call that from blink, you have to have a blink API written in IntRects that converts to gfx::Rects and calls the other API. So this results in extra code and friction across the whole graphics stack. We'd lose this friction if we combined types.

Sure you can. Define "IntRect::operator gfx::Rect()" and "IntRect::IntRect(const gfx::Rect&)" to enable implicit conversions, and it should Just Work™ in 98% of cases. I'm not saying I prefer this to a unified world, but it's a middle ground that exists.

Yeah, I tried to mention that elsewhere, and it's a possible step in a migration plan, this would let us get rid of the Web type conversions in between blink and gfx types. But it would require access to ui/gfx/geometry from blink to do that.

ui/gfx/geometry/ is permitted in platform/geometry/ at present (and in fact some of these conversions exist). But yeah, it would end up (implicitly) using gfx:: from more places in Blink.

Kentaro Hara

unread,
Jul 2, 2017, 5:21:26 AM7/2/17
to Jeremy Roman, Dana Jansens, Nico Weber, Stuart, Daniel Cheng, Antoine Labour, platform-architecture-dev, enne
One of the goals of Onion Soup 2.0 is to reduce the friction between the Chromium code base and the Blink code base. We want to share as many types and code as possible. So, +1 to sharing the gfx types between Chromium and Blink.

Also I think it's fine to make Blink depend on //ui/gfx/geometry/ as long as //ui/gfx/geometry/ is self-contained. (IMHO I don't think we need to move things to //base/ to let Blink depend on the things. What matters is just that the things Blink depends on are well self-contained.)



--
Kentaro Hara, Tokyo, Japan
Reply all
Reply to author
Forward
0 new messages