Clarifying guidelines for using STL in Blink

[Daniel Cheng]

We've had several questions come up lately about when it's OK to use non-Chrome code from Blink code (since it would introduce more usage of STL types in Blink).

Two recent examples:

1. Using net::CanonicalCookie: it would be nice to directly use this in Blink to support the async cookies API implementation. However, since net::CanonicalCookie uses GURL and std::string, we opted to write a wrapper instead. Unfortunately, this also means that we need to duplicate a bunch of fields into the Blink wrapper—which is the opposite of what we've been trying to do elsewhere.
   
2. Plumbing field trial values: sclittle@ had to add a lot of plumbing to pass field trial values into Blink for one feature: https://chromium-review.googlesource.com/c/chromium/src/+/1069338. Using //base directly would remove the need for this plumbing.
   

My reading of our current Blink C++ policy is that we should allow both net::CanonicalCookie and field trials to be used from Blink ("STL string and container types should only be used at the boundary to interoperate with ‘//base’, //third_party/blink/common, and other Chromium-side code"). Does that sound right?

Assuming that's correct, I'm curious what people think about some specific implementation questions below:

For net::CanonicalCookie, it would be nice to avoid writing a Blink wrapper. However, without a wrapper, we need to convert std::string to WTF::String each time an attribute of net::CanonicalCookie is accessed via IDL. That seems inefficient—but we already have IDL attributes that return WTF::String by value already. Converting each time doesn't seem that bad (though it isn't great either), so is it worth adding helpers to WTF::String to convert from std::string (and maybe base::string16)?

For field trials, using the //base code directly is simple enough. However, the previously linked CL also needed to parse the field trial values to turn them into ints. It seems better to just use //base/strings for this, since we already have std::string, rather than first converting to WTF::String to use the WTF routines for parsing. To me, this still falls under the boundary exception for using STL strings/containers.

Does this also sound right?

Daniel

[Kentaro Hara]

On Fri, Jun 1, 2018 at 7:36 AM Daniel Cheng <dch...@chromium.org> wrote:

We've had several questions come up lately about when it's OK to use non-Chrome code from Blink code (since it would introduce more usage of STL types in Blink).

Two recent examples:

1. Using net::CanonicalCookie: it would be nice to directly use this in Blink to support the async cookies API implementation. However, since net::CanonicalCookie uses GURL and std::string, we opted to write a wrapper instead. Unfortunately, this also means that we need to duplicate a bunch of fields into the Blink wrapper—which is the opposite of what we've been trying to do elsewhere.
   
2. Plumbing field trial values: sclittle@ had to add a lot of plumbing to pass field trial values into Blink for one feature: https://chromium-review.googlesource.com/c/chromium/src/+/1069338. Using //base directly would remove the need for this plumbing.
   

My reading of our current Blink C++ policy is that we should allow both net::CanonicalCookie and field trials to be used from Blink ("STL string and container types should only be used at the boundary to interoperate with ‘//base’, //third_party/blink/common, and other Chromium-side code"). Does that sound right?

Sounds right to me. As long as we use different primitives between Chromium and Blink, we have to do the conversion somewhere. It sounds totally reasonable to allow the boundary to use both primitives and do the conversion.

Also, in the end state, I hope that the list of the different primitives becomes small: String, AtomicString, KURL, Vector, HashSet/Map and only a couple of data structures.

Assuming that's correct, I'm curious what people think about some specific implementation questions below:

For net::CanonicalCookie, it would be nice to avoid writing a Blink wrapper. However, without a wrapper, we need to convert std::string to WTF::String each time an attribute of net::CanonicalCookie is accessed via IDL. That seems inefficient—but we already have IDL attributes that return WTF::String by value already. Converting each time doesn't seem that bad (though it isn't great either), so is it worth adding helpers to WTF::String to convert from std::string (and maybe base::string16)?

+1 to adding a helper.

We already have StringUTF8Adaptor to convert WTF::String to base::StringPiece without mallocing a separate buffer. We could introduce a similar helper for the opposite conversion.

 

For field trials, using the //base code directly is simple enough. However, the previously linked CL also needed to parse the field trial values to turn them into ints. It seems better to just use //base/strings for this, since we already have std::string, rather than first converting to WTF::String to use the WTF routines for parsing. To me, this still falls under the boundary exception for using STL strings/containers.

+1.

 

Does this also sound right?

Daniel

--

Kentaro Hara, Tokyo, Japan

[Ryan Sleevi]

On Thu, May 31, 2018 at 10:08 PM, Kentaro Hara <har...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 7:36 AM Daniel Cheng <dch...@chromium.org> wrote:

We've had several questions come up lately about when it's OK to use non-Chrome code from Blink code (since it would introduce more usage of STL types in Blink).

Two recent examples:

1. Using net::CanonicalCookie: it would be nice to directly use this in Blink to support the async cookies API implementation. However, since net::CanonicalCookie uses GURL and std::string, we opted to write a wrapper instead. Unfortunately, this also means that we need to duplicate a bunch of fields into the Blink wrapper—which is the opposite of what we've been trying to do elsewhere.
   
2. Plumbing field trial values: sclittle@ had to add a lot of plumbing to pass field trial values into Blink for one feature: https://chromium-review.googlesource.com/c/chromium/src/+/1069338. Using //base directly would remove the need for this plumbing.
   

My reading of our current Blink C++ policy is that we should allow both net::CanonicalCookie and field trials to be used from Blink ("STL string and container types should only be used at the boundary to interoperate with ‘//base’, //third_party/blink/common, and other Chromium-side code"). Does that sound right?

Sounds right to me. As long as we use different primitives between Chromium and Blink, we have to do the conversion somewhere. It sounds totally reasonable to allow the boundary to use both primitives and do the conversion.

Also, in the end state, I hope that the list of the different primitives becomes small: String, AtomicString, KURL, Vector, HashSet/Map and only a couple of data structures.

Assuming that's correct, I'm curious what people think about some specific implementation questions below:

