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

Issue 19612004: Support sending MIDI data with throttling (Closed)

Created:
7 years, 5 months ago by Chris Rogers
Modified:
7 years, 4 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Support sending MIDI data with throttling The Web MIDI API allows sending of MIDI data, but we need to take care to not send too much data from the renderer to the browser. We implement a throttling mechanism where the browser process acknowledges the number of sent bytes back to the renderer, and the renderer only sends new data if it hasn't already sent too much unacknowledged data. BUG=163795 TEST=none Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214570

Patch Set 1 #

Patch Set 2 : more comments #

Total comments: 17

Patch Set 3 : Address first comments - throttle sent bytes in browser too #

Patch Set 4 : #

Total comments: 2

Patch Set 5 : put lock around whole block #

Total comments: 12

Patch Set 6 : #

Total comments: 18

Patch Set 7 : de-indent block #

Patch Set 8 : Address Chris Palmer comments #

Patch Set 9 : #

Patch Set 10 : rebase #

Patch Set 11 : Fix Android compile #

Unified diffs Side-by-side diffs Delta from patch set Stats (+197 lines, -24 lines) Patch
M content/browser/renderer_host/media/midi_host.h View 1 2 3 4 5 2 chunks +17 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/midi_host.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +74 lines, -5 lines 0 comments Download
M content/common/media/midi_messages.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/media/midi_message_filter.h View 1 2 3 4 5 2 chunks +7 lines, -0 lines 0 comments Download
M content/renderer/media/midi_message_filter.cc View 1 2 3 4 5 6 7 5 chunks +35 lines, -9 lines 0 comments Download
M media/midi/midi_manager.h View 1 2 3 4 5 5 chunks +31 lines, -4 lines 0 comments Download
M media/midi/midi_manager.cc View 1 2 3 4 5 6 7 2 chunks +23 lines, -2 lines 0 comments Download
M media/midi/midi_manager_mac.h View 1 chunk +2 lines, -1 line 0 comments Download
M media/midi/midi_manager_mac.cc View 2 chunks +5 lines, -3 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
Chris Rogers
7 years, 5 months ago (2013-07-17 19:58:43 UTC) #1
Chris Rogers
piman: for content OWNERS and overall design scherkus: for media toyoshim: for general comments
7 years, 5 months ago (2013-07-17 22:33:58 UTC) #2
scherkus (not reviewing)
few comments but I want to back up a bit and sanity check a few ...
7 years, 5 months ago (2013-07-18 01:52:13 UTC) #3
piman
https://codereview.chromium.org/19612004/diff/1001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/19612004/diff/1001/content/browser/renderer_host/media/midi_host.cc#newcode90 content/browser/renderer_host/media/midi_host.cc:90: midi_manager_->DispatchSendMIDIData( I think we want to add a check ...
7 years, 5 months ago (2013-07-22 21:37:30 UTC) #4
Chris Rogers
Thanks for your reviews so far - PTAL https://codereview.chromium.org/19612004/diff/1001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/19612004/diff/1001/content/browser/renderer_host/media/midi_host.cc#newcode25 content/browser/renderer_host/media/midi_host.cc:25: const ...
7 years, 5 months ago (2013-07-24 21:34:24 UTC) #5
piman
https://codereview.chromium.org/19612004/diff/18001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/19612004/diff/18001/content/browser/renderer_host/media/midi_host.cc#newcode95 content/browser/renderer_host/media/midi_host.cc:95: if (sent_bytes_in_flight_ > kMaxInFlightBytes || if sent_bytes_in_flight_ can indeed ...
7 years, 5 months ago (2013-07-24 22:15:02 UTC) #6
Chris Rogers
https://codereview.chromium.org/19612004/diff/18001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/19612004/diff/18001/content/browser/renderer_host/media/midi_host.cc#newcode95 content/browser/renderer_host/media/midi_host.cc:95: if (sent_bytes_in_flight_ > kMaxInFlightBytes || On 2013/07/24 22:15:03, piman ...
7 years, 5 months ago (2013-07-24 23:04:48 UTC) #7
piman
lgtm
7 years, 5 months ago (2013-07-24 23:17:48 UTC) #8
scherkus (not reviewing)
re: worker pool, there's src/base/threading/worker_pool.h have I used it? no, but hey it might be ...
7 years, 5 months ago (2013-07-24 23:30:59 UTC) #9
Chris Rogers
Andrew, PTAL I had a look at the worker pool solution, but think it might ...
7 years, 5 months ago (2013-07-27 00:19:00 UTC) #10
scherkus (not reviewing)
lgtm w/ one nit https://codereview.chromium.org/19612004/diff/36001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/19612004/diff/36001/content/browser/renderer_host/media/midi_host.cc#newcode95 content/browser/renderer_host/media/midi_host.cc:95: { considering the scope of ...
7 years, 4 months ago (2013-07-29 17:46:23 UTC) #11
palmer
https://codereview.chromium.org/19612004/diff/36001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/19612004/diff/36001/content/browser/renderer_host/media/midi_host.cc#newcode109 content/browser/renderer_host/media/midi_host.cc:109: // permission. The GURL, not a render view ID, ...
7 years, 4 months ago (2013-07-29 19:55:24 UTC) #12
Chris Rogers
Chris, thanks for the comments! - PTAL https://codereview.chromium.org/19612004/diff/36001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/19612004/diff/36001/content/browser/renderer_host/media/midi_host.cc#newcode95 content/browser/renderer_host/media/midi_host.cc:95: { On ...
7 years, 4 months ago (2013-07-29 20:37:25 UTC) #13
palmer
LGTM. Thank you!
7 years, 4 months ago (2013-07-29 22:03:06 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/crogers@google.com/19612004/57002
7 years, 4 months ago (2013-07-30 07:33:21 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/crogers@google.com/19612004/57002
7 years, 4 months ago (2013-07-30 11:14:02 UTC) #16
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 4 months ago (2013-07-30 20:25:37 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/crogers@google.com/19612004/82001
7 years, 4 months ago (2013-07-30 23:04:43 UTC) #18
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 4 months ago (2013-07-31 01:33:17 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/crogers@google.com/19612004/82001
7 years, 4 months ago (2013-07-31 02:00:16 UTC) #20
commit-bot: I haz the power
7 years, 4 months ago (2013-07-31 05:16:51 UTC) #21
Message was sent while issue was closed.
Change committed as 214570

Powered by Google App Engine
This is Rietveld 408576698