RFC: allow base::Optional<T> in Blink

[Mounir Lamouri]

Hi blink-dev,

base::Optional<T> just landed in Chromium [1] and I think we should use
this opportunity to deprecate Nullable [2] and WTF::Optional [3] in
favour of the base/ type.

The dependency of base::Optional<T> are very simple:
- base/logging.h (which is already used in Blink)
- base/memory/aligned_memory.h (not directly included in Blink but
base/memory/ is heavily used)

WDYT?

[1]
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/tt_WqDRw-68/y2p23g_zMQAJ
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/bindings/core/v8/Nullable.h
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/wtf/Optional.h

-- Mounir

[Kentaro Hara]

+yutak

+1 to replacing WTF::Optional with base::Optional.

Though I'm not sure if we can replace Nullable with base::Optional because Nullable needs to trace underlying Oilpan objects.

--

Kentaro Hara, Tokyo, Japan

[Harald Alvestrand]

If we have base::Optional<T> on both sides of the content / Blink boundary, does that mean that we can use base::Optional<T> members in Web* classes?

Should we?
(I tried to use WTF::Optional<T> in an API class long ago, but failed for obvious reasons.)

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

[Kentaro Hara]

If we have base::Optional<T> on both sides of the content / Blink boundary, does that mean that we can use base::Optional<T> members in Web* classes? 

 

Should we?

No. The current policy of the Onion Soup project is to allow only wtf/ and platform/ to directly use base/. core/, modules/ and web/ need to use a thin wrapper defined in wtf/ or platform/. So you can replace the implementation of WTF::Optional with base::Optional, but you cannot remove WTF::Optional.

[Harald Alvestrand]

modules/ can't depend on wtf/ for types exposed to content, so that leaves us with another wrapper in platform/.

I do wonder what the benefit of all these wrappings is, if the semantics of all of them is going to be identical.

[Kentaro Hara]

modules/ can't depend on wtf/ for types exposed to content, so that leaves us with another wrapper in platform/.

Would you elaborate on what you want to do? Things exposed to content/ should be written in platform/ (not modules/), and platform/ is allowed to use base/. I don't think you need another wrapper in platform/.

I do wonder what the benefit of all these wrappings is, if the semantics of all of them is going to be identical.

We want to avoid the engine layer (core/ and modules/) from starting using base/ things randomly. Chromium and Blink have different assumptions and principles about performance, memory and code health, so we want to be careful about the unification.

[Yuta Kitamura]

On Tue, Apr 19, 2016 at 10:29 PM, Mounir Lamouri <mou...@lamouri.fr> wrote:

Hi blink-dev,

base::Optional<T> just landed in Chromium [1] and I think we should use
this opportunity to deprecate Nullable [2] and WTF::Optional [3] in
favour of the base/ type.

The dependency of base::Optional<T> are very simple:
- base/logging.h (which is already used in Blink)
- base/memory/aligned_memory.h (not directly included in Blink but
base/memory/ is heavily used)

WDYT?

I'm okay with replacing WTF::Optional with base::Optional, but core/ and modules/ can't directly use base/ according to our current policy. So you'll need to add a small shim in WTF.

Nullable is a binding concept and isn't a generic library; you shouldn't mess with it.

[Dana Jansens]

On Tue, Apr 19, 2016 at 10:41 PM, Yuta Kitamura <yu...@chromium.org> wrote:

On Tue, Apr 19, 2016 at 10:29 PM, Mounir Lamouri <mou...@lamouri.fr> wrote:

Hi blink-dev,

base::Optional<T> just landed in Chromium [1] and I think we should use
this opportunity to deprecate Nullable [2] and WTF::Optional [3] in
favour of the base/ type.

The dependency of base::Optional<T> are very simple:
- base/logging.h (which is already used in Blink)
- base/memory/aligned_memory.h (not directly included in Blink but
base/memory/ is heavily used)

WDYT?