For net::CanonicalCookie, it would be nice to avoid writing a Blink wrapper. However, without a wrapper, we need to convert std::string to WTF::String each time an attribute of net::CanonicalCookie is accessed via IDL. That seems inefficient—but we already have IDL attributes that return WTF::String by value already. Converting each time doesn't seem that bad (though it isn't great either), so is it worth adding helpers to WTF::String to convert from std::string (and maybe base::string16)?

+1 to adding a helper.

We already have StringUTF8Adaptor to convert WTF::String to base::StringPiece without mallocing a separate buffer. We could introduce a similar helper for the opposite conversion.

Sorry to chime in from a peanut gallery, and probably a dumb question, but wouldn't you still need a wrapper for GURL <-> KURL? And doesn't the Blink side do additional URL validation if IDL-exposed?

Just wondering if the std::string <-> WTF::String is the only thing taht needs a wrapper?

 

 

For field trials, using the //base code directly is simple enough. However, the previously linked CL also needed to parse the field trial values to turn them into ints. It seems better to just use //base/strings for this, since we already have std::string, rather than first converting to WTF::String to use the WTF routines for parsing. To me, this still falls under the boundary exception for using STL strings/containers.

+1.

 

Does this also sound right?

Daniel

--

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/CABg10jyEwz98%3DCA0LWN4v11r-Z-DY94bfD4ns-G5OHPm23%2BnPg%40mail.gmail.com.

[Kentaro Hara]

On Fri, Jun 1, 2018 at 3:46 PM Ryan Sleevi <rsl...@chromium.org> wrote:

On Thu, May 31, 2018 at 10:08 PM, Kentaro Hara <har...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 7:36 AM Daniel Cheng <dch...@chromium.org> wrote:

We've had several questions come up lately about when it's OK to use non-Chrome code from Blink code (since it would introduce more usage of STL types in Blink).

Two recent examples:

1. Using net::CanonicalCookie: it would be nice to directly use this in Blink to support the async cookies API implementation. However, since net::CanonicalCookie uses GURL and std::string, we opted to write a wrapper instead. Unfortunately, this also means that we need to duplicate a bunch of fields into the Blink wrapper—which is the opposite of what we've been trying to do elsewhere.
   
2. Plumbing field trial values: sclittle@ had to add a lot of plumbing to pass field trial values into Blink for one feature: https://chromium-review.googlesource.com/c/chromium/src/+/1069338. Using //base directly would remove the need for this plumbing.
   

My reading of our current Blink C++ policy is that we should allow both net::CanonicalCookie and field trials to be used from Blink ("STL string and container types should only be used at the boundary to interoperate with ‘//base’, //third_party/blink/common, and other Chromium-side code"). Does that sound right?

Sounds right to me. As long as we use different primitives between Chromium and Blink, we have to do the conversion somewhere. It sounds totally reasonable to allow the boundary to use both primitives and do the conversion.

Also, in the end state, I hope that the list of the different primitives becomes small: String, AtomicString, KURL, Vector, HashSet/Map and only a couple of data structures.

Assuming that's correct, I'm curious what people think about some specific implementation questions below:

For net::CanonicalCookie, it would be nice to avoid writing a Blink wrapper. However, without a wrapper, we need to convert std::string to WTF::String each time an attribute of net::CanonicalCookie is accessed via IDL. That seems inefficient—but we already have IDL attributes that return WTF::String by value already. Converting each time doesn't seem that bad (though it isn't great either), so is it worth adding helpers to WTF::String to convert from std::string (and maybe base::string16)?

+1 to adding a helper.

We already have StringUTF8Adaptor to convert WTF::String to base::StringPiece without mallocing a separate buffer. We could introduce a similar helper for the opposite conversion.

Sorry to chime in from a peanut gallery, and probably a dumb question, but wouldn't you still need a wrapper for GURL <-> KURL? And doesn't the Blink side do additional URL validation if IDL-exposed?

Just wondering if the std::string <-> WTF::String is the only thing taht needs a wrapper? 

Yes, we need a helper for all type conversions. Regarding KURL => GURL, we have operator GURL() in KURL.h.

+1 to making the helper functions more consistent...

 

For field trials, using the //base code directly is simple enough. However, the previously linked CL also needed to parse the field trial values to turn them into ints. It seems better to just use //base/strings for this, since we already have std::string, rather than first converting to WTF::String to use the WTF routines for parsing. To me, this still falls under the boundary exception for using STL strings/containers.

+1.

 

Does this also sound right?

Daniel

--

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.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jyEwz98%3DCA0LWN4v11r-Z-DY94bfD4ns-G5OHPm23%2BnPg%40mail.gmail.com.

[Ryan Sleevi]

On Fri, Jun 1, 2018 at 2:50 AM, Kentaro Hara <har...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 3:46 PM Ryan Sleevi <rsl...@chromium.org> wrote:

On Thu, May 31, 2018 at 10:08 PM, Kentaro Hara <har...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 7:36 AM Daniel Cheng <dch...@chromium.org> wrote:

We've had several questions come up lately about when it's OK to use non-Chrome code from Blink code (since it would introduce more usage of STL types in Blink).

Two recent examples:

1. Using net::CanonicalCookie: it would be nice to directly use this in Blink to support the async cookies API implementation. However, since net::CanonicalCookie uses GURL and std::string, we opted to write a wrapper instead. Unfortunately, this also means that we need to duplicate a bunch of fields into the Blink wrapper—which is the opposite of what we've been trying to do elsewhere.
   
2. Plumbing field trial values: sclittle@ had to add a lot of plumbing to pass field trial values into Blink for one feature: https://chromium-review.googlesource.com/c/chromium/src/+/1069338. Using //base directly would remove the need for this plumbing.
   

My reading of our current Blink C++ policy is that we should allow both net::CanonicalCookie and field trials to be used from Blink ("STL string and container types should only be used at the boundary to interoperate with ‘//base’, //third_party/blink/common, and other Chromium-side code"). Does that sound right?

Sounds right to me. As long as we use different primitives between Chromium and Blink, we have to do the conversion somewhere. It sounds totally reasonable to allow the boundary to use both primitives and do the conversion.

Also, in the end state, I hope that the list of the different primitives becomes small: String, AtomicString, KURL, Vector, HashSet/Map and only a couple of data structures.

