Allowing //base types in core/ and modules/

82 views
Skip to first unread message

Kentaro Hara

unread,
May 23, 2017, 8:04:28 AM5/23/17
to platform-architecture-dev, Yuta Kitamura, John Abd-El-Malek, Daniel Cheng
Hi

TL;DR: I'd propose allowing //base types in core/ and modules/.

Currently //base types are not allowed in core/ and modules/. If core/ and modules/ want to use //base types, thin wrapper classes need to be defined in platform/. This rule becomes more problematic as we move more code from Chromium to Blink in Onion Soup 2.0. The rule is already causing issues like this, this and this. So I propose allowing //base types in core/ and modules/.

However, we need to be careful not to allow developers to use random //base types. For example, we should prevent developers from using base::scoped_refptr, std::string, std::vector etc because WTF::RefPtr, WTF::String, WTF::Vector etc should be used instead.

The question is how to enforce the rule (see this thread as well). DEPS is not sufficient because DEPS can only check included headers. DEPS cannot check types transitively included. I'm thinking about adding whitelisted //base types and std'::types to the PRESUBMIT script, but will it work?

(It might not be 100% perfect but I think it's fine if it works for 99% of the cases in practice.)

Any thoughts are welcome!

--
Kentaro Hara, Tokyo, Japan

John Abd-El-Malek

unread,
May 23, 2017, 10:03:25 AM5/23/17
to Kentaro Hara, platform-architecture-dev, Yuta Kitamura, Daniel Cheng
These ideas sound good to me. If you want something foolproof, then maybe a clang plugin?

However, isn't the issue of disallowed types being used orthogonal from having core & modules include parts of base? e.g. status quo is that they include wrappers from base/ which do include the base header, and they can also include platform/ headers which do include base headers, so this situation is already possible. Since this hasn't led to use of STL or other parts of base afaik, that seems like a good data point that current mechanisms are sufficient.

Kentaro Hara

unread,
May 23, 2017, 11:06:45 AM5/23/17
to John Abd-El-Malek, platform-architecture-dev, Yuta Kitamura, Daniel Cheng
These ideas sound good to me. If you want something foolproof, then maybe a clang plugin?

Sounds good :)

 
However, isn't the issue of disallowed types being used orthogonal from having core & modules include parts of base? e.g. status quo is that they include wrappers from base/ which do include the base header, and they can also include platform/ headers which do include base headers, so this situation is already possible. Since this hasn't led to use of STL or other parts of base afaik, that seems like a good data point that current mechanisms are sufficient.

Not really. Currently the PRESUBMIT script simply forbids all "base::" types in core/ and modules/ (c.f., see this thread for more background).

Brett Wilson

unread,
May 23, 2017, 11:25:57 AM5/23/17
to Kentaro Hara, platform-architecture-dev, Yuta Kitamura, John Abd-El-Malek, Daniel Cheng
Would a PRESUBMIT with a blacklist be sufficient? It seems easier to maintain and describe than a whitelist, since for each entry in the blacklist there would be a preferred WTF type that one would use instead. There are a lot of weird std helper functions that could theoretically be used without a problem and will be difficult to whitelist.

Brett

On Tue, May 23, 2017 at 5:03 AM, Kentaro Hara <har...@chromium.org> wrote:

--
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/CABg10jzOELHVG_TdqD-Hpf2DQa5%3Db6ioVfc-__M60n_0-pCHSA%40mail.gmail.com.

Brett Wilson

unread,
May 23, 2017, 11:32:50 AM5/23/17
to Kentaro Hara, platform-architecture-dev, Yuta Kitamura, John Abd-El-Malek, Daniel Cheng
On Tue, May 23, 2017 at 8:25 AM, Brett Wilson <bre...@chromium.org> wrote:
Would a PRESUBMIT with a blacklist be sufficient? It seems easier to maintain and describe than a whitelist, since for each entry in the blacklist there would be a preferred WTF type that one would use instead. There are a lot of weird std helper functions that could theoretically be used without a problem and will be difficult to whitelist.

If you do want to move more conservatively with a blacklist, I would define a request process like we have for C++11 features in the style guide so it's clear to random people how to add things to the list.

Brett

Kentaro Hara

unread,
May 23, 2017, 11:38:20 AM5/23/17
to Brett Wilson, platform-architecture-dev, Yuta Kitamura, John Abd-El-Malek, Daniel Cheng
For safety I'd prefer using a whitelist approach. The whiltelist approach will work for base:: types since base:: types used in Blink are limited.

There are a lot of weird std helper functions that could theoretically be used without a problem and will be difficult to whitelist.

Do you have any example? Historically std:: types were disallowed in Blink, so I hope Blink would not be using a lot of complex helper functions.



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

Brett Wilson

unread,
May 23, 2017, 12:33:00 PM5/23/17
to Kentaro Hara, platform-architecture-dev, Yuta Kitamura, John Abd-El-Malek, Daniel Cheng
On Tue, May 23, 2017 at 8:37 AM, Kentaro Hara <har...@chromium.org> wrote:
For safety I'd prefer using a whitelist approach. The whiltelist approach will work for base:: types since base:: types used in Blink are limited.

There are a lot of weird std helper functions that could theoretically be used without a problem and will be difficult to whitelist.

Do you have any example? Historically std:: types were disallowed in Blink, so I hope Blink would not be using a lot of complex helper functions.

I was thinking mostly of algorithms which I thought blink allowed. If you're doing the checking with a presubmit, I was assuming you would be doing a grep for "std::" and that will match all kinds of things that are already in Blink. I'm imagining a weekly post to Blink dev where some random confused person says "I tried to do std::adjacent_find but the presubmit says that's not allowed. Why can't I use that?"

Also: I noticed there are 60 lines with std::vector and 52 with std::string in core (most, but not all, in tests). There are also some in modules. This may make any plans more complicated.

Brett

Jeremy Roman

unread,
May 23, 2017, 1:29:11 PM5/23/17
to Brett Wilson, Kentaro Hara, platform-architecture-dev, Yuta Kitamura, John Abd-El-Malek, Daniel Cheng
We do use a ton of STL in Blink: most of <algorithm> is fine (and I wish it were used more), std::unique_ptr obviously is, etc. From my perspective the potential problems are:
- std::basic_string (string, string16)
- std::vector, std::map, and other containers

These are things that we'd prefer to use WTF's variants for now, because Blink is sensitive to their performance properties, and the containers have special handling for things like Oilpan.

I could see Blink adopting scoped_refptr instead of WTF::RefPtr now that it seems to have grown support for start-at-one ref counts, but until/unless someone makes an effort there we should definitely use WTF::RefPtr for WTF::(ThreadSafe)?RefCounted types and scoped_refptr for base::RefCounted.

For types without such entanglements like base::TimeDelta/TimeTicks, I'd personally be happy to use the base types. Another similar case is tracked_objects::Location/FROM_HERE. Types like this should also be (and in some cases, already are) allowed in the public/ API, while it continues to exist. A whitelist seems like a safer, more conservative approach to avoid the problem case, which would be tons of conversions back and forth between WTF::String and std::string.

Brett

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

Brett Wilson

unread,
May 23, 2017, 1:36:23 PM5/23/17
to Jeremy Roman, Kentaro Hara, platform-architecture-dev, Yuta Kitamura, John Abd-El-Malek, Daniel Cheng
On Tue, May 23, 2017 at 10:29 AM, Jeremy Roman <jbr...@chromium.org> wrote:
On Tue, May 23, 2017 at 12:32 PM, Brett Wilson <bre...@chromium.org> wrote:
On Tue, May 23, 2017 at 8:37 AM, Kentaro Hara <har...@chromium.org> wrote:
For safety I'd prefer using a whitelist approach. The whiltelist approach will work for base:: types since base:: types used in Blink are limited.

There are a lot of weird std helper functions that could theoretically be used without a problem and will be difficult to whitelist.

Do you have any example? Historically std:: types were disallowed in Blink, so I hope Blink would not be using a lot of complex helper functions.

I was thinking mostly of algorithms which I thought blink allowed. If you're doing the checking with a presubmit, I was assuming you would be doing a grep for "std::" and that will match all kinds of things that are already in Blink. I'm imagining a weekly post to Blink dev where some random confused person says "I tried to do std::adjacent_find but the presubmit says that's not allowed. Why can't I use that?"

Also: I noticed there are 60 lines with std::vector and 52 with std::string in core (most, but not all, in tests). There are also some in modules. This may make any plans more complicated.

We do use a ton of STL in Blink: most of <algorithm> is fine (and I wish it were used more), std::unique_ptr obviously is, etc. From my perspective the potential problems are:
- std::basic_string (string, string16)
- std::vector, std::map, and other containers