I'm okay with replacing WTF::Optional with base::Optional, but core/ and modules/ can't directly use base/ according to our current policy. So you'll need to add a small shim in WTF.

https://codereview.chromium.org/1897863006/ is doing just that.

[Harald Alvestrand]

Late followup, because I just now got to try it:

I tried submitting a CL that had public/platform/WebMediaStreamTrack.h depend on base/optional.h

I got an error saying "Illegal include.... because of -base from third_party's include rules".

So *something* is forbidding the include. I'm not sure what.

[Dana Jansens]

On Thu, May 19, 2016 at 5:28 AM, 'Harald Alvestrand' via blink-dev <blin...@chromium.org> wrote:

Late followup, because I just now got to try it:

I tried submitting a CL that had public/platform/WebMediaStreamTrack.h depend on base/optional.h

I got an error saying "Illegal include.... because of -base from third_party's include rules".

So *something* is forbidding the include. I'm not sure what.

You can use wtf/Optional which is a type alias to the base one with some oilpan checks, or add base/optional.h to the platform/DEPS.

[Jeremy Roman]

On Thu, May 19, 2016 at 4:11 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, May 19, 2016 at 5:28 AM, 'Harald Alvestrand' via blink-dev <blin...@chromium.org> wrote:

Late followup, because I just now got to try it:

I tried submitting a CL that had public/platform/WebMediaStreamTrack.h depend on base/optional.h

I got an error saying "Illegal include.... because of -base from third_party's include rules".

So *something* is forbidding the include. I'm not sure what.

You can use wtf/Optional which is a type alias to the base one with some oilpan checks, or add base/optional.h to the platform/DEPS. 

Do we allow wtf in public/platform/ now?

[Dana Jansens]

On Thu, May 19, 2016 at 2:02 PM, Jeremy Roman <jbr...@chromium.org> wrote:

On Thu, May 19, 2016 at 4:11 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, May 19, 2016 at 5:28 AM, 'Harald Alvestrand' via blink-dev <blin...@chromium.org> wrote:

Late followup, because I just now got to try it:

I tried submitting a CL that had public/platform/WebMediaStreamTrack.h depend on base/optional.h

I got an error saying "Illegal include.... because of -base from third_party's include rules".

So *something* is forbidding the include. I'm not sure what.

You can use wtf/Optional which is a type alias to the base one with some oilpan checks, or add base/optional.h to the platform/DEPS. 

Do we allow wtf in public/platform/ now?

I guess it depends if the code in question is in an INSIDE_BLINK block or not for that directory. There's 2 roads forward anyhow, one of them should be appropriate for any given place in the codebase.

[Daniel Cheng]

On Thu, May 19, 2016 at 2:06 PM Dana Jansens <dan...@chromium.org> wrote:

On Thu, May 19, 2016 at 2:02 PM, Jeremy Roman <jbr...@chromium.org> wrote:

On Thu, May 19, 2016 at 4:11 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, May 19, 2016 at 5:28 AM, 'Harald Alvestrand' via blink-dev <blin...@chromium.org> wrote:

Late followup, because I just now got to try it:

I tried submitting a CL that had public/platform/WebMediaStreamTrack.h depend on base/optional.h

I got an error saying "Illegal include.... because of -base from third_party's include rules".

So *something* is forbidding the include. I'm not sure what.

You can use wtf/Optional which is a type alias to the base one with some oilpan checks, or add base/optional.h to the platform/DEPS. 

Do we allow wtf in public/platform/ now?

I guess it depends if the code in question is in an INSIDE_BLINK block or not for that directory. There's 2 roads forward anyhow, one of them should be appropriate for any given place in the codebase.

WTF is not exposed outside of Blink while public is, so in this case, base::Optional is probably the right thing. We've been pretty conservative about allowing usage of base/ in the public API, but in this particular instance, it should be OK: it's pretty self-contained, using logging.h (which is already allowed in Blink) and aligned_memory.h... which probably has some overlap with wtf/Alignment.h, but that seems like something we can sort out independently.