Assuming that's correct, I'm curious what people think about some specific implementation questions below:

For net::CanonicalCookie, it would be nice to avoid writing a Blink wrapper. However, without a wrapper, we need to convert std::string to WTF::String each time an attribute of net::CanonicalCookie is accessed via IDL. That seems inefficient—but we already have IDL attributes that return WTF::String by value already. Converting each time doesn't seem that bad (though it isn't great either), so is it worth adding helpers to WTF::String to convert from std::string (and maybe base::string16)?

+1 to adding a helper.

We already have StringUTF8Adaptor to convert WTF::String to base::StringPiece without mallocing a separate buffer. We could introduce a similar helper for the opposite conversion.

Sorry to chime in from a peanut gallery, and probably a dumb question, but wouldn't you still need a wrapper for GURL <-> KURL? And doesn't the Blink side do additional URL validation if IDL-exposed?

Just wondering if the std::string <-> WTF::String is the only thing taht needs a wrapper? 

Yes, we need a helper for all type conversions. Regarding KURL => GURL, we have operator GURL() in KURL.h.

+1 to making the helper functions more consistent...

Right, I was more thinking of the GURL => KURL case, since net::CanonicalCookie stores it as GURL, but I may have misunderstood the context. If it was only one way (Blink -> net::CanonicalCookie, WTF::String -> base::StringPiece, KURL -> GURL), then it seems like we wouldn't need a wrapper. However, since it seems like the concern was std::string -> WTF::String, we'd need a GURL -> KURL converter, and that's what wasn't clear to me.

[Kentaro Hara]

I'm suggesting implementing a converter for both directions.

[Ryan Sleevi]

Oh! I misunderstood - so sorry for the noise. I had read your remark about primitives as being ones that shouldn't convert, but now re-reading it, I get that you were saying those were things we should have bidirectional conversions for, which would reduce that overhead :)

[Kinuko Yasuda]

I've been seeing multiple patches that were trying to work around STL and base:: constraints with undesirable wrappers or layering (one of the patches had Daniel and I start this thread), so +1 to the direction and this discussion.

By the way regarding conversions we also have WebString and WebURL which provide almost all the necessary conversion code (including to and from base::string16), wondering we can maybe reuse or share the same code.

[Dmitry Gozman]

Although these particular cases are very unfortunate examples, I think we need a simple-to-follow rule here.

With Onion Soup progressing, one can easily imagine calling from, say, HTMLMediaElement to some mojo interface operating STL types. In this case, defining "boundary to interoperate with the rest of the world" is tricky.

To make it worse, I can now justify storing the data I got over mojo in std::string, since I'll be sending it back anyway, so why double-convert? And once I store it in std::string, it gets all over the place.

Basically, I'd like us to come up with an easy to describe principle which reviewers can refer to and not leave the decision to best judgment in each case, since that will lead to somewhat scattered usage of both worlds.

One example would be "no STL in core/", which is somewhat arbitrary but at least prevents leaking STL to html, dom, styles and layout.

Another example would be "no STL members", which means we are free to use external code operating STL, but always convert to WTF when saving data for future use.

There should definitely be a better rule though :)

Thanks,

Dmitry

[Kentaro Hara]

+1 to creating a clear guideline.

I personally feel that the following guideline is fine though:

- STL is allowed only at the boundary to interoperate with the rest of the world (Is this vague?)

- Converters should be used to convert STL and Blink types (String, Vector, HashSet/Map, KURL etc).

I'm open for ideas :)

[Jeremy Roman]

Aside: I've wondered for awhile whether we should kill WTF::CString and just use std::string for the handful of things it's used for. We almost never actually keep it around (almost all uses use it to get a const char* or a std::string), so having it be a special ref-counted thing doesn't seem that useful. We should still prefer WTF::String, of course, in nearly all other cases, but for cases where you really mean "I want an encoded C-style byte-string in $FORMAT" (i.e. what WTF::CString is meant for), it seems natural enough.

[Dmitry Gozman]

On Fri, Jun 1, 2018 at 8:04 AM, Kentaro Hara <har...@chromium.org> wrote:

+1 to creating a clear guideline.

I personally feel that the following guideline is fine though:

- STL is allowed only at the boundary to interoperate with the rest of the world (Is this vague?)

Just to clarify: can I store net::CanonicalCookie in Document::cookie_? I got it directly from network service (which is at the boundary), and will use to return document.cookie string when asked.

[Kentaro Hara]

On Sat, Jun 2, 2018 at 12:22 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:04 AM, Kentaro Hara <har...@chromium.org> wrote:

+1 to creating a clear guideline.

I personally feel that the following guideline is fine though:

- STL is allowed only at the boundary to interoperate with the rest of the world (Is this vague?)

Just to clarify: can I store net::CanonicalCookie in Document::cookie_? I got it directly from network service (which is at the boundary), and will use to return document.cookie string when asked.

Are you asking if you can store GURL in Document::cookie_url_?

