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

Issue 18611005: Web MIDI: implement WebMIDIClient to handle permissions (Closed)

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

Description

Web MIDI: implement WebMIDIClient to handle permissions Implement WebMIDIClient to handle permissions and remove dependencies on old platform APIs. This transition is needed to remove old interfaces from platform APIs. BUG=163795 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213644

Patch Set 1 #

Patch Set 2 : (rebase) #

Total comments: 22

Patch Set 3 : jochen's review #3 #

Patch Set 4 : scherkus' review #5 #

Total comments: 2

Patch Set 5 : comment out all ipc macros #

Patch Set 6 : (rebase: LKGR looks fine) #

Patch Set 7 : (rebase) #

Patch Set 8 : MIDIDispatcher::OnMessageReceived #

Total comments: 2

Patch Set 9 : review #13 #

Total comments: 12

Patch Set 10 : review #16 and #17 #

Patch Set 11 : url/gurl.h #

Patch Set 12 : roll back "not used yet" code #

Total comments: 4

Patch Set 13 : review #24 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+173 lines, -66 lines) Patch
M content/browser/renderer_host/media/midi_host.h View 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/media/midi_host.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +9 lines, -10 lines 0 comments Download
M content/common/media/midi_messages.h View 8 9 10 11 2 chunks +3 lines, -5 lines 0 comments Download
M content/content_renderer.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
A content/renderer/media/midi_dispatcher.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +54 lines, -0 lines 0 comments Download
A content/renderer/media/midi_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +58 lines, -0 lines 0 comments Download
M content/renderer/media/midi_message_filter.h View 4 chunks +4 lines, -6 lines 0 comments Download
M content/renderer/media/midi_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +14 lines, -18 lines 0 comments Download
M content/renderer/media/renderer_webmidiaccessor_impl.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M content/renderer/media/renderer_webmidiaccessor_impl.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M content/renderer/render_view_impl.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +8 lines, -3 lines 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -0 lines 0 comments Download
M media/midi/midi_manager.h View 1 chunk +6 lines, -11 lines 0 comments Download
M media/midi/midi_manager.cc View 2 chunks +2 lines, -7 lines 0 comments Download

Messages