Daniel

[Elliott Sprehn]

In general Optional is pretty bad since it packs poorly, what do we need it for in the Web* classes?

[Harald Alvestrand]

Boolean + variable:

<type> m_x;

bool m_has_x : 1;

void setX(<type> x) {

   m_has_x = true;

   m_x = true;

}

<type> getX() {

  return m_x;

}

void clearX() {

   m_has_x = false;

}

Optional:

base::Optional<type> x;

That's 12 lines of code as opposed to 1 line.

In addition:

- No chance of the coder getting fancy with getter and setter names - it's always x=base::Optional<type>(value) and value=*x.

- Full set of operations by default (generated classes seem to omit the clearer)

- No chance of coder getting fancy with side effects on the getter and setter (if they want that, they shouldn't be using optional).

- Well known (by now) idiom for the operations, used by other parts of the codebase.

Yes, packing of optional (when implemented as bool + variable) is pretty bad. If there is a solution to that, and it can be written into optional, it will catch all usages, not just the ones we code packing for (one of the standard arguments for using standard constructs rather than inventing our own).

[Brett Wilson]

Optional is good when we really need that exact behavior, but it's not a great pattern to encourage in general. We probably would have added it to base long ago if I hadn't pushed back on more and better use-cases. In many cases, null pointers or default-constructed values work fine. Applying Optional without care can lead to anti-patterns getting propagated and actually more complicated code.

I would be inclines to wait for case where somebody really needs to it before figuring out what to do for the Web API. I think the current discussion is based on theoretical concerns and sharing types in this layer is inherently a bit tricky with the current rules.

Brett

[Jeffrey Yasskin]

On Tue, May 24, 2016 at 6:16 AM, 'Harald Alvestrand' via blink-dev <blin...@chromium.org> wrote:

Boolean + variable:

<type> m_x;

bool m_has_x : 1;

void setX(<type> x) {

   m_has_x = true;

   m_x = true;

}

<type> getX() {

  return m_x;

}

void clearX() {

   m_has_x = false;

}

Optional:

base::Optional<type> x;

That's 12 lines of code as opposed to 1 line.

In addition:

- No chance of the coder getting fancy with getter and setter names - it's always x=base::Optional<type>(value) and value=*x.

- Full set of operations by default (generated classes seem to omit the clearer)

- No chance of coder getting fancy with side effects on the getter and setter (if they want that, they shouldn't be using optional).

- Well known (by now) idiom for the operations, used by other parts of the codebase.

Yes, packing of optional (when implemented as bool + variable) is pretty bad. If there is a solution to that, and it can be written into optional, it will catch all usages, not just the ones we code packing for (one of the standard arguments for using standard constructs rather than inventing our own).

Unfortunately, we didn't get any optimized storage for the boolean into std::optional, so to improve the packing, we'd need to keep using base::Optional and implement an improvement. It's achievable to implement a type trait to identify an unused bit pattern in the contained type, and use that for the 'empty' case. It's much more difficult to have the compiler move the boolean into an unused bit in the containing type.

Jeffrey

[Wez]

Could/should we mitigate misuse of base::Optional<> with types like pointers, which already have a sensible "not set" value by providing template specializations for them that prevent them compiling?

[Dana Jansens]

On Tue, May 24, 2016 at 10:13 AM, Wez <w...@chromium.org> wrote:

Could/should we mitigate misuse of base::Optional<> with types like pointers, which already have a sensible "not set" value by providing template specializations for them that prevent them compiling?

I don't think we should deviate from std::optional, which will replace the base implementation eventually.

[Elliott Sprehn]

If we can do better than the stl with storage optimizations or better asserts than we don't need to replace base::Optional with std one. This isn't any different from wtf, base threading, atomics, time, etc. If we can do better than the stl, then we should.