The answer would be no. It should use KURL. (It won't make sense to store GURL in this case because document.cookie must return String, not std::string.)

Maybe I can rephrase it as follows:

- STL types passed in by an embedder must be *immediately* converted to Blink types using converters (except very limited cases where the immediate conversion significantly affects performance).

[Dmitry Gozman]

On Fri, Jun 1, 2018 at 8:37 AM, Kentaro Hara <har...@chromium.org> wrote:

On Sat, Jun 2, 2018 at 12:22 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:04 AM, Kentaro Hara <har...@chromium.org> wrote:

+1 to creating a clear guideline.

I personally feel that the following guideline is fine though:

- STL is allowed only at the boundary to interoperate with the rest of the world (Is this vague?)

Just to clarify: can I store net::CanonicalCookie in Document::cookie_? I got it directly from network service (which is at the boundary), and will use to return document.cookie string when asked.

Are you asking if you can store GURL in Document::cookie_url_?

The answer would be no. It should use KURL. (It won't make sense to store GURL in this case because document.cookie must return String, not std::string.)

Maybe I can rephrase it as follows:

- STL types passed in by an embedder must be *immediately* converted to Blink types using converters (except very limited cases where the immediate conversion significantly affects performance).

This sounds reasonable and resolves the field trials usecase.

However, it means we should keep the WebCanonicalCookie class, since we should convert from "embedder" net::CanonicalCookie type immediately to some Blink type. Which brings us back to original question from Daniel.

Thanks,

Dmitry

[Kentaro Hara]

On Sat, Jun 2, 2018 at 3:24 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:37 AM, Kentaro Hara <har...@chromium.org> wrote:

On Sat, Jun 2, 2018 at 12:22 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:04 AM, Kentaro Hara <har...@chromium.org> wrote:

+1 to creating a clear guideline.

I personally feel that the following guideline is fine though:

- STL is allowed only at the boundary to interoperate with the rest of the world (Is this vague?)

Just to clarify: can I store net::CanonicalCookie in Document::cookie_? I got it directly from network service (which is at the boundary), and will use to return document.cookie string when asked.

Are you asking if you can store GURL in Document::cookie_url_?

The answer would be no. It should use KURL. (It won't make sense to store GURL in this case because document.cookie must return String, not std::string.)

Maybe I can rephrase it as follows:

- STL types passed in by an embedder must be *immediately* converted to Blink types using converters (except very limited cases where the immediate conversion significantly affects performance).

This sounds reasonable and resolves the field trials usecase.

However, it means we should keep the WebCanonicalCookie class, since we should convert from "embedder" net::CanonicalCookie type immediately to some Blink type. Which brings us back to original question from Daniel.

Yeah, WebCanonicalCookie looks tricky. I agree that WebCanonicalCookie is necessary, although it won't need to be "Web".

- Blink can receive net::CanonicalCookie (i.e., the embedder doesn't need to covert it to WebCanonicalCookie before passing it to Blink).

- Blink defines blink::CanonicalCookie in platform/network/. blink::CanonicalCookie holds Strings and KURLs.

- When Blink receives net::CanonicalCookie, Blink immediately converts it to blink::CanonicalCookie using converters for String <=> std::string, KURL <=> GURL etc.

Thoughts?

[Kinuko Yasuda]

On Mon, Jun 4, 2018 at 10:48 AM Kentaro Hara <har...@chromium.org> wrote:

On Sat, Jun 2, 2018 at 3:24 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:37 AM, Kentaro Hara <har...@chromium.org> wrote:

On Sat, Jun 2, 2018 at 12:22 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:04 AM, Kentaro Hara <har...@chromium.org> wrote:

+1 to creating a clear guideline.

I personally feel that the following guideline is fine though:

- STL is allowed only at the boundary to interoperate with the rest of the world (Is this vague?)

Just to clarify: can I store net::CanonicalCookie in Document::cookie_? I got it directly from network service (which is at the boundary), and will use to return document.cookie string when asked.

Are you asking if you can store GURL in Document::cookie_url_?

The answer would be no. It should use KURL. (It won't make sense to store GURL in this case because document.cookie must return String, not std::string.)

Maybe I can rephrase it as follows:

- STL types passed in by an embedder must be *immediately* converted to Blink types using converters (except very limited cases where the immediate conversion significantly affects performance).

I'm afraid ​"significantly" feels still vague, or may add another myth that just sound we can't use STL even when it could look more reasonable. For example what if we add something like:

 

"When the value is to be ingested, used or held by any other code that uses Blink types (including IDL) the value must be immediately converted to Blink types."  Could it work?  We can add something like "when in doubt, convert them to Blink types"  too to be conservative.

​For example if some value comes from some code that uses STL types and then to be only ingested by the code that also uses STL types I think using STL types should be probably just allowed regardless of performance implications.

I also think that just plainly writing out that why we have Blink types and why we do not want to use STL types in certain cases could be helpful (Oilpan doc covers a lot, while there're also a few others).

This sounds reasonable and resolves the field trials usecase.

However, it means we should keep the WebCanonicalCookie class, since we should convert from "embedder" net::CanonicalCookie type immediately to some Blink type. Which brings us back to original question from Daniel.

Yeah, WebCanonicalCookie looks tricky. I agree that WebCanonicalCookie is necessary, although it won't need to be "Web".

- Blink can receive net::CanonicalCookie (i.e., the embedder doesn't need to covert it to WebCanonicalCookie before passing it to Blink).

- Blink defines blink::CanonicalCookie in platform/network/. blink::CanonicalCookie holds Strings and KURLs.

- When Blink receives net::CanonicalCookie, Blink immediately converts it to blink::CanonicalCookie using converters for String <=> std::string, KURL <=> GURL etc.

Slightly orthogonal, but we tend to have wrapper code in platform/ and in most cases it makes sense, but in other cases just using STL or base types directly in core/ could make more sense from what we could induce from blink/renderer/README.md.

Could we also be clearer whether using platform/ for having such wrapper types is preferable (but of course only when we can't avoid having such wrapper types), or rather we should just try to see where the code makes most sense (w.r.t the blink/renderer/README.md) and use STL and base types even in core/ when necessary (but only if the usage meets the above guideline)?  My assumption is that we do prefer the latter, but the current rule tends to incentivise the other way around so wanted to make sure.

[Kentaro Hara]

In any case, a big +1 to clarifying a guideline...

On Mon, Jun 4, 2018 at 11:43 AM Kinuko Yasuda <kin...@chromium.org> wrote:

On Mon, Jun 4, 2018 at 10:48 AM Kentaro Hara <har...@chromium.org> wrote:

On Sat, Jun 2, 2018 at 3:24 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:37 AM, Kentaro Hara <har...@chromium.org> wrote:

On Sat, Jun 2, 2018 at 12:22 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:04 AM, Kentaro Hara <har...@chromium.org> wrote:

+1 to creating a clear guideline.

I personally feel that the following guideline is fine though:

- STL is allowed only at the boundary to interoperate with the rest of the world (Is this vague?)

Just to clarify: can I store net::CanonicalCookie in Document::cookie_? I got it directly from network service (which is at the boundary), and will use to return document.cookie string when asked.

Are you asking if you can store GURL in Document::cookie_url_?

The answer would be no. It should use KURL. (It won't make sense to store GURL in this case because document.cookie must return String, not std::string.)

Maybe I can rephrase it as follows:

- STL types passed in by an embedder must be *immediately* converted to Blink types using converters (except very limited cases where the immediate conversion significantly affects performance).

I'm afraid ​"significantly" feels still vague, or may add another myth that just sound we can't use STL even when it could look more reasonable. For example what if we add something like:

 

"When the value is to be ingested, used or held by any other code that uses Blink types (including IDL) the value must be immediately converted to Blink types."  Could it work?  We can add something like "when in doubt, convert them to Blink types"  too to be conservative.

​For example if some value comes from some code that uses STL types and then to be only ingested by the code that also uses STL types I think using STL types should be probably just allowed regardless of performance implications.

Do you have any concrete example? If the passed-in STL types are only consumed by the code that also uses STL types, I'm wondering why the code needs to be put in Blink.

I also think that just plainly writing out that why we have Blink types and why we do not want to use STL types in certain cases could be helpful (Oilpan doc covers a lot, while there're also a few others).

This sounds reasonable and resolves the field trials usecase.

However, it means we should keep the WebCanonicalCookie class, since we should convert from "embedder" net::CanonicalCookie type immediately to some Blink type. Which brings us back to original question from Daniel.

Yeah, WebCanonicalCookie looks tricky. I agree that WebCanonicalCookie is necessary, although it won't need to be "Web".

- Blink can receive net::CanonicalCookie (i.e., the embedder doesn't need to covert it to WebCanonicalCookie before passing it to Blink).

- Blink defines blink::CanonicalCookie in platform/network/. blink::CanonicalCookie holds Strings and KURLs.

- When Blink receives net::CanonicalCookie, Blink immediately converts it to blink::CanonicalCookie using converters for String <=> std::string, KURL <=> GURL etc.

Slightly orthogonal, but we tend to have wrapper code in platform/ and in most cases it makes sense, but in other cases just using STL or base types directly in core/ could make more sense from what we could induce from blink/renderer/README.md.

Could we also be clearer whether using platform/ for having such wrapper types is preferable (but of course only when we can't avoid having such wrapper types), or rather we should just try to see where the code makes most sense (w.r.t the blink/renderer/README.md) and use STL and base types even in core/ when necessary (but only if the usage meets the above guideline)?  My assumption is that we do prefer the latter, but the current rule tends to incentivise the other way around so wanted to make sure.

Just to clarify, what wrapper code are you talking about specifically?

IMHO wrapper classes that just wrap STL / base types wouldn't make sense, because core/ can directly use STL / base types not banned in Blink. You need to use converters to use STL / base types banned in Blink but won't need wrapper classes anyway.

[Kinuko Yasuda]

On Mon, Jun 4, 2018 at 2:30 PM Kentaro Hara <har...@chromium.org> wrote:

In any case, a big +1 to clarifying a guideline...

​Yep, thanks for ​trying to make things clearer.

 

On Mon, Jun 4, 2018 at 11:43 AM Kinuko Yasuda <kin...@chromium.org> wrote:

On Mon, Jun 4, 2018 at 10:48 AM Kentaro Hara <har...@chromium.org> wrote:

On Sat, Jun 2, 2018 at 3:24 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:37 AM, Kentaro Hara <har...@chromium.org> wrote:

On Sat, Jun 2, 2018 at 12:22 AM Dmitry Gozman <dgo...@chromium.org> wrote:

On Fri, Jun 1, 2018 at 8:04 AM, Kentaro Hara <har...@chromium.org> wrote:

+1 to creating a clear guideline.

I personally feel that the following guideline is fine though:

- STL is allowed only at the boundary to interoperate with the rest of the world (Is this vague?)

Just to clarify: can I store net::CanonicalCookie in Document::cookie_? I got it directly from network service (which is at the boundary), and will use to return document.cookie string when asked.

Are you asking if you can store GURL in Document::cookie_url_?

The answer would be no. It should use KURL. (It won't make sense to store GURL in this case because document.cookie must return String, not std::string.)

Maybe I can rephrase it as follows:

- STL types passed in by an embedder must be *immediately* converted to Blink types using converters (except very limited cases where the immediate conversion significantly affects performance).

I'm afraid ​"significantly" feels still vague, or may add another myth that just sound we can't use STL even when it could look more reasonable. For example what if we add something like:

 

"When the value is to be ingested, used or held by any other code that uses Blink types (including IDL) the value must be immediately converted to Blink types."  Could it work?  We can add something like "when in doubt, convert them to Blink types"  too to be conservative.

​For example if some value comes from some code that uses STL types and then to be only ingested by the code that also uses STL types I think using STL types should be probably just allowed regardless of performance implications.

Do you have any concrete example? If the passed-in STL types are only consumed by the code that also uses STL types, I'm wondering why the code needs to be put in Blink.

Typical example cases could be where we do have some logic that lives in Blink and want to call multiple //net or other base classes to get the final results that are to be consumed by other Blink classes.  (My the other response below could probably also give one concrete example)

 

I also think that just plainly writing out that why we have Blink types and why we do not want to use STL types in certain cases could be helpful (Oilpan doc covers a lot, while there're also a few others).

This sounds reasonable and resolves the field trials usecase.

However, it means we should keep the WebCanonicalCookie class, since we should convert from "embedder" net::CanonicalCookie type immediately to some Blink type. Which brings us back to original question from Daniel.

Yeah, WebCanonicalCookie looks tricky. I agree that WebCanonicalCookie is necessary, although it won't need to be "Web".

- Blink can receive net::CanonicalCookie (i.e., the embedder doesn't need to covert it to WebCanonicalCookie before passing it to Blink).

- Blink defines blink::CanonicalCookie in platform/network/. blink::CanonicalCookie holds Strings and KURLs.

- When Blink receives net::CanonicalCookie, Blink immediately converts it to blink::CanonicalCookie using converters for String <=> std::string, KURL <=> GURL etc.

Slightly orthogonal, but we tend to have wrapper code in platform/ and in most cases it makes sense, but in other cases just using STL or base types directly in core/ could make more sense from what we could induce from blink/renderer/README.md.

Could we also be clearer whether using platform/ for having such wrapper types is preferable (but of course only when we can't avoid having such wrapper types), or rather we should just try to see where the code makes most sense (w.r.t the blink/renderer/README.md) and use STL and base types even in core/ when necessary (but only if the usage meets the above guideline)?  My assumption is that we do prefer the latter, but the current rule tends to incentivise the other way around so wanted to make sure.

Just to clarify, what wrapper code are you talking about specifically?

One example could be the one Daniel mentioned in his first email, i.e. one core/ logic wants to get some field trial values via base::GetFieldTrialParamValueByFeature, which is just to call one base method but the returned value is std::string and we also often want to call some base:: method to extract actual values from that (e.g. to split the comma-separated values or to get numeric values).  We could do either:

1) Do the same as other chromium code does, i.e. get the values in std::string and call common base:: functions to extract necessary values to get the final parameter values.  If the values are to be used by other Blink code convert the results into Blink types at this time.

2) Get the values in std::string, and immediately convert them into WTF strings.  Call WTF utilities or write some code (which could be the combination of multiple WTF calls) to extract necessary values.

3) Have a generic thin wrapper for GetFieldTrialParamValueByFeature in platform/.

I prefer #1 most and #3 least.  If we want to stick to 'convert the STL values immediately into Blink types' #2 should be the recommendation, which could make sense but I feel it's okay to allow #1 too if the resulting code can be simpler.

 

--

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/CABg10jx0-eNvd4YmQOYdohuBBwRvhbr%3D2TDbHMPT66jZS_nN6w%40mail.gmail.com.

[Kentaro Hara]

One example could be the one Daniel mentioned in his first email, i.e. one core/ logic wants to get some field trial values via base::GetFieldTrialParamValueByFeature, which is just to call one base method but the returned value is std::string and we also often want to call some base:: method to extract actual values from that (e.g. to split the comma-separated values or to get numeric values).  We could do either: 

 

1) Do the same as other chromium code does, i.e. get the values in std::string and call common base:: functions to extract necessary values to get the final parameter values.  If the values are to be used by other Blink code convert the results into Blink types at this time.
2) Get the values in std::string, and immediately convert them into WTF strings.  Call WTF utilities or write some code (which could be the combination of multiple WTF calls) to extract necessary values.
3) Have a generic thin wrapper for GetFieldTrialParamValueByFeature in platform/. 

 

I prefer #1 most and #3 least.  If we want to stick to 'convert the STL values immediately into Blink types' #2 should be the recommendation, which could make sense but I feel it's okay to allow #1 too if the resulting code can be simpler.

Thanks, I got your point.

Yeah, #1 would make sense in this particular case. My worry about recommending #1 as a guideline is that then it may allow people to pass around std::string until they return the value to JavaScript (i.e., std::string is scattered in the Blink code base). I feel like that #1 sounds more vague than #2 in the sense that #1 doesn't define when the conversion should be done.

For example, adopting #1 would mean that we allow using net::CanonicalCookie (which holds std::strings and GURLs) in Blink. Then all call sites of getters of net::CanonicalCookie will get std::strings and GURLs. Then we may end up with scattering std::strings and GURLs in Blink...

So I thought that it would be nicer to make #2 as a guideline % some case-by-case exceptions for performance / code simplicity. However, IMHO I don't have any good idea here. I want to hear thoughts of Daniel, Dmitry, Jeremy and others as well :)

[dan...@chromium.org]

On Mon, Jun 4, 2018 at 4:07 AM Kentaro Hara <har...@chromium.org> wrote:

One example could be the one Daniel mentioned in his first email, i.e. one core/ logic wants to get some field trial values via base::GetFieldTrialParamValueByFeature, which is just to call one base method but the returned value is std::string and we also often want to call some base:: method to extract actual values from that (e.g. to split the comma-separated values or to get numeric values).  We could do either: 

 

1) Do the same as other chromium code does, i.e. get the values in std::string and call common base:: functions to extract necessary values to get the final parameter values.  If the values are to be used by other Blink code convert the results into Blink types at this time.
2) Get the values in std::string, and immediately convert them into WTF strings.  Call WTF utilities or write some code (which could be the combination of multiple WTF calls) to extract necessary values.
3) Have a generic thin wrapper for GetFieldTrialParamValueByFeature in platform/. 

 

I prefer #1 most and #3 least.  If we want to stick to 'convert the STL values immediately into Blink types' #2 should be the recommendation, which could make sense but I feel it's okay to allow #1 too if the resulting code can be simpler.

Thanks, I got your point.

Yeah, #1 would make sense in this particular case. My worry about recommending #1 as a guideline is that then it may allow people to pass around std::string until they return the value to JavaScript (i.e., std::string is scattered in the Blink code base). I feel like that #1 sounds more vague than #2 in the sense that #1 doesn't define when the conversion should be done.

For example, adopting #1 would mean that we allow using net::CanonicalCookie (which holds std::strings and GURLs) in Blink. Then all call sites of getters of net::CanonicalCookie will get std::strings and GURLs. Then we may end up with scattering std::strings and GURLs in Blink...

So I thought that it would be nicer to make #2 as a guideline % some case-by-case exceptions for performance / code simplicity. However, IMHO I don't have any good idea here. I want to hear thoughts of Daniel, Dmitry, Jeremy and others as well :)

A rule allowing std:: and base:: types on the stack, but not as class members (even transitively), except for well-documented performance exceptions, might be a way to write guidance. I thought maybe also to say and not allowed as function arguments, but helper functions could be useful. Maybe not as class member function arguments, only static methods. At least these types of rules would be concrete around language concepts and not just the authors impressions.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jynAA7-HtwHZFzU7D6GG03rLhHWNwTBG9xBWgJ-zHeM-g%40mail.gmail.com.

[Dmitry Gozman]

On Mon, Jun 4, 2018 at 5:30 AM, <dan...@chromium.org> wrote:

On Mon, Jun 4, 2018 at 4:07 AM Kentaro Hara <har...@chromium.org> wrote:

One example could be the one Daniel mentioned in his first email, i.e. one core/ logic wants to get some field trial values via base::GetFieldTrialParamValueByFeature, which is just to call one base method but the returned value is std::string and we also often want to call some base:: method to extract actual values from that (e.g. to split the comma-separated values or to get numeric values).  We could do either: 

 

1) Do the same as other chromium code does, i.e. get the values in std::string and call common base:: functions to extract necessary values to get the final parameter values.  If the values are to be used by other Blink code convert the results into Blink types at this time.
2) Get the values in std::string, and immediately convert them into WTF strings.  Call WTF utilities or write some code (which could be the combination of multiple WTF calls) to extract necessary values.
3) Have a generic thin wrapper for GetFieldTrialParamValueByFeature in platform/. 

 

I prefer #1 most and #3 least.  If we want to stick to 'convert the STL values immediately into Blink types' #2 should be the recommendation, which could make sense but I feel it's okay to allow #1 too if the resulting code can be simpler.

Thanks, I got your point.

Yeah, #1 would make sense in this particular case. My worry about recommending #1 as a guideline is that then it may allow people to pass around std::string until they return the value to JavaScript (i.e., std::string is scattered in the Blink code base). I feel like that #1 sounds more vague than #2 in the sense that #1 doesn't define when the conversion should be done.

For example, adopting #1 would mean that we allow using net::CanonicalCookie (which holds std::strings and GURLs) in Blink. Then all call sites of getters of net::CanonicalCookie will get std::strings and GURLs. Then we may end up with scattering std::strings and GURLs in Blink...

So I thought that it would be nicer to make #2 as a guideline % some case-by-case exceptions for performance / code simplicity. However, IMHO I don't have any good idea here. I want to hear thoughts of Daniel, Dmitry, Jeremy and others as well :)

A rule allowing std:: and base:: types on the stack, but not as class members (even transitively), except for well-documented performance exceptions, might be a way to write guidance. I thought maybe also to say and not allowed as function arguments, but helper functions could be useful. Maybe not as class member function arguments, only static methods. At least these types of rules would be concrete around language concepts and not just the authors impressions.

+1. The "no STL members" rule is pretty clear and achieves most of the goals we have in mind. Still leaves a wrapper for canonical cookie (since we store it somewhere), but allows us to call various helper functions operating STL if those are available. For function arguments, we could say "go with WTF by default" and this should cover most of the code which already has WTF types in hand.

 

 

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/CABg10jx0-eNvd4YmQOYdohuBBwRvhbr%3D2TDbHMPT66jZS_nN6w%40mail.gmail.com.

--

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.

[Jeremy Roman]

On Mon, Jun 4, 2018 at 8:30 AM, <dan...@chromium.org> wrote:

On Mon, Jun 4, 2018 at 4:07 AM Kentaro Hara <har...@chromium.org> wrote:

One example could be the one Daniel mentioned in his first email, i.e. one core/ logic wants to get some field trial values via base::GetFieldTrialParamValueByFeature, which is just to call one base method but the returned value is std::string and we also often want to call some base:: method to extract actual values from that (e.g. to split the comma-separated values or to get numeric values).  We could do either: 

 

1) Do the same as other chromium code does, i.e. get the values in std::string and call common base:: functions to extract necessary values to get the final parameter values.  If the values are to be used by other Blink code convert the results into Blink types at this time.
2) Get the values in std::string, and immediately convert them into WTF strings.  Call WTF utilities or write some code (which could be the combination of multiple WTF calls) to extract necessary values.
3) Have a generic thin wrapper for GetFieldTrialParamValueByFeature in platform/. 

 

I prefer #1 most and #3 least.  If we want to stick to 'convert the STL values immediately into Blink types' #2 should be the recommendation, which could make sense but I feel it's okay to allow #1 too if the resulting code can be simpler.

Thanks, I got your point.

Yeah, #1 would make sense in this particular case. My worry about recommending #1 as a guideline is that then it may allow people to pass around std::string until they return the value to JavaScript (i.e., std::string is scattered in the Blink code base). I feel like that #1 sounds more vague than #2 in the sense that #1 doesn't define when the conversion should be done.

For example, adopting #1 would mean that we allow using net::CanonicalCookie (which holds std::strings and GURLs) in Blink. Then all call sites of getters of net::CanonicalCookie will get std::strings and GURLs. Then we may end up with scattering std::strings and GURLs in Blink...

So I thought that it would be nicer to make #2 as a guideline % some case-by-case exceptions for performance / code simplicity. However, IMHO I don't have any good idea here. I want to hear thoughts of Daniel, Dmitry, Jeremy and others as well :)

A rule allowing std:: and base:: types on the stack, but not as class members (even transitively), except for well-documented performance exceptions, might be a way to write guidance. I thought maybe also to say and not allowed as function arguments, but helper functions could be useful. Maybe not as class member function arguments, only static methods. At least these types of rules would be concrete around language concepts and not just the authors impressions.

I'm a little hesitant to be this prescriptive. I think I'd leave it as something like "convert to WTF types as soon as reasonable". We probably don't want IDL methods doing STL->WTF conversion on each call, for instance, but fairly local uses (even in tiny anonymous-namespace classes) seem okay.

(And as I said earlier, I actually am tempted to confuse the issue even more, because I think WTF::CString should probably be replaced by std::string.)

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/CABg10jx0-eNvd4YmQOYdohuBBwRvhbr%3D2TDbHMPT66jZS_nN6w%40mail.gmail.com.

--

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/CABg10jynAA7-HtwHZFzU7D6GG03rLhHWNwTBG9xBWgJ-zHeM-g%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+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/CAHtyhaQ1kjOBLcakWCrQ20BHDNtO4fhO0dNFTD1z8Gk0-Ty-DQ%40mail.gmail.com.

[Daniel Cheng]

On Mon, Jun 4, 2018 at 7:39 AM Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jun 4, 2018 at 8:30 AM, <dan...@chromium.org> wrote:

On Mon, Jun 4, 2018 at 4:07 AM Kentaro Hara <har...@chromium.org> wrote:

One example could be the one Daniel mentioned in his first email, i.e. one core/ logic wants to get some field trial values via base::GetFieldTrialParamValueByFeature, which is just to call one base method but the returned value is std::string and we also often want to call some base:: method to extract actual values from that (e.g. to split the comma-separated values or to get numeric values).  We could do either: 

 

1) Do the same as other chromium code does, i.e. get the values in std::string and call common base:: functions to extract necessary values to get the final parameter values.  If the values are to be used by other Blink code convert the results into Blink types at this time.
2) Get the values in std::string, and immediately convert them into WTF strings.  Call WTF utilities or write some code (which could be the combination of multiple WTF calls) to extract necessary values.
3) Have a generic thin wrapper for GetFieldTrialParamValueByFeature in platform/. 

 