These are things that we'd prefer to use WTF's variants for now, because Blink is sensitive to their performance properties, and the containers have special handling for things like Oilpan.

I think everybody agrees on the std::string and std::vector policies.
 

I could see Blink adopting scoped_refptr instead of WTF::RefPtr now that it seems to have grown support for start-at-one ref counts, but until/unless someone makes an effort there we should definitely use WTF::RefPtr for WTF::(ThreadSafe)?RefCounted types and scoped_refptr for base::RefCounted.

For types without such entanglements like base::TimeDelta/TimeTicks, I'd personally be happy to use the base types. Another similar case is tracked_objects::Location/FROM_HERE. Types like this should also be (and in some cases, already are) allowed in the public/ API, while it continues to exist. A whitelist seems like a safer, more conservative approach to avoid the problem case, which would be tons of conversions back and forth between WTF::String and std::string.

Blink folks should decide what's best for now. I think either way will be fine if the presubmit error links to clear instructions on the policy and how to update the list.

Brett

Fernando

unread,
May 23, 2017, 2:44:27 PM5/23/17
to platform-architecture-dev, jbr...@chromium.org, har...@chromium.org, yu...@chromium.org, j...@chromium.org, dch...@chromium.org
Would it be reasonable, as a quick first step, to whitelist tests in Blink to allow base:: types?

Daniel Cheng

unread,
May 23, 2017, 2:58:35 PM5/23/17
to Fernando, platform-architecture-dev, jbr...@chromium.org, har...@chromium.org, yu...@chromium.org, j...@chromium.org
For base:
Doing the suggested PRESUBMIT with simple checks of headers + grepping for base::<list of allowed types/functions> seems good to me. I think we've namespaced most significant parts of //base at this point, so I wouldn't be /too/ worried about accidental using not-allowed functionality from headers that are transitively included. It would also allow us to bypass the presubmit as needed for things that do need to reference otherwise disallow base functionality as an implementation detail.

For STL:
I think a blocklist might work better for STL, mostly we just want to forbid the STL container types, right?

Using the clang plugin is an option as well, though someone would need to implement it: we'd also want to make it configurable without requiring a clang roll.

Daniel

Kentaro Hara

unread,
May 23, 2017, 8:29:23 PM5/23/17
to Daniel Cheng, Fernando, platform-architecture-dev, Jeremy Roman, Yuta Kitamura, John Abd-El-Malek
On Wed, May 24, 2017 at 3:58 AM, Daniel Cheng <dch...@chromium.org> wrote:
For base:
Doing the suggested PRESUBMIT with simple checks of headers + grepping for base::<list of allowed types/functions> seems good to me. I think we've namespaced most significant parts of //base at this point, so I wouldn't be /too/ worried about accidental using not-allowed functionality from headers that are transitively included. It would also allow us to bypass the presubmit as needed for things that do need to reference otherwise disallow base functionality as an implementation detail.

For STL:
I think a blocklist might work better for STL, mostly we just want to forbid the STL container types, right?

+1 to this approach. Whitelisting base & blacklisting STL.

Regarding the approval process for new base / STL types, I'm thinking about just using code review with OWNERS (rather than creating some approval process in blink-dev).

What we need here would be a non-stressful guideline that will work in 99% of the cases in practice :-)



Using the clang plugin is an option as well, though someone would need to implement it: we'd also want to make it configurable without requiring a clang roll.

Daniel

On Tue, May 23, 2017 at 11:44 AM Fernando <fs...@chromium.org> wrote:
Would it be reasonable, as a quick first step, to whitelist tests in Blink to allow base:: types?

On Tuesday, May 23, 2017 at 1:36:23 PM UTC-4, Brett Wilson wrote:
On Tue, May 23, 2017 at 10:29 AM, Jeremy Roman <jbr...@chromium.org> wrote:
On Tue, May 23, 2017 at 12:32 PM, Brett Wilson <bre...@chromium.org> wrote:
On Tue, May 23, 2017 at 8:37 AM, Kentaro Hara <har...@chromium.org> wrote:
For safety I'd prefer using a whitelist approach. The whiltelist approach will work for base:: types since base:: types used in Blink are limited.

There are a lot of weird std helper functions that could theoretically be used without a problem and will be difficult to whitelist.

Do you have any example? Historically std:: types were disallowed in Blink, so I hope Blink would not be using a lot of complex helper functions.

I was thinking mostly of algorithms which I thought blink allowed. If you're doing the checking with a presubmit, I was assuming you would be doing a grep for "std::" and that will match all kinds of things that are already in Blink. I'm imagining a weekly post to Blink dev where some random confused person says "I tried to do std::adjacent_find but the presubmit says that's not allowed. Why can't I use that?"

Also: I noticed there are 60 lines with std::vector and 52 with std::string in core (most, but not all, in tests). There are also some in modules. This may make any plans more complicated.

We do use a ton of STL in Blink: most of <algorithm> is fine (and I wish it were used more), std::unique_ptr obviously is, etc. From my perspective the potential problems are:
- std::basic_string (string, string16)
- std::vector, std::map, and other containers

These are things that we'd prefer to use WTF's variants for now, because Blink is sensitive to their performance properties, and the containers have special handling for things like Oilpan.

I think everybody agrees on the std::string and std::vector policies.
 

I could see Blink adopting scoped_refptr instead of WTF::RefPtr now that it seems to have grown support for start-at-one ref counts, but until/unless someone makes an effort there we should definitely use WTF::RefPtr for WTF::(ThreadSafe)?RefCounted types and scoped_refptr for base::RefCounted.

For types without such entanglements like base::TimeDelta/TimeTicks, I'd personally be happy to use the base types. Another similar case is tracked_objects::Location/FROM_HERE. Types like this should also be (and in some cases, already are) allowed in the public/ API, while it continues to exist. A whitelist seems like a safer, more conservative approach to avoid the problem case, which would be tons of conversions back and forth between WTF::String and std::string.

Blink folks should decide what's best for now. I think either way will be fine if the presubmit error links to clear instructions on the policy and how to update the list.

Brett

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

Brett Wilson

unread,
May 23, 2017, 8:53:23 PM5/23/17
to Kentaro Hara, Daniel Cheng, Fernando, platform-architecture-dev, Jeremy Roman, Yuta Kitamura, John Abd-El-Malek
On Tue, May 23, 2017 at 5:28 PM, Kentaro Hara <har...@chromium.org> wrote:
On Wed, May 24, 2017 at 3:58 AM, Daniel Cheng <dch...@chromium.org> wrote:
For base:
Doing the suggested PRESUBMIT with simple checks of headers + grepping for base::<list of allowed types/functions> seems good to me. I think we've namespaced most significant parts of //base at this point, so I wouldn't be /too/ worried about accidental using not-allowed functionality from headers that are transitively included. It would also allow us to bypass the presubmit as needed for things that do need to reference otherwise disallow base functionality as an implementation detail.

For STL:
I think a blocklist might work better for STL, mostly we just want to forbid the STL container types, right?

+1 to this approach. Whitelisting base & blacklisting STL.

Regarding the approval process for new base / STL types, I'm thinking about just using code review with OWNERS (rather than creating some approval process in blink-dev).

What we need here would be a non-stressful guideline that will work in 99% of the cases in practice :-)

SGTM!

Brett

Dimitri Glazkov

unread,
May 23, 2017, 11:34:04 PM5/23/17
to Brett Wilson, Kentaro Hara, Daniel Cheng, Fernando, platform-architecture-dev, Jeremy Roman, Yuta Kitamura, John Abd-El-Malek
SGTM!! #pileon 

Brett

--
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/CABiGVV8ZLLdOZ1pkH8BtMvT9%2BD%2BCvMVP0999AyZBXa9%2BPMDy9g%40mail.gmail.com.

Yuta Kitamura

unread,
May 24, 2017, 5:19:09 AM5/24/17
to Kentaro Hara, platform-architecture-dev, John Abd-El-Malek, Daniel Cheng
As commented in the doc, I don't understand why we need to do this in the first place.

Two of your "this" links are actually a technical problem of how we wrap base::TimeDelta as WTF::TimeDelta. This is a solvable problem (I was never asked about those problems so I had no chance to respond), and I don't feel a problem like this is a showstopper. The third "this" doesn't even seem like a problem.

WTF already exposes several basic base:: types. They cover most of common use cases. I'm pretty happy to expose further if necessary. What else do we need? What's wrong with this approach?

Is allowing //base in Blink-wide strictly better than the current approach? Honestly I can't see any technical benefit in this move. Rather, I think there's a clear risk of creating a chaos in our code base.

Do we really need to do this?

Kentaro Hara

