Fix passing blobs on MessagePort and BroadcastChannel. [chromium/src : master]
Marijn Kruisselbrink (Gerrit)
This change is ready for review.
To view, visit change 594495. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/w3c/web-platform-tests/pull/7887.
If this CL lands and Travis CI upstream is green, we will auto-merge the PR.
Note: Please check the Travis CI status (at the bottom of the PR) before landing this CL and only land this CL if the status is green. Otherwise a human needs to step in and resolve it manually. (This may be automated in the future, see https://crbug.com/711447)
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md#Automatic-export-process
Daniel Murphy (Gerrit)
Another callsite:
https://cs.chromium.org/chromium/src/content/renderer/service_worker/service_worker_context_client.cc?q=GetBlobFromUUID&l=1046&dr=C
Should we add a histogram there too?
Daniel Cheng (Gerrit)
Patch set 24:Code-Review +1
1 comment:
To view, visit change 594495. To unsubscribe, or for help writing mail filters, visit settings.
Josh Matthews (Gerrit)
Patch Set 24: Code-Review+1
(1 comment)
1 comment:
File third_party/WebKit/LayoutTests/external/wpt/webmessaging/Channel_postMessage_Blob.htm:
Patch Set #24, Line 26:
if (self.gc) self.gc();Since this is a non-standard API, per https://github.com/w3c/web-platform-tests/pull/7887#pullrequestreview-70408610 please add a TODO comment to each use of `self.gc()` in these tests that points at https://github.com/w3c/web-platform-tests/issues/7899 .
To view, visit change 594495. To unsubscribe, or for help writing mail filters, visit settings.
Marijn Kruisselbrink (Gerrit)
Patch Set 24:
Another callsite:
https://cs.chromium.org/chromium/src/content/renderer/service_worker/service_worker_context_client.cc?q=GetBlobFromUUID&l=1046&dr=CShould we add a histogram there too?
dmurph: I assume this was meant to be a comment for https://chromium-review.googlesource.com/c/chromium/src/+/727191 instead?
2 comments:
Patch Set #24, Line 26:
if (self.gc) self.gc();
Since this is a non-standard API, per https://github. […]
Done
File third_party/WebKit/common/message_port/cloneable_message.h:
Nit: sent
Done
To view, visit change 594495. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "address comments": https://github.com/w3c/web-platform-tests/pull/7887
Daniel Murphy (Gerrit)
Question: Do those tests fail before this change? I noticed that you marked them as general failures - should they start passing now?
Patch set 25:Code-Review +1
1 comment:
File third_party/WebKit/common/message_port/cloneable_message.cc:
Patch Set #25, Line 22:
blob->blob->Clone(MakeRequest(&blob_clone));IT'S BLOBS ALL THE WAY DOWN
To view, visit change 594495. To unsubscribe, or for help writing mail filters, visit settings.
Marijn Kruisselbrink (Gerrit)
Patch Set 25: Code-Review+1
(1 comment)
Question: Do those tests fail before this change? I noticed that you marked them as general failures - should they start passing now?
Yes, if you look at the layout test results (https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel_ng/568029/layout-test-results/external/wpt/webmessaging/broadcastchannel/blobs-actual.txt for example), these tests fail without mojo blobs, and pass with mojo blobs enabled (hence the TestExpectations that marks them as failing, but not for the virtual/mojo-blobs/ version)
1 comment:
Patch Set #25, Line 22:
blob->blob->Clone(MakeRequest(&blob_clone));IT'S BLOBS ALL THE WAY DOWN
Ack
To view, visit change 594495. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "rebase": https://github.com/w3c/web-platform-tests/pull/7887
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "fix one more typo in comment": https://github.com/w3c/web-platform-tests/pull/7887
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "address one more WPT review comment": https://github.com/w3c/web-platform-tests/pull/7887
Marijn Kruisselbrink (Gerrit)
Patch set 28:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"address one more WPT review comment" https://chromium-review.googlesource.com/c/594495/28
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/594495/28
Bot data: {"action": "start", "triggered_at": "2017-10-20T17:42:27.0Z", "cq_cfg_revision": "fc2b2f04ed20d88113c10951036ca403891c435f", "revision": "6356517ad0d7f247133b7b1947e7e95c95635a9b"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Fix passing blobs on MessagePort and BroadcastChannel.
Also add/update WPT tests to test this, and try to garbage collect the
blob being send before the message is received.
Bug: 740744, 351753
Change-Id: Ibe2f130dd3fa37130ceb0ed6330b5ba9282f9a7b
Reviewed-on: https://chromium-review.googlesource.com/594495
Commit-Queue: Marijn Kruisselbrink <m...@chromium.org>
Reviewed-by: Daniel Murphy <dmu...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510541}
---
M third_party/WebKit/LayoutTests/TestExpectations
M third_party/WebKit/LayoutTests/VirtualTestSuites
A third_party/WebKit/LayoutTests/external/wpt/webmessaging/Channel_postMessage_Blob.htm
M third_party/WebKit/LayoutTests/external/wpt/webmessaging/broadcastchannel/blobs.html
M third_party/WebKit/LayoutTests/external/wpt/webmessaging/broadcastchannel/resources/worker.js
A third_party/WebKit/LayoutTests/virtual/mojo-blobs/external/wpt/webmessaging/README.txt
M third_party/WebKit/Source/core/dom/BlinkCloneableMessageStructTraits.cpp
M third_party/WebKit/Source/core/dom/BlinkCloneableMessageStructTraits.h
M third_party/WebKit/common/message_port/cloneable_message.cc
M third_party/WebKit/common/message_port/cloneable_message.h
M third_party/WebKit/common/message_port/cloneable_message_struct_traits.cc
M third_party/WebKit/common/message_port/cloneable_message_struct_traits.h
M third_party/WebKit/common/message_port/message_port.mojom
13 files changed, 141 insertions(+), 4 deletions(-)
Blink WPT Bot (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/w3c/web-platform-tests/pull/7887