Where to put an enum...

48 views
Skip to first unread message

dli...@microsoft.com

unread,
May 3, 2019, 6:20:56 PM5/3/19
to platform-architecture-dev, bo...@chromium.org, ara...@microsoft.com
We've been doing some work for scrollbar scrolling and would like to share an enum in a few places in the code that generally don't have the same dependencies.

WebScrollGranularity is currently duplicated into ui/events/gesture_event_details.h, since ui/events can't depend on blink. arakeri@ has a change where he'd like to use the enum from cc/ as well in order to specify the units for a scroll delta computed based on the results of hit testing a mouse event against the scrollbar layer.

What would be the best place to put an enum that can be accessed from all of those locations? base/ doesn't seem like a great fit - should we carve out a new spot in ui/? I'd prefer not to add another duplicated enum if possible.


Kentaro Hara

unread,
May 3, 2019, 8:51:59 PM5/3/19
to Daniel Libby, platform-architecture-dev, David Bokan, ara...@microsoft.com
Technically I think we can put the enum in //cc/ because Blink and //ui/ can depend on cc/.

I'm not sure if it conceptually makes sense -- David, do you have any insight? :)



--
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/ed1b5466-f44a-4362-895f-0522b3b01e1c%40chromium.org.


--
Kentaro Hara, Tokyo, Japan

David Bokan

unread,
May 6, 2019, 9:10:18 AM5/6/19
to Kentaro Hara, Daniel Libby, platform-architecture-dev, Rahul Arakeri
Right, I missed that //ui/ can depend on //cc/. It does feel a little awkward that cc define this type since it's a property of the blink input event, but I don't really have a better idea. There is also precedent where a number of similar types are defined in //cc/ and used by both Blink and //ui/, e.g. overscroll_behavior.h, main_thread_scrolling_reason.h, etc so I think that's probably the best existing place for it, unless anyone has a better idea.

dan...@chromium.org

unread,
May 7, 2019, 11:16:55 AM5/7/19
to David Bokan, Kentaro Hara, Daniel Libby, platform-architecture-dev, Rahul Arakeri
On Mon, May 6, 2019 at 9:10 AM David Bokan <bo...@chromium.org> wrote:
Right, I missed that //ui/ can depend on //cc/. It does feel a little awkward that cc define this type since it's a property of the blink input event, but I don't really have a better idea. There is also precedent where a number of similar types are defined in //cc/ and used by both Blink and //ui/, e.g. overscroll_behavior.h, main_thread_scrolling_reason.h, etc so I think that's probably the best existing place for it, unless anyone has a better idea.

I would not like to see //cc become "base for blink and ui shared code". Can we have blink depend on ui/events/ or make a new directory in ui/events/<here> or elsewhere that blink can depend on?
 


On Fri, May 3, 2019 at 8:52 PM Kentaro Hara <har...@chromium.org> wrote:
Technically I think we can put the enum in //cc/ because Blink and //ui/ can depend on cc/.

I'm not sure if it conceptually makes sense -- David, do you have any insight? :)



On Sat, May 4, 2019 at 7:20 AM dlibby via platform-architecture-dev <platform-arc...@chromium.org> wrote:
We've been doing some work for scrollbar scrolling and would like to share an enum in a few places in the code that generally don't have the same dependencies.

WebScrollGranularity is currently duplicated into ui/events/gesture_event_details.h, since ui/events can't depend on blink. arakeri@ has a change where he'd like to use the enum from cc/ as well in order to specify the units for a scroll delta computed based on the results of hit testing a mouse event against the scrollbar layer.

What would be the best place to put an enum that can be accessed from all of those locations? base/ doesn't seem like a great fit - should we carve out a new spot in ui/? I'd prefer not to add another duplicated enum if possible.


--
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/ed1b5466-f44a-4362-895f-0522b3b01e1c%40chromium.org.


--
Kentaro Hara, Tokyo, Japan

--
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.

Kentaro Hara

unread,
May 7, 2019, 11:20:43 AM5/7/19
to Dana Jansens, David Bokan, Daniel Libby, platform-architecture-dev, Rahul Arakeri
I would not like to see //cc become "base for blink and ui shared code". Can we have blink depend on ui/events/ or make a new directory in ui/events/<here> or elsewhere that blink can depend on?

I'm fine with it as long as the ui/events/<here> is clearly designed not to contain random things Blink should not depend on :)

