Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2351943003:
Adapt ScriptStreamer to recent changes in v8 streaming. (Closed)
DescriptionAdapt ScriptStreamer to recent changes in v8 streaming.
- Depends on crrev.com/2354973002
- Also drive-by fix ASSERT -> DCHECK, since the presubmit script was complaining.
R=marja@chromium.org
BUG=v8:4947
Committed: https://crrev.com/046f328882f55a923b756e4ae963fbd064d97ea0
Cr-Commit-Position: refs/heads/master@{#420295}
Patch Set 1 #Patch Set 2 : prettified comments #Patch Set 3 : WTF::Mutex::locked is #ifdef-ed on ASSERT, so keep that one ASSERT around. #
Total comments: 3
Patch Set 4 : Marja's feedback. #Messages
Total messages: 20 (11 generated)
The CQ bit was checked by vogelheim@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...
Description was changed from ========== Adapt ScriptStreamer to recent changes in v8 streaming. (Also, drive-by fix ASSERT -> DCHECK, since the presubmit script was complaining.) R=marja@chromium.org BUG=v8:4947 ========== to ========== Adapt ScriptStreamer to recent changes in v8 streaming. - Depends on crrev.com/2354973002 - Also drive-by fix ASSERT -> DCHECK, since the presubmit script was complaining. R=marja@chromium.org BUG=v8:4947 ==========
The CQ bit was checked by vogelheim@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
lgtm w/ metacomment https://codereview.chromium.org/2351943003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right): https://codereview.chromium.org/2351943003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:433: // streaming if the encoding is different from the one we started This comment is not correct: we don't abort streaming if the encoding is different. convertEncoding returns false when the encoding not supported, and in that case we suppress the streaming. In other cases we just update m_encoding and carry on. A side though from checking convertEncoding: as we don't stream two byte scripts, is the V8 side of TwoByteExternalStreamingStream dead code?
https://codereview.chromium.org/2351943003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right): https://codereview.chromium.org/2351943003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:433: // streaming if the encoding is different from the one we started On 2016/09/20 19:03:55, marja wrote: > This comment is not correct: we don't abort streaming if the encoding is > different. convertEncoding returns false when the encoding not supported, and in > that case we suppress the streaming. In other cases we just update m_encoding > and carry on. Done. https://codereview.chromium.org/2351943003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:433: // streaming if the encoding is different from the one we started On 2016/09/20 19:03:55, marja wrote: > A side though from checking convertEncoding: as we don't stream two byte > scripts, is the V8 side of TwoByteExternalStreamingStream dead code? Within Chromium: Yes, I think so. This is a bit suprising... The V8 API supports it. Options are: 1, We keep TwoByteExternalStreamingStream in (despite it not being used by Chromium, I believe not by node.js or other clients either). 2, We UNREACHABLE() it in the API. 3, Or we change the API to remove it. Not sure I think about this, yet. At least it has unit tests, now. :-]
The CQ bit was checked by vogelheim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org Link to the patchset: https://codereview.chromium.org/2351943003/#ps60001 (title: "Marja's feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Yeah, we probably should return the two-byte stream from the API.
On Thu, Sep 22, 2016 at 10:49 AM, <marja@chromium.org> wrote: > Yeah, we probably should return the two-byte stream from the API. > "return .. from" == "remove .. from" ?? -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Sep 22, 2016 at 10:49 AM, <marja@chromium.org> wrote: > Yeah, we probably should return the two-byte stream from the API. > "return .. from" == "remove .. from" ?? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
haha, yes, remove
Message was sent while issue was closed.
Description was changed from ========== Adapt ScriptStreamer to recent changes in v8 streaming. - Depends on crrev.com/2354973002 - Also drive-by fix ASSERT -> DCHECK, since the presubmit script was complaining. R=marja@chromium.org BUG=v8:4947 ========== to ========== Adapt ScriptStreamer to recent changes in v8 streaming. - Depends on crrev.com/2354973002 - Also drive-by fix ASSERT -> DCHECK, since the presubmit script was complaining. R=marja@chromium.org BUG=v8:4947 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Adapt ScriptStreamer to recent changes in v8 streaming. - Depends on crrev.com/2354973002 - Also drive-by fix ASSERT -> DCHECK, since the presubmit script was complaining. R=marja@chromium.org BUG=v8:4947 ========== to ========== Adapt ScriptStreamer to recent changes in v8 streaming. - Depends on crrev.com/2354973002 - Also drive-by fix ASSERT -> DCHECK, since the presubmit script was complaining. R=marja@chromium.org BUG=v8:4947 Committed: https://crrev.com/046f328882f55a923b756e4ae963fbd064d97ea0 Cr-Commit-Position: refs/heads/master@{#420295} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/046f328882f55a923b756e4ae963fbd064d97ea0 Cr-Commit-Position: refs/heads/master@{#420295} |