unread,
May 24, 2017, 5:23:48 AM5/24/17
to Yuta Kitamura, platform-architecture-dev, John Abd-El-Malek, Daniel Cheng
Now that Chromium and Blink are the same code base (once the Onion Soup is done, they interact more), so it wouldn't make much sense to use different types (through aliases and wrappers) between Chromium and Blink except for some containers that really need to be specialized for Blink.




--
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/CAFJcur_z6b9BPYqy321CmsUQ1EzwLqpX7-KFs5RomeK8GgEMZw%40mail.gmail.com.

Yuta Kitamura

unread,
May 24, 2017, 5:34:46 AM5/24/17
to Kentaro Hara, platform-architecture-dev, John Abd-El-Malek, Daniel Cheng
On Wed, May 24, 2017 at 6:23 PM, Kentaro Hara <har...@chromium.org> wrote:
Now that Chromium and Blink are the same code base (once the Onion Soup is done, they interact more), so it wouldn't make much sense to use different types (through aliases and wrappers) between Chromium and Blink except for some containers that really need to be specialized for Blink.

There's a clear technical reason we have aliases and wrappers in WTF: to deal with the differences of data types used in Blink and Chromium.

IMO, "being in the same code base" isn't really a technical justification and isn't a sufficient reason to stop doing WTF aliasing/wrapping, because the data type difference still exists in the unified world.

I would like to hear a "technical" benefit of doing this.

Kentaro Hara

unread,
May 24, 2017, 6:05:16 AM5/24/17
to Yuta Kitamura, platform-architecture-dev, John Abd-El-Malek, Daniel Cheng
On Wed, May 24, 2017 at 6:34 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
On Wed, May 24, 2017 at 6:23 PM, Kentaro Hara <har...@chromium.org> wrote:
Now that Chromium and Blink are the same code base (once the Onion Soup is done, they interact more), so it wouldn't make much sense to use different types (through aliases and wrappers) between Chromium and Blink except for some containers that really need to be specialized for Blink.

There's a clear technical reason we have aliases and wrappers in WTF: to deal with the differences of data types used in Blink and Chromium.