(FWIW Blink's accessibility code is thinking about creating a dependency to ui/accessibility/<somewhere>.)


Dave Tapuska

unread,
May 7, 2019, 11:24:37 AM5/7/19
to Kentaro Hara, Dana Jansens, David Bokan, Daniel Libby, platform-architecture-dev, Rahul Arakeri
ui/base/page_transition_types.h is allowed to be included in platform according to the current DEPS. As long as the header is restricted to an enumeration seems ok to me.

dave.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jw_Hdr31_V07k2kMZ4%3DFVVmr40f%2B_233Gt0D%2BmKtdotGw%40mail.gmail.com.

bo...@chromium.org

unread,
May 16, 2019, 1:24:25 PM5/16/19
to platform-architecture-dev, har...@chromium.org, dan...@chromium.org, bo...@chromium.org, dli...@microsoft.com, ara...@microsoft.com
I agree it feels better to have Blink and CC depend on UI event-types rather than having CC be the base (since the events originate in UI and flow into Blink and CC). What does everyone think of adding //ui/events/types (or common?) and putting event-related types like this that are usable across the whole pipeline there?

On Tuesday, May 7, 2019 at 11:24:37 AM UTC-4, Dave Tapuska wrote:
ui/base/page_transition_types.h is allowed to be included in platform according to the current DEPS. As long as the header is restricted to an enumeration seems ok to me.

dave.

On Tue, May 7, 2019 at 11:20 AM Kentaro Hara <har...@chromium.org> wrote:
I would not like to see //cc become "base for blink and ui shared code". Can we have blink depend on ui/events/ or make a new directory in ui/events/<here> or elsewhere that blink can depend on?

I'm fine with it as long as the ui/events/<here> is clearly designed not to contain random things Blink should not depend on :)

(FWIW Blink's accessibility code is thinking about creating a dependency to ui/accessibility/<somewhere>.)


On Wed, May 8, 2019 at 12:16 AM <dan...@chromium.org> wrote:
On Mon, May 6, 2019 at 9:10 AM David Bokan <bo...@chromium.org> wrote:
Right, I missed that //ui/ can depend on //cc/. It does feel a little awkward that cc define this type since it's a property of the blink input event, but I don't really have a better idea. There is also precedent where a number of similar types are defined in //cc/ and used by both Blink and //ui/, e.g. overscroll_behavior.h, main_thread_scrolling_reason.h, etc so I think that's probably the best existing place for it, unless anyone has a better idea.

I would not like to see //cc become "base for blink and ui shared code". Can we have blink depend on ui/events/ or make a new directory in ui/events/<here> or elsewhere that blink can depend on?
 
On Fri, May 3, 2019 at 8:52 PM Kentaro Hara <har...@chromium.org> wrote:
Technically I think we can put the enum in //cc/ because Blink and //ui/ can depend on cc/.

I'm not sure if it conceptually makes sense -- David, do you have any insight? :)



On Sat, May 4, 2019 at 7:20 AM dlibby via platform-architecture-dev <platform-arc...@chromium.org> wrote:
We've been doing some work for scrollbar scrolling and would like to share an enum in a few places in the code that generally don't have the same dependencies.

WebScrollGranularity is currently duplicated into ui/events/gesture_event_details.h, since ui/events can't depend on blink. arakeri@ has a change where he'd like to use the enum from cc/ as well in order to specify the units for a scroll delta computed based on the results of hit testing a mouse event against the scrollbar layer.

What would be the best place to put an enum that can be accessed from all of those locations? base/ doesn't seem like a great fit - should we carve out a new spot in ui/? I'd prefer not to add another duplicated enum if possible.


--
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.


--
Kentaro Hara, Tokyo, Japan

--
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.

--
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.


--
Kentaro Hara, Tokyo, Japan

--
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.

dan...@chromium.org

unread,
May 16, 2019, 1:30:57 PM5/16/19
to David Bokan, platform-architecture-dev, Kentaro Hara, Daniel Libby, Rahul Arakeri
On Thu, May 16, 2019 at 1:24 PM <bo...@chromium.org> wrote:
I agree it feels better to have Blink and CC depend on UI event-types rather than having CC be the base (since the events originate in UI and flow into Blink and CC). What does everyone think of adding //ui/events/types (or common?) and putting event-related types like this that are usable across the whole pipeline there?

"common" is typically used to mean shared between processes so i'd avoid that. types sounds fine to me!
 

On Tuesday, May 7, 2019 at 11:24:37 AM UTC-4, Dave Tapuska wrote:
ui/base/page_transition_types.h is allowed to be included in platform according to the current DEPS. As long as the header is restricted to an enumeration seems ok to me.

dave.

On Tue, May 7, 2019 at 11:20 AM Kentaro Hara <har...@chromium.org> wrote:
I would not like to see //cc become "base for blink and ui shared code". Can we have blink depend on ui/events/ or make a new directory in ui/events/<here> or elsewhere that blink can depend on?

I'm fine with it as long as the ui/events/<here> is clearly designed not to contain random things Blink should not depend on :)

(FWIW Blink's accessibility code is thinking about creating a dependency to ui/accessibility/<somewhere>.)


On Wed, May 8, 2019 at 12:16 AM <dan...@chromium.org> wrote:
On Mon, May 6, 2019 at 9:10 AM David Bokan <bo...@chromium.org> wrote:
Right, I missed that //ui/ can depend on //cc/. It does feel a little awkward that cc define this type since it's a property of the blink input event, but I don't really have a better idea. There is also precedent where a number of similar types are defined in //cc/ and used by both Blink and //ui/, e.g. overscroll_behavior.h, main_thread_scrolling_reason.h, etc so I think that's probably the best existing place for it, unless anyone has a better idea.

I would not like to see //cc become "base for blink and ui shared code". Can we have blink depend on ui/events/ or make a new directory in ui/events/<here> or elsewhere that blink can depend on?
 
On Fri, May 3, 2019 at 8:52 PM Kentaro Hara <har...@chromium.org> wrote:
Technically I think we can put the enum in //cc/ because Blink and //ui/ can depend on cc/.

I'm not sure if it conceptually makes sense -- David, do you have any insight? :)



On Sat, May 4, 2019 at 7:20 AM dlibby via platform-architecture-dev <platform-arc...@chromium.org> wrote:
We've been doing some work for scrollbar scrolling and would like to share an enum in a few places in the code that generally don't have the same dependencies.

WebScrollGranularity is currently duplicated into ui/events/gesture_event_details.h, since ui/events can't depend on blink. arakeri@ has a change where he'd like to use the enum from cc/ as well in order to specify the units for a scroll delta computed based on the results of hit testing a mouse event against the scrollbar layer.

What would be the best place to put an enum that can be accessed from all of those locations? base/ doesn't seem like a great fit - should we carve out a new spot in ui/? I'd prefer not to add another duplicated enum if possible.


--
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.


--
Kentaro Hara, Tokyo, Japan

--
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.

--
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.


--
Kentaro Hara, Tokyo, Japan

--
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.
Reply all
Reply to author
Forward
0 new messages