Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2327653002:
Support delegating serialization of host objects. (Closed)
DescriptionSupport delegating serialization of host objects.
This exposes an interface for the embedder to provide a delegate which can
serialize or deserialize embedder-specific objects, like Blink's DOM wrappers.
BUG=chromium:148757
Committed: https://crrev.com/d825492bb6ae0874400bde7456da1ed9f64b308c
Cr-Commit-Position: refs/heads/master@{#39422}
Patch Set 1 #Patch Set 2 : signed/unsigned conversion #Patch Set 3 : misc #Patch Set 4 : #pragma clang diagnostic #Patch Set 5 : MSVC does not like being told what clang will do #Patch Set 6 : promote scheduled exceptions after delegate calls that may throw them #
Total comments: 4
Patch Set 7 : Isolate* argument to delegate #
Total comments: 3
Messages
Total messages: 44 (34 generated)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/13972)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/14008)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WIP: Support delegating serialization of host objects. BUG=chromium:148757 ========== to ========== Support delegating serialization of host objects. This exposes an interface for the embedder to provide a delegate which can serialize or deserialize embedder-specific objects, like Blink's DOM wrappers. BUG=chromium:148757 ==========
jbroman@chromium.org changed reviewers: + jkummerow@chromium.org
The API methods for reading bytes and numbers from the wire don't use Maybe, so that the invariant that API methods that use Maybe throw exceptions on failure (and it seemed heavyweight to do the setup to throw an exception for each value read from the wire, but we could do so). For reference, the first (WIP) Blink CL that will follow this up is at https://codereview.chromium.org/2323413002.
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a comment. (Sorry for high review latency this week.) https://codereview.chromium.org/2327653002/diff/100001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2327653002/diff/100001/src/api.cc#newcode2851 src/api.cc:2851: i::Isolate* isolate = reinterpret_cast<i::Isolate*>(Isolate::GetCurrent()); AFAIK Isolate::GetCurrent() is deprecated and new calls to it should not be introduced. Please pass in the Isolate* as a parameter where it is needed. https://codereview.chromium.org/2327653002/diff/100001/src/api.cc#newcode2915 src/api.cc:2915: i::Isolate* isolate = reinterpret_cast<i::Isolate*>(Isolate::GetCurrent()); same here.
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jbroman@chromium.org changed reviewers: + jochen@chromium.org
+jochen for include/ https://codereview.chromium.org/2327653002/diff/100001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2327653002/diff/100001/src/api.cc#newcode2851 src/api.cc:2851: i::Isolate* isolate = reinterpret_cast<i::Isolate*>(Isolate::GetCurrent()); On 2016/09/14 at 01:09:20, Jakob Kummerow wrote: > AFAIK Isolate::GetCurrent() is deprecated and new calls to it should not be introduced. Please pass in the Isolate* as a parameter where it is needed. OK, done. In practice the delegate will already have the isolate on hand; it's just the default implementation that does this. Nonetheless, done. https://codereview.chromium.org/2327653002/diff/100001/src/api.cc#newcode2915 src/api.cc:2915: i::Isolate* isolate = reinterpret_cast<i::Isolate*>(Isolate::GetCurrent()); On 2016/09/14 at 01:09:20, Jakob Kummerow wrote: > same here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2327653002/diff/120001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2327653002/diff/120001/include/v8.h#newcode1690 include/v8.h:1690: virtual Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object); should this be pure virtual? https://codereview.chromium.org/2327653002/diff/120001/include/v8.h#newcode1764 include/v8.h:1764: virtual MaybeLocal<Object> ReadHostObject(Isolate* isolate); same here?
https://codereview.chromium.org/2327653002/diff/120001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2327653002/diff/120001/include/v8.h#newcode1690 include/v8.h:1690: virtual Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object); On 2016/09/14 at 15:44:31, jochen wrote: > should this be pure virtual? Ideally, yes. How does V8 usually manage rollout of this sort of thing? (That will immediately fail to pass the Blink bots, since that method is missing there.) Options include: - land an implementation in Blink first (without "override", and possibly with suppression of compiler warnings for missing "override" depending on what the warning settings are there) - land this default implementation, then remove it once Blink implements - continue to have a default implementation WDYT?
ah, ok. landing like this lgtm then.
On 2016/09/14 at 16:37:20, jochen wrote: > ah, ok. landing like this lgtm then. Thanks, CQ-ing. Am I understanding correctly that you would like this to become pure virtual once Blink overrides (after a delay)?
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2327653002/#ps120001 (title: "Isolate* argument to delegate")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Support delegating serialization of host objects. This exposes an interface for the embedder to provide a delegate which can serialize or deserialize embedder-specific objects, like Blink's DOM wrappers. BUG=chromium:148757 ========== to ========== Support delegating serialization of host objects. This exposes an interface for the embedder to provide a delegate which can serialize or deserialize embedder-specific objects, like Blink's DOM wrappers. BUG=chromium:148757 Committed: https://crrev.com/d825492bb6ae0874400bde7456da1ed9f64b308c Cr-Commit-Position: refs/heads/master@{#39422} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d825492bb6ae0874400bde7456da1ed9f64b308c Cr-Commit-Position: refs/heads/master@{#39422} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
