At the moment, we can't use Optional<T> with Oilpan's container types, which means 'sequence<long>?' becomes
Optional<Vector<int32_t>>
while 'sequence<Document>?' would have to become
HeapVector<Document>*
this is confusing, but correct. Is that OK with others?
We could also make it possible to write Optional<HeapVector<>>. I don't know of any reason it won't work technically, we just currently ban it to avoid misuse (e.g. putting it in an Oilpan object and then slipping past some of the tracing safety checks).
FWIW I've got a patch here removing Nullable<T> from the tree and dropping all Oilpan checks from WTF::Optional<T>, we just need to decide whether the latter is the way to go.
We discussed this offline with haraken. Using Optional looks OK. We can remove OptionalGarbageCollected check from blink_gc_plugin and then replace Nullable with Optional. raphael, if you have the blink_gc_plugin patch ready please put it up for review. If not I could make the change for you.
Thanks for the follow-up.
Curiously enough, I've never hit the OptionalGarbageCollected checks here in my builds, possibly because I don't think we currently use Nullable<T> for GC types, and HeapVector<T> does not derive from GarbageCollected/GarbageCollectedMixin, but rather only has the IS_GARBAGE_COLLECTED_TYPE macro in its body. In this case, I wonder if it makes sense to remove that check from blink_gc_plugin or just leave it as-is.
If blink_gc_plugin doesn't block your CL then no worries. Please CC me on your CL and I'll look into how we should tighten up the blink_gc_plugin checks.
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/dd1b0228d93b79bb627f1f26091d9ab9ed0fd25e
commit dd1b0228d93b79bb627f1f26091d9ab9ed0fd25e
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Sat Jan 27 14:52:56 2018
wtf, oilpan: Remove GC checks from WTF::Optional<T>.
Architecturally, there is nothing really preventing garbage-collected types
from being used with Optional<T> other than avoiding misuse.
Relax the existing checks by removing the ones in place in Optional.h
itself, but keep OptionalGarbageCollected in //tools/clang/blink_gc_plugin.
In practice, this is a relaxation of the rules that:
* Allows Optional<HeapVector<T>> (and other Oilpan containers). These
containers do not inherit from GarbageCollected or GarbageCollectedMixin,
but do have the IS_GARBAGE_COLLECTED_TYPE() macro in their declarations.
As we are moving to replace the binding layer's Nullable<T> with wtf's
Optional<T>, doing so makes things uniform by letting Optional<> be used
with both Vector and HeapVector types so that Web IDL's
sequence<long>?
and
sequence<Document>?
are both translated into C++ as Optional<T>.
* Types that do inherit from GarbageCollected or GarbageCollectedMixin are
still disallowed by the checks in blink_gc_plugin since none of the
changes to get rid of Nullable<T> require them to be turned off or
relaxed. Web IDL's
Document?
is still represented as Document* in C++.
TraceTrait<T> had to be augmented with a specialization for Optional<T>, as
we may need to Trace() its inner type if it's a GC container type.
Bug: 798464
Change-Id: I12cad41976219654985f5a8d560e7db3f4cedf82
Reviewed-on: https://chromium-review.googlesource.com/888918
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#532217}
[modify] https://crrev.com/dd1b0228d93b79bb627f1f26091d9ab9ed0fd25e/third_party/WebKit/Source/platform/heap/TraceTraits.h
[modify] https://crrev.com/dd1b0228d93b79bb627f1f26091d9ab9ed0fd25e/third_party/WebKit/Source/platform/wtf/Optional.h
Comment 1 by yukishiino@chromium.org
, Jan 10 2018