I prefer #1 most and #3 least.  If we want to stick to 'convert the STL values immediately into Blink types' #2 should be the recommendation, which could make sense but I feel it's okay to allow #1 too if the resulting code can be simpler.

Thanks, I got your point.

Yeah, #1 would make sense in this particular case. My worry about recommending #1 as a guideline is that then it may allow people to pass around std::string until they return the value to JavaScript (i.e., std::string is scattered in the Blink code base). I feel like that #1 sounds more vague than #2 in the sense that #1 doesn't define when the conversion should be done.

For example, adopting #1 would mean that we allow using net::CanonicalCookie (which holds std::strings and GURLs) in Blink. Then all call sites of getters of net::CanonicalCookie will get std::strings and GURLs. Then we may end up with scattering std::strings and GURLs in Blink...

So I thought that it would be nicer to make #2 as a guideline % some case-by-case exceptions for performance / code simplicity. However, IMHO I don't have any good idea here. I want to hear thoughts of Daniel, Dmitry, Jeremy and others as well :)

A rule allowing std:: and base:: types on the stack, but not as class members (even transitively), except for well-documented performance exceptions, might be a way to write guidance. I thought maybe also to say and not allowed as function arguments, but helper functions could be useful. Maybe not as class member function arguments, only static methods. At least these types of rules would be concrete around language concepts and not just the authors impressions.