Total messages: 29 (0 generated)
Takashi Toyoshima
jochen: OWNERS review for */media scherkus: OWNERS review for content/renderer crogers: CCed for MIDI Hi, ...
7 years, 5 months ago (2013-07-12 11:35:53 UTC) #1
Takashi Toyoshima
Sorry, one important typo. > The client API is not added on blink side, so ...
7 years, 5 months ago (2013-07-12 11:37:46 UTC) #2
jochen (gone - plz use gerrit)
It looks like the browser side is still TBD? How will you handle the permissions ...
7 years, 5 months ago (2013-07-15 14:35:21 UTC) #3
Takashi Toyoshima
> It looks like the browser side is still TBD? How will you handle the ...
7 years, 5 months ago (2013-07-15 15:02:28 UTC) #4
scherkus (not reviewing)
https://codereview.chromium.org/18611005/diff/4001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/18611005/diff/4001/content/browser/renderer_host/media/midi_host.cc#newcode63 content/browser/renderer_host/media/midi_host.cc:63: approved = midi_manager_->StartSession(this); this is misleading MIDIManager::StartSession() returns true/false ...
7 years, 5 months ago (2013-07-15 17:28:24 UTC) #5
Takashi Toyoshima
Addressed jochen's comments in Patch Set 3. I'll work on scherkus's comment soon. https://codereview.chromium.org/18611005/diff/4001/content/renderer/media/midi_dispatcher.cc File ...
7 years, 5 months ago (2013-07-15 17:43:47 UTC) #6
Takashi Toyoshima
Thanks! https://codereview.chromium.org/18611005/diff/4001/content/browser/renderer_host/media/midi_host.cc File content/browser/renderer_host/media/midi_host.cc (right): https://codereview.chromium.org/18611005/diff/4001/content/browser/renderer_host/media/midi_host.cc#newcode63 content/browser/renderer_host/media/midi_host.cc:63: approved = midi_manager_->StartSession(this); Agreed. Here, I just rename ...
7 years, 5 months ago (2013-07-15 18:41:10 UTC) #7
scherkus (not reviewing)
you've got some compile failures is there an outstanding blink CL? https://codereview.chromium.org/18611005/diff/26001/content/renderer/media/midi_dispatcher.cc File content/renderer/media/midi_dispatcher.cc (right): ...
7 years, 5 months ago (2013-07-15 19:36:12 UTC) #8
Takashi Toyoshima
Hum... try bot looks checking out very old revision.
7 years, 5 months ago (2013-07-15 19:59:00 UTC) #9
scherkus (not reviewing)
On 2013/07/15 19:59:00, Takashi Toyoshima (chromium) wrote: > Hum... try bot looks checking out very ...
7 years, 5 months ago (2013-07-15 20:00:41 UTC) #10
Takashi Toyoshima
Comment out all IPC macros in Patch Set5, and post "git cl try -r 211650" ...
7 years, 5 months ago (2013-07-15 20:05:10 UTC) #11
Takashi Toyoshima
scherkus: I fixed this change to handle MIDIDispatcher::OnMessageReceived since it causes test failures. PTAL.
7 years, 5 months ago (2013-07-23 07:04:39 UTC) #12
scherkus (not reviewing)
lgtm w/ nit https://codereview.chromium.org/18611005/diff/57001/content/common/media/midi_messages.h File content/common/media/midi_messages.h (right): https://codereview.chromium.org/18611005/diff/57001/content/common/media/midi_messages.h#newcode38 content/common/media/midi_messages.h:38: remove extra blank line
7 years, 5 months ago (2013-07-23 16:47:16 UTC) #13
Takashi Toyoshima
Thanks! https://codereview.chromium.org/18611005/diff/57001/content/common/media/midi_messages.h File content/common/media/midi_messages.h (right): https://codereview.chromium.org/18611005/diff/57001/content/common/media/midi_messages.h#newcode38 content/common/media/midi_messages.h:38: On 2013/07/23 16:47:17, scherkus wrote: > remove extra ...
7 years, 5 months ago (2013-07-23 17:38:06 UTC) #14
Takashi Toyoshima
+palmer for IPC review Can you check content/common/media/midi_messages.h ?
7 years, 5 months ago (2013-07-23 17:39:45 UTC) #15
jochen (gone - plz use gerrit)
https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h File content/common/media/midi_messages.h (right): https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h#newcode27 content/common/media/midi_messages.h:27: IPC_MESSAGE_CONTROL3(MIDIHostMsg_RequestSysExPermission, where is this used? https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h#newcode28 content/common/media/midi_messages.h:28: int /* ...
7 years, 5 months ago (2013-07-23 19:09:21 UTC) #16
palmer
https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h File content/common/media/midi_messages.h (right): https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h#newcode30 content/common/media/midi_messages.h:30: std::string /* origin */) On 2013/07/23 19:09:21, jochen wrote: ...
7 years, 5 months ago (2013-07-24 00:24:00 UTC) #17
Takashi Toyoshima
https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h File content/common/media/midi_messages.h (right): https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h#newcode27 content/common/media/midi_messages.h:27: IPC_MESSAGE_CONTROL3(MIDIHostMsg_RequestSysExPermission, Not used in this change. I'll use it ...
7 years, 5 months ago (2013-07-24 01:42:56 UTC) #18
jochen (gone - plz use gerrit)
https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h File content/common/media/midi_messages.h (right): https://codereview.chromium.org/18611005/diff/77001/content/common/media/midi_messages.h#newcode27 content/common/media/midi_messages.h:27: IPC_MESSAGE_CONTROL3(MIDIHostMsg_RequestSysExPermission, On 2013/07/24 01:42:56, Takashi Toyoshima (chromium) wrote: > ...
7 years, 5 months ago (2013-07-24 11:35:36 UTC) #19
Takashi Toyoshima
I split new IPCs until Patch Set 7. But it makes some tests fail, or ...
7 years, 5 months ago (2013-07-24 11:50:52 UTC) #20
Takashi Toyoshima
OK, I made a Patch Set 12 where "not used yet" code including unused IPC ...
7 years, 5 months ago (2013-07-24 12:49:42 UTC) #21
jochen (gone - plz use gerrit)
lgtm
7 years, 5 months ago (2013-07-24 14:01:54 UTC) #22
Takashi Toyoshima
Thanks! I'll wait for the last review on IPC by Palmer for CQ.
7 years, 5 months ago (2013-07-24 14:30:41 UTC) #23
palmer
LGTM with nits. https://codereview.chromium.org/18611005/diff/95001/content/renderer/media/midi_dispatcher.h File content/renderer/media/midi_dispatcher.h (right): https://codereview.chromium.org/18611005/diff/95001/content/renderer/media/midi_dispatcher.h#newcode37 content/renderer/media/midi_dispatcher.h:37: const WebKit::WebMIDIPermissionRequest&) OVERRIDE; Nit: Use a ...
7 years, 5 months ago (2013-07-24 20:22:50 UTC) #24
Takashi Toyoshima
https://codereview.chromium.org/18611005/diff/95001/content/renderer/media/midi_dispatcher.h File content/renderer/media/midi_dispatcher.h (right): https://codereview.chromium.org/18611005/diff/95001/content/renderer/media/midi_dispatcher.h#newcode37 content/renderer/media/midi_dispatcher.h:37: const WebKit::WebMIDIPermissionRequest&) OVERRIDE; On 2013/07/24 20:22:51, Chromium Palmer wrote: ...
7 years, 5 months ago (2013-07-25 06:04:19 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/18611005/115001
7 years, 5 months ago (2013-07-25 06:06:47 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/18611005/115001
7 years, 5 months ago (2013-07-25 09:26:31 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/18611005/115001
7 years, 5 months ago (2013-07-25 09:42:58 UTC) #28
commit-bot: I haz the power
7 years, 5 months ago (2013-07-25 16:23:15 UTC) #29
Message was sent while issue was closed.
Change committed as 213644

Powered by Google App Engine
This is Rietveld 408576698