Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1226)

Issue 577733002: Mojo JS bindings: draining a DataPipe (Closed)

Created:
6 years, 3 months ago by hansmuller
Modified:
6 years, 2 months ago
CC:
abarth-chromium, Aaron Boodman, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Mojo JS bindings: draining a DataPipe Add a drainData(dataPipeHandle) utility function to the Mojo JS core module. The drainData() function asynchronously reads from the data pipe until the remote handle is closed or an error occurs. A Promise is returned whose settled value is an object like this: {result: core.RESULT_OK, buffer: dataArrayBuffer}. If the read failed, then the result will be the actual error code and the buffer will contain whatever was read before the error occurred. The drainData dataPipeHandle argument is closed automatically. BUG=414338 Committed: https://crrev.com/5a5cb9e791961a2491c4269fc0f2144f385dc173 Cr-Commit-Position: refs/heads/master@{#296750}

Patch Set 1 #

Patch Set 2 : Added async DataPipe reader to the demo content #

Patch Set 3 : Flattened the promise chain in main.js #

Patch Set 4 : Non-working C++ version of drainData() #

Patch Set 5 : Added a HCTB CHECK in GetPromise() #

Patch Set 6 : Enable v8 microtask execution #

Patch Set 7 : Revised the way the TaskObserver is managed #

Total comments: 19

Patch Set 8 : Reduce data buffer copying, add some documentation #

Total comments: 6

Patch Set 9 : Whitespace fixes #

Patch Set 10 : Use size_t for total data buffer size #

Patch Set 11 : More size_t #

Unified diffs Side-by-side diffs Delta from patch set Stats (+317 lines, -30 lines) Patch
M gin/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M gin/gin.gyp View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M gin/isolate_holder.cc View 1 2 3 4 5 6 4 chunks +16 lines, -0 lines 0 comments Download
M gin/public/isolate_holder.h View 1 2 3 4 5 6 2 chunks +14 lines, -0 lines 0 comments Download
A gin/run_microtasks_observer.h View 1 2 3 4 5 6 1 chunk +30 lines, -0 lines 0 comments Download
A gin/run_microtasks_observer.cc View 1 2 3 4 5 1 chunk +21 lines, -0 lines 0 comments Download
M mojo/apps/js/js_app.cc View 1 2 3 4 5 6 3 chunks +3 lines, -1 line 0 comments Download
M mojo/apps/js/main.js View 1 2 3 4 5 6 1 chunk +16 lines, -29 lines 0 comments Download
M mojo/bindings/js/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M mojo/bindings/js/core.cc View 1 2 3 4 5 6 7 3 chunks +14 lines, -0 lines 0 comments Download
A mojo/bindings/js/drain_data.h View 1 2 3 4 5 6 7 8 1 chunk +64 lines, -0 lines 0 comments Download
A mojo/bindings/js/drain_data.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +131 lines, -0 lines 0 comments Download
M mojo/mojo_base.gyp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (7 generated)
hansmuller
I'd appreciate it if you-all would take a look.
6 years, 3 months ago (2014-09-23 20:49:44 UTC) #3
Matt Perry
https://codereview.chromium.org/577733002/diff/140001/mojo/apps/js/main.js File mojo/apps/js/main.js (right): https://codereview.chromium.org/577733002/diff/140001/mojo/apps/js/main.js#newcode43 mojo/apps/js/main.js:43: var drainDataPromise = core.drainData(result.response.body); Is this temporary necessary? https://codereview.chromium.org/577733002/diff/140001/mojo/bindings/js/drain_data.cc ...
6 years, 3 months ago (2014-09-24 00:39:42 UTC) #4
hansmuller
Thanks for the review. I've made nearly all of the suggested changes. https://codereview.chromium.org/577733002/diff/140001/mojo/apps/js/main.js File mojo/apps/js/main.js ...
6 years, 3 months ago (2014-09-24 17:47:53 UTC) #5
Matt Perry
LGTM https://codereview.chromium.org/577733002/diff/140001/mojo/apps/js/main.js File mojo/apps/js/main.js (right): https://codereview.chromium.org/577733002/diff/140001/mojo/apps/js/main.js#newcode43 mojo/apps/js/main.js:43: var drainDataPromise = core.drainData(result.response.body); On 2014/09/24 17:47:53, hansmuller ...
6 years, 3 months ago (2014-09-24 21:30:47 UTC) #6
abarth-chromium
LGTM https://codereview.chromium.org/577733002/diff/160001/gin/isolate_holder.cc File gin/isolate_holder.cc (right): https://codereview.chromium.org/577733002/diff/160001/gin/isolate_holder.cc#newcode49 gin/isolate_holder.cc:49: base::MessageLoop::current()->RemoveTaskObserver(task_observer_.get()); Should this call RemoveRunMicrotasksObserver() ? https://codereview.chromium.org/577733002/diff/160001/mojo/bindings/js/core.cc File ...
6 years, 3 months ago (2014-09-25 00:01:42 UTC) #7
hansmuller
Thanks for your help with getting this sorted out! https://codereview.chromium.org/577733002/diff/160001/gin/isolate_holder.cc File gin/isolate_holder.cc (right): https://codereview.chromium.org/577733002/diff/160001/gin/isolate_holder.cc#newcode49 gin/isolate_holder.cc:49: ...
6 years, 2 months ago (2014-09-25 14:36:35 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/577733002/180001
6 years, 2 months ago (2014-09-25 14:37:13 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_swarming/builds/13014)
6 years, 2 months ago (2014-09-25 15:36:35 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/577733002/200001
6 years, 2 months ago (2014-09-25 16:19:06 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/577733002/220001
6 years, 2 months ago (2014-09-25 17:15:41 UTC) #17
commit-bot: I haz the power
Committed patchset #11 (id:220001) as c1e2cf843d9365c9c3e58c71ca0cb9054a4cf1fa
6 years, 2 months ago (2014-09-25 18:41:26 UTC) #18
commit-bot: I haz the power
6 years, 2 months ago (2014-09-25 18:43:04 UTC) #19
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/5a5cb9e791961a2491c4269fc0f2144f385dc173
Cr-Commit-Position: refs/heads/master@{#296750}

Powered by Google App Engine
This is Rietveld 408576698