​Would this rule apply transitively? i.e. class members shouldn't transitively contain members that contain STL types?​

 

I'm a little hesitant to be this prescriptive. I think I'd leave it as something like "convert to WTF types as soon as reasonable". We probably don't want IDL methods doing STL->WTF conversion on each call, for instance, but fairly local uses (even in tiny anonymous-namespace classes) seem okay.

Doing STL->WTF conversions with each call isn't great, but it doesn't actually seem worse than what we already do today: any IDL method that's backed by code in //content/renderer already does this (e.g. document.cookie currently requires a STL->WTF conversion for each invocation).​

Also, to provide some additional background, we (pwnall, jsbell, dgozman, me) did previously discuss the idea of having a blink::CanonicalCookie and having a Blink variant typemap. The issue we ran into here was how to share the cookie parsing code between the Blink and the non-Blink variants.

Daniel

 

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jx0-eNvd4YmQOYdohuBBwRvhbr%3D2TDbHMPT66jZS_nN6w%40mail.gmail.com.

--

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.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jynAA7-HtwHZFzU7D6GG03rLhHWNwTBG9xBWgJ-zHeM-g%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-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

[Kentaro Hara]

We discussed this at arch-leads.

A rough conclusion is:

- Let's go with the "no STL members" rule.

- It means that net::CanonicalCookie needs a wrapper in Blink. (Note: The wrapper doesn't need to be exposed to "Web".) This is a bit unfortunate but this pattern wouldn't be that common.

- How to treat CString should be discussed separately.

- dgozman@ will write a guideline.

(Correct me if my understanding is not right :-)

[Kinuko Yasuda]

Thanks for updating.

(Fwiw let me give +1 to eventually deprecate CString sometime somehow.)

[Kentaro Hara]

As far as I grep in the code base, it looks rare that CString is used outside the machine stack.

We can probably just replace CString with std::string as Jeremy suggested and apply the same rule: "no STL members".

[Kentaro Hara]

Hi

A bunch of discussions about guidelines have been happening on platform-architecture-dev@. I want to make sure that all agreed-on guidelines are written in one place in the code base (not only in the discussion log of platform-architecture-dev@).

I think that Blink C++ Style Guide and blink/renderer/README.md would be good places to collect all Blink-related guidelines. (Yeah, they are two places -- but it would make sense to separate a coding style guide from a guide of more semantic rules.)

Thoughts?

[Yuta Kitamura]

On Wed, Jun 6, 2018 at 1:33 PM Kentaro Hara <har...@chromium.org> wrote:

As far as I grep in the code base, it looks rare that CString is used outside the machine stack.

We can probably just replace CString with std::string as Jeremy suggested and apply the same rule: "no STL members".

I have a little bit of concern about the allocator -- switching to std::string means we'll no longer use PartitionAlloc, right? I don't expect that's going to be very problematic, but we need to be aware of that.

[Yuta Kitamura]

Perhaps that may be a problem security-wise?

 

[Kentaro Hara]

Thanks yuta-san!

Security-wise: Yes, it's nice to allocate more objects on PartitionAlloc (Note: My hope is to replace tcmalloc with PartitionAlloc eventually). However, there're already a ton of objects allocated with tcmalloc (e.g., std::string in //content/renderer/). So I don't think that moving CString to tcmalloc makes any difference. Security-wise, a right approach would be to switch the memory allocator of std::string to PartitionAlloc rather than avoiding using std::string.

Tracing-wise: As long as CString is used only on the stack, it will be gone in the very near future, so we won't need to worry about it too much.

[Mike West]

+palmer@ for the PartitionAlloc discussion.

-mike

--

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/CABg10jw7_oM0XtTkh7VWHWJAExa-X21EFGwCgZu46%3Dmy76km4g%40mail.gmail.com.

[Dmitry Gozman]

This sounds very reasonable.

Thanks,

Dmitry

[Daniel Cheng]

This makes sense to me as well. We can also cross-link between them to make one more discoverable from the other.

Daniel

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

[Kinuko Yasuda]

Sounds good to me too!