- If there is a difference in the data type, it would make sense to create a wrapper. (However, we should reduce # of the wrappers. It should be technically better to not have two types that do similar things.)

- If there is no difference, I don't see any benefit in using the aliases. It should be technically simpler to just use the same type directly.

 
IMO, "being in the same code base" isn't really a technical justification and isn't a sufficient reason to stop doing WTF aliasing/wrapping, because the data type difference still exists in the unified world.

The plan is to reduce the data type differences. For data types that really need to be differentiated for some specific reasons, it makes sense to keep the WTF-specific types (e.g., WTF::Vector, WTF::String). Other data types should be just unified between Chromium and Blink.


 

I would like to hear a "technical" benefit of doing this.

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

Yuta Kitamura

unread,
May 24, 2017, 6:55:12 AM5/24/17
to Kentaro Hara, platform-architecture-dev, John Abd-El-Malek, Daniel Cheng


On Wed, May 24, 2017 at 7:04 PM, Kentaro Hara <har...@chromium.org> wrote:

On Wed, May 24, 2017 at 6:34 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Wed, May 24, 2017 at 6:23 PM, Kentaro Hara <har...@chromium.org> wrote:
Now that Chromium and Blink are the same code base (once the Onion Soup is done, they interact more), so it wouldn't make much sense to use different types (through aliases and wrappers) between Chromium and Blink except for some containers that really need to be specialized for Blink.

There's a clear technical reason we have aliases and wrappers in WTF: to deal with the differences of data types used in Blink and Chromium.

- If there is a difference in the data type, it would make sense to create a wrapper. (However, we should reduce # of the wrappers. It should be technically better to not have two types that do similar things.)

- If there is no difference, I don't see any benefit in using the aliases. It should be technically simpler to just use the same type directly.

Sorry for not being clear. I meant e.g. "WTF::Vector vs. std::vector and WTF::String vs. std::string" by "the differences of data types". They don't imply the difference in wrappers and aliases.

Let me rephrase: there is a technical reason (WTF::Vector vs std::vector) in having aliases and wrappers in WTF. What you said ("we are in the same code base") did not seem technical to me, and I want to hear a technical advantage of allowing //base, so I can compare the two strategies (WTF aliases/wrappers and allowing //base) fairly.
 

 
IMO, "being in the same code base" isn't really a technical justification and isn't a sufficient reason to stop doing WTF aliasing/wrapping, because the data type difference still exists in the unified world.

The plan is to reduce the data type differences. For data types that really need to be differentiated for some specific reasons, it makes sense to keep the WTF-specific types (e.g., WTF::Vector, WTF::String). Other data types should be just unified between Chromium and Blink.

As long as we want to keep WTF::Vector and WTF::String, I regard that as a significant difference, and some base stuff relying on std::vector or std::string can't be used in a sane way without wrappers.

Aliases create completely same types (as in typedefs), and I don't understand why we want to remove them. They do not differ. They are already unified in the C++ sense. There is no technical advantage in removing those, except for small savings coming from removing a slight number of lines of code.

I'm deeply hoping to make this discussion productive. You seem to keep saying "we should do it" without presenting a concrete reason. That doesn't answer any of my question. I really want to learn the technical advantage we'll gain by allowing //base so we can make a rational decision based on technical merits and risks.

Kentaro Hara

unread,
May 24, 2017, 7:50:48 AM5/24/17
to Yuta Kitamura, platform-architecture-dev, John Abd-El-Malek, Daniel Cheng
On Wed, May 24, 2017 at 7:54 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:


On Wed, May 24, 2017 at 7:04 PM, Kentaro Hara <har...@chromium.org> wrote:
On Wed, May 24, 2017 at 6:34 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Wed, May 24, 2017 at 6:23 PM, Kentaro Hara <har...@chromium.org> wrote:
Now that Chromium and Blink are the same code base (once the Onion Soup is done, they interact more), so it wouldn't make much sense to use different types (through aliases and wrappers) between Chromium and Blink except for some containers that really need to be specialized for Blink.

There's a clear technical reason we have aliases and wrappers in WTF: to deal with the differences of data types used in Blink and Chromium.

- If there is a difference in the data type, it would make sense to create a wrapper. (However, we should reduce # of the wrappers. It should be technically better to not have two types that do similar things.)

- If there is no difference, I don't see any benefit in using the aliases. It should be technically simpler to just use the same type directly.

Sorry for not being clear. I meant e.g. "WTF::Vector vs. std::vector and WTF::String vs. std::string" by "the differences of data types". They don't imply the difference in wrappers and aliases.

Let me rephrase: there is a technical reason (WTF::Vector vs std::vector) in having aliases and wrappers in WTF. What you said ("we are in the same code base") did not seem technical to me, and I want to hear a technical advantage of allowing //base, so I can compare the two strategies (WTF aliases/wrappers and allowing //base) fairly.
 

 
IMO, "being in the same code base" isn't really a technical justification and isn't a sufficient reason to stop doing WTF aliasing/wrapping, because the data type difference still exists in the unified world.

The plan is to reduce the data type differences. For data types that really need to be differentiated for some specific reasons, it makes sense to keep the WTF-specific types (e.g., WTF::Vector, WTF::String). Other data types should be just unified between Chromium and Blink.

As long as we want to keep WTF::Vector and WTF::String, I regard that as a significant difference, and some base stuff relying on std::vector or std::string can't be used in a sane way without wrappers.

Aliases create completely same types (as in typedefs), and I don't understand why we want to remove them. They do not differ. They are already unified in the C++ sense. There is no technical advantage in removing those, except for small savings coming from removing a slight number of lines of code.

I'm confused.

Just to clarify, the proposal is:

- Once Onion Soup is done, use WTF::Vector and WTF::String all over the renderer process. The renderer process should not use std::vector or std::string. WebVector and WebString are gone.

- The proposal is to remove data types that are very similar between Chromium and Blink (scoped_ptr and WTF::RefPtr, base::Bind and WTF::bind etc) and aliases (WTF::AutoReset, WTF::Optional, WTF::SpinLock etc) by letting Blink directly use the base things.

Am I on the same page with you?


 

I'm deeply hoping to make this discussion productive. You seem to keep saying "we should do it" without presenting a concrete reason. That doesn't answer any of my question. I really want to learn the technical advantage we'll gain by allowing //base so we can make a rational decision based on technical merits and risks.

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

Daniel Cheng

unread,
May 24, 2017, 2:06:26 PM5/24/17
to Yuta Kitamura, Kentaro Hara, platform-architecture-dev, John Abd-El-Malek
On Wed, May 24, 2017 at 3:55 AM 'Yuta Kitamura' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
On Wed, May 24, 2017 at 7:04 PM, Kentaro Hara <har...@chromium.org> wrote:
On Wed, May 24, 2017 at 6:34 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
On Wed, May 24, 2017 at 6:23 PM, Kentaro Hara <har...@chromium.org> wrote:
Now that Chromium and Blink are the same code base (once the Onion Soup is done, they interact more), so it wouldn't make much sense to use different types (through aliases and wrappers) between Chromium and Blink except for some containers that really need to be specialized for Blink.

There's a clear technical reason we have aliases and wrappers in WTF: to deal with the differences of data types used in Blink and Chromium.

- If there is a difference in the data type, it would make sense to create a wrapper. (However, we should reduce # of the wrappers. It should be technically better to not have two types that do similar things.)

- If there is no difference, I don't see any benefit in using the aliases. It should be technically simpler to just use the same type directly.

Sorry for not being clear. I meant e.g. "WTF::Vector vs. std::vector and WTF::String vs. std::string" by "the differences of data types". They don't imply the difference in wrappers and aliases.

Let me rephrase: there is a technical reason (WTF::Vector vs std::vector) in having aliases and wrappers in WTF. What you said ("we are in the same code base") did not seem technical to me, and I want to hear a technical advantage of allowing //base, so I can compare the two strategies (WTF aliases/wrappers and allowing //base) fairly.

Two examples where using //base would be useful come to mind:

1. The current situation for base::TimeDelta results in more complex code when we have a separate WTF type. Either:
  • code needs to rely on the implementation detail that WTF::TimeDelta is just an alias for base::TimeDelta
  • we need to create yet another Web wrapper class just to pass arguments through the web layer

2. Having separate base and WTF callbacks is another area we could clean up if we used the base types directly.
  • base callback is a value type, while WTF one needs to be kept alive by a std::unique_ptr
  • WTF callbacks need to be converted to a base::Callback when invoking Mojo methods by calling ConvertToBaseCallback().
I suspect this layering complexity is an indirect cause of bugs like https://crbug.com/679915 as well, since it wasn't obvious that we were copying callback objects under the hood.

Daniel
 
 

 
IMO, "being in the same code base" isn't really a technical justification and isn't a sufficient reason to stop doing WTF aliasing/wrapping, because the data type difference still exists in the unified world.

The plan is to reduce the data type differences. For data types that really need to be differentiated for some specific reasons, it makes sense to keep the WTF-specific types (e.g., WTF::Vector, WTF::String). Other data types should be just unified between Chromium and Blink.

As long as we want to keep WTF::Vector and WTF::String, I regard that as a significant difference, and some base stuff relying on std::vector or std::string can't be used in a sane way without wrappers.

Aliases create completely same types (as in typedefs), and I don't understand why we want to remove them. They do not differ. They are already unified in the C++ sense. There is no technical advantage in removing those, except for small savings coming from removing a slight number of lines of code.

I'm deeply hoping to make this discussion productive. You seem to keep saying "we should do it" without presenting a concrete reason. That doesn't answer any of my question. I really want to learn the technical advantage we'll gain by allowing //base so we can make a rational decision based on technical merits and risks.

--
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/CAFJcur_6szVS-cXUNbzAkZYL74M7hXKupYc8Ryz2-O-uG_s3oQ%40mail.gmail.com.

Nico Weber

unread,
May 24, 2017, 2:18:53 PM5/24/17
to Daniel Cheng, Yuta Kitamura, Kentaro Hara, platform-architecture-dev, John Abd-El-Malek
On Wed, May 24, 2017 at 2:06 PM, Daniel Cheng <dch...@chromium.org> wrote:
On Wed, May 24, 2017 at 3:55 AM 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Wed, May 24, 2017 at 7:04 PM, Kentaro Hara <har...@chromium.org> wrote:
On Wed, May 24, 2017 at 6:34 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Wed, May 24, 2017 at 6:23 PM, Kentaro Hara <har...@chromium.org> wrote:
Now that Chromium and Blink are the same code base (once the Onion Soup is done, they interact more), so it wouldn't make much sense to use different types (through aliases and wrappers) between Chromium and Blink except for some containers that really need to be specialized for Blink.

There's a clear technical reason we have aliases and wrappers in WTF: to deal with the differences of data types used in Blink and Chromium.

- If there is a difference in the data type, it would make sense to create a wrapper. (However, we should reduce # of the wrappers. It should be technically better to not have two types that do similar things.)

- If there is no difference, I don't see any benefit in using the aliases. It should be technically simpler to just use the same type directly.

Sorry for not being clear. I meant e.g. "WTF::Vector vs. std::vector and WTF::String vs. std::string" by "the differences of data types". They don't imply the difference in wrappers and aliases.

Let me rephrase: there is a technical reason (WTF::Vector vs std::vector) in having aliases and wrappers in WTF. What you said ("we are in the same code base") did not seem technical to me, and I want to hear a technical advantage of allowing //base, so I can compare the two strategies (WTF aliases/wrappers and allowing //base) fairly.
Two examples where using //base would be useful come to mind:

1. The current situation for base::TimeDelta results in more complex code when we have a separate WTF type. Either:
  • code needs to rely on the implementation detail that WTF::TimeDelta is just an alias for base::TimeDelta
  • we need to create yet another Web wrapper class just to pass arguments through the web layer

2. Having separate base and WTF callbacks is another area we could clean up if we used the base types directly.
  • base callback is a value type, while WTF one needs to be kept alive by a std::unique_ptr
  • WTF callbacks need to be converted to a base::Callback when invoking Mojo methods by calling ConvertToBaseCallback().

3. The wrapper types are confusing for people who work on chrome but not often on blink, see e.g. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Yp6IRGKigOc
 
I suspect this layering complexity is an indirect cause of bugs like https://crbug.com/679915 as well, since it wasn't obvious that we were copying callback objects under the hood.

Daniel
 

 
IMO, "being in the same code base" isn't really a technical justification and isn't a sufficient reason to stop doing WTF aliasing/wrapping, because the data type difference still exists in the unified world.

The plan is to reduce the data type differences. For data types that really need to be differentiated for some specific reasons, it makes sense to keep the WTF-specific types (e.g., WTF::Vector, WTF::String). Other data types should be just unified between Chromium and Blink.

As long as we want to keep WTF::Vector and WTF::String, I regard that as a significant difference, and some base stuff relying on std::vector or std::string can't be used in a sane way without wrappers.

Aliases create completely same types (as in typedefs), and I don't understand why we want to remove them. They do not differ. They are already unified in the C++ sense. There is no technical advantage in removing those, except for small savings coming from removing a slight number of lines of code.

I'm deeply hoping to make this discussion productive. You seem to keep saying "we should do it" without presenting a concrete reason. That doesn't answer any of my question. I really want to learn the technical advantage we'll gain by allowing //base so we can make a rational decision based on technical merits and risks.

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

--
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/CAF3XrKrcbze7D252MPiaogb%2BASgNDdhpmCLSS5y%3DP0HWa9R7_w%40mail.gmail.com.

John Abd-El-Malek

unread,
May 24, 2017, 3:47:31 PM5/24/17
to Nico Weber, Daniel Cheng, Yuta Kitamura, Kentaro Hara, platform-architecture-dev
On Wed, May 24, 2017 at 11:18 AM, Nico Weber <tha...@google.com> wrote:
On Wed, May 24, 2017 at 2:06 PM, Daniel Cheng <dch...@chromium.org> wrote:
On Wed, May 24, 2017 at 3:55 AM 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Wed, May 24, 2017 at 7:04 PM, Kentaro Hara <har...@chromium.org> wrote:
On Wed, May 24, 2017 at 6:34 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Wed, May 24, 2017 at 6:23 PM, Kentaro Hara <har...@chromium.org> wrote:
Now that Chromium and Blink are the same code base (once the Onion Soup is done, they interact more), so it wouldn't make much sense to use different types (through aliases and wrappers) between Chromium and Blink except for some containers that really need to be specialized for Blink.

There's a clear technical reason we have aliases and wrappers in WTF: to deal with the differences of data types used in Blink and Chromium.

- If there is a difference in the data type, it would make sense to create a wrapper. (However, we should reduce # of the wrappers. It should be technically better to not have two types that do similar things.)

- If there is no difference, I don't see any benefit in using the aliases. It should be technically simpler to just use the same type directly.

Sorry for not being clear. I meant e.g. "WTF::Vector vs. std::vector and WTF::String vs. std::string" by "the differences of data types". They don't imply the difference in wrappers and aliases.

Let me rephrase: there is a technical reason (WTF::Vector vs std::vector) in having aliases and wrappers in WTF. What you said ("we are in the same code base") did not seem technical to me, and I want to hear a technical advantage of allowing //base, so I can compare the two strategies (WTF aliases/wrappers and allowing //base) fairly.

(we should probably pick whether we keep the conversation in the doc or in the thread :) )

Having different container/string classes leads to some things from base/ not being shareable (e.g. StringPiece, string helpers etc...). But there are still plenty of stuff there that isn't impacted by string/container differences that can, and already are, reused (e.g. Time). Having thin wrappers for them is busy-work. 


Two examples where using //base would be useful come to mind:

1. The current situation for base::TimeDelta results in more complex code when we have a separate WTF type. Either:
  • code needs to rely on the implementation detail that WTF::TimeDelta is just an alias for base::TimeDelta
  • we need to create yet another Web wrapper class just to pass arguments through the web layer

2. Having separate base and WTF callbacks is another area we could clean up if we used the base types directly.
  • base callback is a value type, while WTF one needs to be kept alive by a std::unique_ptr
  • WTF callbacks need to be converted to a base::Callback when invoking Mojo methods by calling ConvertToBaseCallback().

3. The wrapper types are confusing for people who work on chrome but not often on blink, see e.g. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Yp6IRGKigOc
 
I suspect this layering complexity is an indirect cause of bugs like https://crbug.com/679915 as well, since it wasn't obvious that we were copying callback objects under the hood.

Daniel
 

 
IMO, "being in the same code base" isn't really a technical justification and isn't a sufficient reason to stop doing WTF aliasing/wrapping, because the data type difference still exists in the unified world.

The plan is to reduce the data type differences. For data types that really need to be differentiated for some specific reasons, it makes sense to keep the WTF-specific types (e.g., WTF::Vector, WTF::String). Other data types should be just unified between Chromium and Blink.

As long as we want to keep WTF::Vector and WTF::String, I regard that as a significant difference, and some base stuff relying on std::vector or std::string can't be used in a sane way without wrappers.

Aliases create completely same types (as in typedefs), and I don't understand why we want to remove them. They do not differ. They are already unified in the C++ sense. There is no technical advantage in removing those, except for small savings coming from removing a slight number of lines of code.

I'm deeply hoping to make this discussion productive. You seem to keep saying "we should do it" without presenting a concrete reason. That doesn't answer any of my question. I really want to learn the technical advantage we'll gain by allowing //base so we can make a rational decision based on technical merits and risks.

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

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

Kentaro Hara

unread,
May 25, 2017, 12:15:29 AM5/25/17
to John Abd-El-Malek, Nico Weber, Daniel Cheng, Yuta Kitamura, platform-architecture-dev
Let's keep discussing on this thread (rather than the doc ;-).

Another example I hit today is this CL. The CL is introducing a Platform public API because core/ cannot call base::SysInfo::AmountOfPhysicalMemoryMB().


Yuta Kitamura

unread,
May 25, 2017, 2:40:25 AM5/25/17
to Kentaro Hara, John Abd-El-Malek, Nico Weber, Daniel Cheng, platform-architecture-dev
Re TimeDelta/TimeTicks: Yeah, I agree that how TimeDelta/TimeTicks are wrapped in WTF is problematic in a number of ways. I think we probably should convert it to a plain type alias. I regard this as a bug.

Re Bind(): I also agree that the difference in the semantics is problematic, but allowing both base::Bind() and WTF::bind() can also be confusing. People would probably start to use both; I can easily imagine a chaos caused by this.

Re Writing thin wrappers is busy work: I don't think so. In most cases we should just write a type alias (typedef). It's easy, and I'm happy to add such features in WTF. As long as there are not too many requests, I can keep up with little efforts. I feel like the features already supported in WTF cover many cases, and I don't expect too many addtions.

Re Confusion in folks on Chromium: Hm, well yeah, that's true, but even if we allow //base, Blink and Chromium are still fairly different, and I don't think allowing //base would change the situation drastically. I would categorize this as a lack of education; actually I've been thinking about how to educate non-Blink developers for a while. Maybe I should put more effort on that.

Thanks for your opinions. Let me summarize my position here.

Let's compare the current situation with the proposal side-by-side:

Current situation (WTF gating //base feature usage)

Good: Good control over //base feature usage
Bad?: Need to add alias and/or wrappers as necessary ---> I don't feel like this would require much effort

Allowing //base in Blink

Good: WTF aliases and wrappers are no longer necessary
Bad: Lose some control over //base feature usage:
  • Can't block the usage of //base APIs employing std::string/std::vector etc. ---> Can be mitigated by presubmit?
  • Can't block the usage of overlapping APIs between WTF and //base, e.g. base::Bind() vs WTF::bind(), base::RandBytes() vs WTF::CryptographicallyRandomNumber(). ---> Can we mitigate this case? Do we need to merge those?
In sum, I think WTF gating //base feature usage is a practical technical measure based on the current situation around Chromium and Blink. Allowing //base essentially removes that barrier. To be able to allow //base in Blink, we need to make sure the total gain of //base usage exceeds that of WTF gating. I'm still not convinced that it actually does.

Thank you for your constructive discussions. I'm happy to hear your opinion on those points.

Alex Clarke

unread,
May 25, 2017, 4:44:51 AM5/25/17
to Yuta Kitamura, Kentaro Hara, John Abd-El-Malek, Nico Weber, Daniel Cheng, platform-architecture-dev
On 25 May 2017 at 07:40, 'Yuta Kitamura' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
Re TimeDelta/TimeTicks: Yeah, I agree that how TimeDelta/TimeTicks are wrapped in WTF is problematic in a number of ways. I think we probably should convert it to a plain type alias. I regard this as a bug.

Re Bind(): I also agree that the difference in the semantics is problematic, but allowing both base::Bind() and WTF::bind() can also be confusing. People would probably start to use both; I can easily imagine a chaos caused by this.

Re Writing thin wrappers is busy work: I don't think so. In most cases we should just write a type alias (typedef). It's easy, and I'm happy to add such features in WTF. As long as there are not too many requests, I can keep up with little efforts. I feel like the features already supported in WTF cover many cases, and I don't expect too many addtions.

Re Confusion in folks on Chromium: Hm, well yeah, that's true, but even if we allow //base, Blink and Chromium are still fairly different, and I don't think allowing //base would change the situation drastically. I would categorize this as a lack of education; actually I've been thinking about how to educate non-Blink developers for a while. Maybe I should put more effort on that.

It's just annoying having to remember to use base::ClassName and WTF::ClassName in different parts of the code, and maybe in the same CL.
 
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.

Kentaro Hara

unread,
May 25, 2017, 5:26:21 AM5/25/17
to Yuta Kitamura, John Abd-El-Malek, Nico Weber, Daniel Cheng, platform-architecture-dev
On Thu, May 25, 2017 at 3:40 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
Re TimeDelta/TimeTicks: Yeah, I agree that how TimeDelta/TimeTicks are wrapped in WTF is problematic in a number of ways. I think we probably should convert it to a plain type alias. I regard this as a bug.

Re Bind(): I also agree that the difference in the semantics is problematic, but allowing both base::Bind() and WTF::bind() can also be confusing. People would probably start to use both; I can easily imagine a chaos caused by this.

I'm proposing to unify the two and just use only base::Bind() throughout the code base. If we cannot unify the two for some reason, we need to keep base::Bind() and WTF::bind() but then we should forbid base::Bind() in the renderer. I'm never proposing to mix the two.

 
Re Writing thin wrappers is busy work: I don't think so. In most cases we should just write a type alias (typedef). It's easy, and I'm happy to add such features in WTF. As long as there are not too many requests, I can keep up with little efforts. I feel like the features already supported in WTF cover many cases, and I don't expect too many addtions.

Re Confusion in folks on Chromium: Hm, well yeah, that's true, but even if we allow //base, Blink and Chromium are still fairly different, and I don't think allowing //base would change the situation drastically. I would categorize this as a lack of education; actually I've been thinking about how to educate non-Blink developers for a while. Maybe I should put more effort on that.

In addition to Alex's point, another confusing part is that platform/ can use base whereas modules/ cannot. This rule is confusing for features that are implemented in both platform/ and modules/ (e.g., webaudio, mediastream).

In general we have invested a lot of efforts to get Blink and Chromium closer. We did the repository merge, did the blink big rename, allowed a bunch of direct dependencies, did a lot of Onion Soup work etc. Onion Soup 2.0 is planning to remove all public APIs, where there will be no distinction between Blink and Chromium. Trying to keep the difference seems like going in an opposite direction to me.



Thanks for your opinions. Let me summarize my position here.

Let's compare the current situation with the proposal side-by-side:

Current situation (WTF gating //base feature usage)

Good: Good control over //base feature usage
Bad?: Need to add alias and/or wrappers as necessary ---> I don't feel like this would require much effort

Bad: People need to switch their mind depending on what code they are touching.


Allowing //base in Blink

Good: WTF aliases and wrappers are no longer necessary
Bad: Lose some control over //base feature usage:
  • Can't block the usage of //base APIs employing std::string/std::vector etc. ---> Can be mitigated by presubmit?
  • Can't block the usage of overlapping APIs between WTF and //base, e.g. base::Bind() vs WTF::bind(), base::RandBytes() vs WTF::CryptographicallyRandomNumber(). ---> Can we mitigate this case? Do we need to merge those?
Can we use the presubmit script for this as well? The plan is to whilelist allowed base types in the presubmit script. We can just not add base::Bind and base::RandBytes to the presubmit script.

 
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.

Yuta Kitamura

unread,
May 25, 2017, 7:44:46 AM5/25/17
to Kentaro Hara, John Abd-El-Malek, Nico Weber, Daniel Cheng, platform-architecture-dev


On Thu, May 25, 2017 at 6:25 PM, Kentaro Hara <har...@chromium.org> wrote:

On Thu, May 25, 2017 at 3:40 PM, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
Re TimeDelta/TimeTicks: Yeah, I agree that how TimeDelta/TimeTicks are wrapped in WTF is problematic in a number of ways. I think we probably should convert it to a plain type alias. I regard this as a bug.

Re Bind(): I also agree that the difference in the semantics is problematic, but allowing both base::Bind() and WTF::bind() can also be confusing. People would probably start to use both; I can easily imagine a chaos caused by this.

I'm proposing to unify the two and just use only base::Bind() throughout the code base. If we cannot unify the two for some reason, we need to keep base::Bind() and WTF::bind() but then we should forbid base::Bind() in the renderer. I'm never proposing to mix the two.

Hm yeah OK, but that makes it impossible to use //base features requiring base::Bind/Callback in Blink.
 

 
Re Writing thin wrappers is busy work: I don't think so. In most cases we should just write a type alias (typedef). It's easy, and I'm happy to add such features in WTF. As long as there are not too many requests, I can keep up with little efforts. I feel like the features already supported in WTF cover many cases, and I don't expect too many addtions.

Re Confusion in folks on Chromium: Hm, well yeah, that's true, but even if we allow //base, Blink and Chromium are still fairly different, and I don't think allowing //base would change the situation drastically. I would categorize this as a lack of education; actually I've been thinking about how to educate non-Blink developers for a while. Maybe I should put more effort on that.

In addition to Alex's point, another confusing part is that platform/ can use base whereas modules/ cannot. This rule is confusing for features that are implemented in both platform/ and modules/ (e.g., webaudio, mediastream).

In general we have invested a lot of efforts to get Blink and Chromium closer. We did the repository merge, did the blink big rename, allowed a bunch of direct dependencies, did a lot of Onion Soup work etc. Onion Soup 2.0 is planning to remove all public APIs, where there will be no distinction between Blink and Chromium. Trying to keep the difference seems like going in an opposite direction to me.

What I'm saying is basically: "reducing the difference may be worse than keeping the difference." You said that Vector, String, RefPtr etc. would remain Blink-only. This is a critical difference between Blink and Chromium that will be kept in the future. As long as we have this policy, we somehow need to restrict the use of //base features. The WTF gating system is a fairly good mechanism for this. Your proposal will break this mechanism, without presenting a good techical merit brought by it (from my point of view).




Thanks for your opinions. Let me summarize my position here.

Let's compare the current situation with the proposal side-by-side:

Current situation (WTF gating //base feature usage)

Good: Good control over //base feature usage
Bad?: Need to add alias and/or wrappers as necessary ---> I don't feel like this would require much effort

Bad: People need to switch their mind depending on what code they are touching.

Yeah, but people still need to switch their mind as long as Blink uses Vector, String or RefPtr. Allowing //base does not change the situation very much.
 


Allowing //base in Blink

Good: WTF aliases and wrappers are no longer necessary
Bad: Lose some control over //base feature usage:
  • Can't block the usage of //base APIs employing std::string/std::vector etc. ---> Can be mitigated by presubmit?
  • Can't block the usage of overlapping APIs between WTF and //base, e.g. base::Bind() vs WTF::bind(), base::RandBytes() vs WTF::CryptographicallyRandomNumber(). ---> Can we mitigate this case? Do we need to merge those?
Can we use the presubmit script for this as well? The plan is to whilelist allowed base types in the presubmit script. We can just not add base::Bind and base::RandBytes to the presubmit script.

I want you to compare the objective aspects above and explain why allowing //base is definitely a win. So far you only said "we are in the same code base" or "we want to keep the difference minimum", but I don't think these arguments can justify the loss of the technical merits (good control over //base usage). As I kept saying, I really want to hear an objective, techical analysis rather than subjective arguments you gave, because it's not feasible to evaluate subjective metrics like "people need to switch their mind" -- how bad is that?

Kentaro Hara

unread,
May 25, 2017, 8:44:41 AM5/25/17
to Yuta Kitamura, John Abd-El-Malek, Nico Weber, Daniel Cheng, platform-architecture-dev
Not allowing //base in Blink:

- Chromium needs to use base::AutoReset and std::string
- platform/ needs to use base::AutoReset and WTF::String
- platform/ needs to define WTF::AutoReset as an alias
- core/ and modules/ need to use WTF::AutoReset and WTF::String

Allowing //base in Blink:

- Chromium needs to use base::AutoReset and std::string
- platform/ needs to use base::AutoReset and WTF::String
- core/ and modules/ need to use base::AutoReset and WTF::String

The latter has less types and rules than the former and thus is better. Isn't this a good objective, technical reason to adopt the latter?


(FWIW I'm viewing Blink's development as a kind of art 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+unsubsc...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

dan...@chromium.org

unread,
May 25, 2017, 11:16:04 AM5/25/17
to Yuta Kitamura, Kentaro Hara, John Abd-El-Malek, Nico Weber, Daniel Cheng, platform-architecture-dev
On Thu, May 25, 2017 at 2:40 AM, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
Re TimeDelta/TimeTicks: Yeah, I agree that how TimeDelta/TimeTicks are wrapped in WTF is problematic in a number of ways. I think we probably should convert it to a plain type alias. I regard this as a bug.

Re Bind(): I also agree that the difference in the semantics is problematic, but allowing both base::Bind() and WTF::bind() can also be confusing. People would probably start to use both; I can easily imagine a chaos caused by this.

I think what haraken is proposing is a mechanism by which we /can/ merge types like Bind and String in the future. Without it, we are deciding to forever keep a mirror of base/. Notably the types in WTF that we intend to keep all collide with STL types, and not all with base/.
 
Re Writing thin wrappers is busy work: I don't think so. In most cases we should just write a type alias (typedef). It's easy, and I'm happy to add such features in WTF. As long as there are not too many requests, I can keep up with little efforts. I feel like the features already supported in WTF cover many cases, and I don't expect too many addtions.

It's not entirely typedefs. It's also forwarding functions, templates, and wrappers. This adds to the cognitive load to read and understand the codebase. When you grep for cc::PaintRecorder you think you've found all uses, until you realize there is a typedef in blink. The same goes for base/ things.

It puts people in a position where if they move some code from directory A to B, they also have to build a wrapper around a base type/template, which seems only like wasted and patch complexity to me.

As well, if no base things are allowed, then no APIs with base things in them are allowed, as this rule is transitive. This implies we will be forced to keep Web types to convert between blink types and other (for eg. cc) types that are built from base things.

My feeling is that you're approaching this from the perspective if what would change in the compiled binary as a result, and that not a lot of things would right now. But it gives us a path to making changes in the future to have less translations between similar types and it removes a bunch of cognitive load reading and writing code around chrome which impacts a lot of developers and the decisions they would make in how to structure the codebase.

- Dana
 

Yuta Kitamura

unread,
May 26, 2017, 3:01:03 AM5/26/17
to Kentaro Hara, John Abd-El-Malek, Nico Weber, Daniel Cheng, platform-architecture-dev
On Thu, May 25, 2017 at 9:44 PM, Kentaro Hara <har...@chromium.org> wrote:
Not allowing //base in Blink:

- Chromium needs to use base::AutoReset and std::string
- platform/ needs to use base::AutoReset and WTF::String

Why isn't this case not the same as core/?

Yuta Kitamura

unread,
May 26, 2017, 4:18:02 AM5/26/17
to Dana Jansens, Kentaro Hara, John Abd-El-Malek, Nico Weber, Daniel Cheng, platform-architecture-dev
On Fri, May 26, 2017 at 12:15 AM, <dan...@chromium.org> wrote:
On Thu, May 25, 2017 at 2:40 AM, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
Re TimeDelta/TimeTicks: Yeah, I agree that how TimeDelta/TimeTicks are wrapped in WTF is problematic in a number of ways. I think we probably should convert it to a plain type alias. I regard this as a bug.

Re Bind(): I also agree that the difference in the semantics is problematic, but allowing both base::Bind() and WTF::bind() can also be confusing. People would probably start to use both; I can easily imagine a chaos caused by this.

I think what haraken is proposing is a mechanism by which we /can/ merge types like Bind and String in the future. Without it, we are deciding to forever keep a mirror of base/. Notably the types in WTF that we intend to keep all collide with STL types, and not all with base/.
 
Re Writing thin wrappers is busy work: I don't think so. In most cases we should just write a type alias (typedef). It's easy, and I'm happy to add such features in WTF. As long as there are not too many requests, I can keep up with little efforts. I feel like the features already supported in WTF cover many cases, and I don't expect too many addtions.

It's not entirely typedefs. It's also forwarding functions, templates, and wrappers. This adds to the cognitive load to read and understand the codebase. When you grep for cc::PaintRecorder you think you've found all uses, until you realize there is a typedef in blink. The same goes for base/ things.

It puts people in a position where if they move some code from directory A to B, they also have to build a wrapper around a base type/template, which seems only like wasted and patch complexity to me.

As well, if no base things are allowed, then no APIs with base things in them are allowed, as this rule is transitive. This implies we will be forced to keep Web types to convert between blink types and other (for eg. cc) types that are built from base things.

My feeling is that you're approaching this from the perspective if what would change in the compiled binary as a result, and that not a lot of things would right now. But it gives us a path to making changes in the future to have less translations between similar types and it removes a bunch of cognitive load reading and writing code around chrome which impacts a lot of developers and the decisions they would make in how to structure the codebase.

I understand the cognitive load argument to some extent, but I'm afraid allowing //base could also make the coding more complex in some respects.

Currently, what Blink can use is what WTF provides. That's all. This simple mindset can cover all the cases. (This ignores the complexity of std:: stuff, but that won't change if we allow //base.)

If we allow //base in Blink, the rule would be more complex:
  • You can use all of WTF.
  • You can use //base except for stuff requiring std::string, std::vector, std::map, base::Callback, ... etc.
The second rule is very cumbersome to remember (and actually the blacklist is still incomplete) and has a negative impact on developers' cognitive load, doesn't it? It's hard for me to say this is a simplification.

--

As for wrappers: let me first define some terms.

An alias: a synonym of a base:: name. A typedef/type alias/alias template for a base:: class or class template; a using declaration for a base:: function; or a definition of a forwarding function template in WTF for a base:: function template.

A wrapper: A new name introduced in WTF to handle a base:: concept. For example: a wrapper class that converts WTF::Vector and std::vector on the way.

Aliases are very cheap. It's trivial to write them, and the maintenance is not too much a burden. You all seem to assume this is a lot of burden, but I believe that's factually wrong.

Wrappers can be costly, on the other hand. However, for features that doesn't require forbidden types like std::string or std::vector, aliases would be usually sufficient, and wrappers are not necessary.

This proposal only covers the cases where aliases are sufficient. As I said, aliases are cheap and simple, and removing existing aliases itself is not a big win. Also, it's extremely easy to add new aliases. And also, I don't expect there will be a demand for a lot of new aliases in the future, considering the past record. I expect the demand will stay within a manageable range.

As far as I understand, one of the proposal's biggest advantage is removal of those WTF aliases. I can hardly believe that's significant enough to cancel the potential bad effects of allowing //base in Blink.

--

Regarding potential bad effects of allowing //base in Blink,

I think presubmit is an incomplete measure and cannot prevent all possible misuses of banned features, such as function returns and ADLs.

I'm not insisting to make those checks complete; rather, what I want to say here is: why do you move to a flawed system while we already have a good existing system (WTF aliasing)? Does the gain of allowing //base really surpass the risk of allowing //base? I haven't heard a convincing argument on this point so far.


John Abd-El-Malek

unread,
May 26, 2017, 9:59:14 AM5/26/17
to Yuta Kitamura, Dana Jansens, Kentaro Hara, Nico Weber, Daniel Cheng, platform-architecture-dev
Your summary misses a few things noted earlier:
-parts of base/ are already used in parts of webkit 
-the cost isn't just writing a typedef, it's the cognitive load of having different versions to use in blink

It seems that the majority of people who've given input on this feel that it's preferable to allow whitelisted types. At this point it seems the thread is going in circles, so perhaps interest parties & deciders should meet to reach a decision and update us on this thread?

Dimitri Glazkov

unread,
May 26, 2017, 11:30:56 AM5/26/17
to John Abd-El-Malek, Yuta Kitamura, Dana Jansens, Kentaro Hara, Nico Weber, Daniel Cheng, platform-architecture-dev
On Fri, May 26, 2017 at 6:59 AM John Abd-El-Malek <j...@chromium.org> wrote:
On Fri, May 26, 2017 at 1:17 AM, Yuta Kitamura <yu...@google.com> wrote:
On Fri, May 26, 2017 at 12:15 AM, <dan...@chromium.org> wrote:

+1. If you need more organized tooling around how to drive the decision-making, please use the decision framework I outlined in https://groups.google.com/a/chromium.org/d/msg/blink-dev/KKNbuzj-3HY/VhD-inMbBwAJ.

:DG<
 

--
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/CALhVsw1rKxRNo_j6zViLw-GiZb-hcKxBSHYDrdAViZTDrNk7%3DQ%40mail.gmail.com.

Yuta Kitamura

unread,
Jun 2, 2017, 4:39:31 AM6/2/17
to John Abd-El-Malek, Dana Jansens, Kentaro Hara, Nico Weber, Daniel Cheng, platform-architecture-dev
On Fri, May 26, 2017 at 10:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:
Your summary misses a few things noted earlier:
-parts of base/ are already used in parts of webkit 

I know. But I actually feel like allowing //base in platform/ (except for wtf) could be a mistake. My impression is that the code has become messy and harder to read and write, just because there are more ways to write the same thing. platform/scheduler is the most unfortunate example and I wish we don't repeat it.
 
-the cost isn't just writing a typedef, it's the cognitive load of having different versions to use in blink

As I said earlier, the cognitive load can be worse because there are more rules in what we can use in Blink (you need to learn when to use WTF stuff and when to use base stuff, and that is not really obvious). I can expect many mistakes will be made because of this. With that said, will the cognitive load for Blink devs definitely decrease? Can you logically explain?

--

I really want people to consider every aspect of this change and compare the pros and the cons fairly. I'm very eager to hear why allowing //base is a win for us in total, instead of discussions over individual aspects. (I'm not saying discussing individual topics is bad; I honor them. However, we need to sum them up in the end to make a rational decision.)

Alex Clarke

unread,
Jun 2, 2017, 5:06:16 AM6/2/17
to Yuta Kitamura, John Abd-El-Malek, Dana Jansens, Kentaro Hara, Nico Weber, Daniel Cheng, platform-architecture-dev
On 2 June 2017 at 09:39, 'Yuta Kitamura' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
On Fri, May 26, 2017 at 10:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:
Your summary misses a few things noted earlier:
-parts of base/ are already used in parts of webkit 

I know. But I actually feel like allowing //base in platform/ (except for wtf) could be a mistake. My impression is that the code has become messy and harder to read and write, just because there are more ways to write the same thing. platform/scheduler is the most unfortunate example and I wish we don't repeat it.

What exactly is wrong with platform/scheduler?
 

 
 
-the cost isn't just writing a typedef, it's the cognitive load of having different versions to use in blink

As I said earlier, the cognitive load can be worse because there are more rules in what we can use in Blink (you need to learn when to use WTF stuff and when to use base stuff, and that is not really obvious). I can expect many mistakes will be made because of this. With that said, will the cognitive load for Blink devs definitely decrease? Can you logically explain?

--

I really want people to consider every aspect of this change and compare the pros and the cons fairly. I'm very eager to hear why allowing //base is a win for us in total, instead of discussions over individual aspects. (I'm not saying discussing individual topics is bad; I honor them. However, we need to sum them up in the end to make a rational decision.)

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

Kentaro Hara

unread,
Jun 2, 2017, 11:01:57 AM6/2/17
to Alex Clarke, Yuta Kitamura, John Abd-El-Malek, Dana Jansens, Nico Weber, Daniel Cheng, platform-architecture-dev
Let me set a meeting next week.



On Fri, Jun 2, 2017 at 6:06 PM, 'Alex Clarke' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
On 2 June 2017 at 09:39, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Fri, May 26, 2017 at 10:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:
Your summary misses a few things noted earlier:
-parts of base/ are already used in parts of webkit 

I know. But I actually feel like allowing //base in platform/ (except for wtf) could be a mistake. My impression is that the code has become messy and harder to read and write, just because there are more ways to write the same thing. platform/scheduler is the most unfortunate example and I wish we don't repeat it.

What exactly is wrong with platform/scheduler?
 

 
 
-the cost isn't just writing a typedef, it's the cognitive load of having different versions to use in blink

As I said earlier, the cognitive load can be worse because there are more rules in what we can use in Blink (you need to learn when to use WTF stuff and when to use base stuff, and that is not really obvious). I can expect many mistakes will be made because of this. With that said, will the cognitive load for Blink devs definitely decrease? Can you logically explain?

--

I really want people to consider every aspect of this change and compare the pros and the cons fairly. I'm very eager to hear why allowing //base is a win for us in total, instead of discussions over individual aspects. (I'm not saying discussing individual topics is bad; I honor them. However, we need to sum them up in the end to make a rational decision.)

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

Kentaro Hara

unread,
Jul 5, 2017, 5:06:17 AM7/5/17
to Alex Clarke, Yuta Kitamura, John Abd-El-Malek, Dana Jansens, Nico Weber, Daniel Cheng, platform-architecture-dev
Let me wrap up this thread.

A couple of people on this thread met together and discussed. To move the needle forward haraken@ made the following decision:

- //base types and STL types are allowed in Blink including core/ and modules/ (except a couple of containers that really need to be optimized for Blink; i.e., String, AtomicString, Vector, HashSet, HashMap etc). You don't need to define thin wrappers in platform/wtf/.

- The rule is checked by the presubmit script (even though the check won't be 100% perfect).

Is there anyone who is interested in updating the rule in the code base, improving the presubmit check, removing the existing thin wrappers etc? :)




On Sat, Jun 3, 2017 at 12:01 AM, Kentaro Hara <har...@chromium.org> wrote:
Let me set a meeting next week.


On Fri, Jun 2, 2017 at 6:06 PM, 'Alex Clarke' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On 2 June 2017 at 09:39, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Fri, May 26, 2017 at 10:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:
Your summary misses a few things noted earlier:
-parts of base/ are already used in parts of webkit 

I know. But I actually feel like allowing //base in platform/ (except for wtf) could be a mistake. My impression is that the code has become messy and harder to read and write, just because there are more ways to write the same thing. platform/scheduler is the most unfortunate example and I wish we don't repeat it.

What exactly is wrong with platform/scheduler?
 

 
 
-the cost isn't just writing a typedef, it's the cognitive load of having different versions to use in blink

As I said earlier, the cognitive load can be worse because there are more rules in what we can use in Blink (you need to learn when to use WTF stuff and when to use base stuff, and that is not really obvious). I can expect many mistakes will be made because of this. With that said, will the cognitive load for Blink devs definitely decrease? Can you logically explain?

--

I really want people to consider every aspect of this change and compare the pros and the cons fairly. I'm very eager to hear why allowing //base is a win for us in total, instead of discussions over individual aspects. (I'm not saying discussing individual topics is bad; I honor them. However, we need to sum them up in the end to make a rational decision.)

--
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.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAFJcur8d4G5JQeWzouUpiF9o1xZn42uBRxgaw4PFFiYtDi9CSw%40mail.gmail.com.

--
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.
--
Kentaro Hara, Tokyo, Japan

Daniel Cheng

unread,
Jul 26, 2017, 7:16:46 PM7/26/17
to Kentaro Hara, Alex Clarke, Yuta Kitamura, John Abd-El-Malek, Dana Jansens, Nico Weber, platform-architecture-dev
Btw, I wonder if we should be checking mojom files as well? Due to typemapping, they can bring in dependencies on stuff we might not want to use in Blink (such as base::Value).

Daniel

On Wed, Jul 5, 2017 at 2:06 AM Kentaro Hara <har...@chromium.org> wrote:
Let me wrap up this thread.

A couple of people on this thread met together and discussed. To move the needle forward haraken@ made the following decision:

- //base types and STL types are allowed in Blink including core/ and modules/ (except a couple of containers that really need to be optimized for Blink; i.e., String, AtomicString, Vector, HashSet, HashMap etc). You don't need to define thin wrappers in platform/wtf/.

- The rule is checked by the presubmit script (even though the check won't be 100% perfect).

Is there anyone who is interested in updating the rule in the code base, improving the presubmit check, removing the existing thin wrappers etc? :)



On Sat, Jun 3, 2017 at 12:01 AM, Kentaro Hara <har...@chromium.org> wrote:
Let me set a meeting next week.



On Fri, Jun 2, 2017 at 6:06 PM, 'Alex Clarke' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
On 2 June 2017 at 09:39, 'Yuta Kitamura' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
On Fri, May 26, 2017 at 10:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:
Your summary misses a few things noted earlier:
-parts of base/ are already used in parts of webkit 

I know. But I actually feel like allowing //base in platform/ (except for wtf) could be a mistake. My impression is that the code has become messy and harder to read and write, just because there are more ways to write the same thing. platform/scheduler is the most unfortunate example and I wish we don't repeat it.

What exactly is wrong with platform/scheduler?
 

 
 
-the cost isn't just writing a typedef, it's the cognitive load of having different versions to use in blink

As I said earlier, the cognitive load can be worse because there are more rules in what we can use in Blink (you need to learn when to use WTF stuff and when to use base stuff, and that is not really obvious). I can expect many mistakes will be made because of this. With that said, will the cognitive load for Blink devs definitely decrease? Can you logically explain?

--

I really want people to consider every aspect of this change and compare the pros and the cons fairly. I'm very eager to hear why allowing //base is a win for us in total, instead of discussions over individual aspects. (I'm not saying discussing individual topics is bad; I honor them. However, we need to sum them up in the end to make a rational decision.)

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

--
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, Tokyo, Japan

Ian Clelland

unread,
Jul 27, 2017, 10:04:04 AM7/27/17
to Daniel Cheng, Kentaro Hara, Alex Clarke, Yuta Kitamura, John Abd-El-Malek, Dana Jansens, Nico Weber, platform-architecture-dev, Paul Meyer
On Wed, Jul 26, 2017 at 7:16 PM, Daniel Cheng <dch...@chromium.org> wrote:
Btw, I wonder if we should be checking mojom files as well? Due to typemapping, they can bring in dependencies on stuff we might not want to use in Blink (such as base::Value).

Daniel

I just saw an instance of that yesterday -- should the solution there be to have Mojo convert mojo.common.mojom.Value to something like blink::JSONValue in the generated Mojo<->Blink bindings instead?


+paulmeyer, who actually came across that case, and raised the question to me of exactly what is and isn't allowed in core/


On Wed, Jul 5, 2017 at 2:06 AM Kentaro Hara <har...@chromium.org> wrote:
Let me wrap up this thread.

A couple of people on this thread met together and discussed. To move the needle forward haraken@ made the following decision:

- //base types and STL types are allowed in Blink including core/ and modules/ (except a couple of containers that really need to be optimized for Blink; i.e., String, AtomicString, Vector, HashSet, HashMap etc). You don't need to define thin wrappers in platform/wtf/.

- The rule is checked by the presubmit script (even though the check won't be 100% perfect).

Is there anyone who is interested in updating the rule in the code base, improving the presubmit check, removing the existing thin wrappers etc? :)



On Sat, Jun 3, 2017 at 12:01 AM, Kentaro Hara <har...@chromium.org> wrote:
Let me set a meeting next week.



On Fri, Jun 2, 2017 at 6:06 PM, 'Alex Clarke' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On 2 June 2017 at 09:39, 'Yuta Kitamura' via platform-architecture-dev <platform-architecture-dev@chromium.org> wrote:
On Fri, May 26, 2017 at 10:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:
Your summary misses a few things noted earlier:
-parts of base/ are already used in parts of webkit 

I know. But I actually feel like allowing //base in platform/ (except for wtf) could be a mistake. My impression is that the code has become messy and harder to read and write, just because there are more ways to write the same thing. platform/scheduler is the most unfortunate example and I wish we don't repeat it.

What exactly is wrong with platform/scheduler?
 

 
 
-the cost isn't just writing a typedef, it's the cognitive load of having different versions to use in blink

As I said earlier, the cognitive load can be worse because there are more rules in what we can use in Blink (you need to learn when to use WTF stuff and when to use base stuff, and that is not really obvious). I can expect many mistakes will be made because of this. With that said, will the cognitive load for Blink devs definitely decrease? Can you logically explain?

--

I really want people to consider every aspect of this change and compare the pros and the cons fairly. I'm very eager to hear why allowing //base is a win for us in total, instead of discussions over individual aspects. (I'm not saying discussing individual topics is bad; I honor them. However, we need to sum them up in the end to make a rational decision.)

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

--
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.
--
Kentaro Hara, Tokyo, Japan



--
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.
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/CAF3XrKrLp7y7fp4b2qYgF36m2rDjCMsGxO_hBhLeeuv_SC1PVA%40mail.gmail.com.

Reply all
Reply to author
Forward
0 new messages