Cancel MIDI permission request infobar on MIDIAccess stop.

Cancel MIDI permission request infobar on MIDIAccess stop.

On WebCore::MIDIAccess::stop(), renderer didn't cancel the MIDI
permission request infobar. As a result, when an iframe webpage that
requested MIDI permission is navigated away, the MIDI infobar is not
dismissed, unlike the geolocation infobar. This CL makes the MIDI infobar
be dismissed on WebCore::MIDIAccess::stop().

BUG=309893
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231719

[Kibeom Kim (inactive), 2013-10-23 22:48:59]

Mostly parallel to the Geolocation infobar counterpart. Manually tested on Linux
by forcing flag. I'll try on Mac once I get the build environment working.

Added reviewers based on the previous CL.
https://chromiumcodereview.appspot.com/20990005

[Peter Kasting, 2013-10-23 22:50:46]

On 2013/10/23 22:48:59, Kibeom Kim wrote:
> Added reviewers based on the previous CL.
> https://chromiumcodereview.appspot.com/20990005

When you add multiple reviewers, please say what you want each to review.

[Kibeom Kim (inactive), 2013-10-23 23:13:19]

initial owner breakdown:
- palmer@ content/common/media/midi_messages.h
- jam@ content/public/browser/*
- jochen@ content/browser/renderer_host/render_process_host_impl.cc
- bulach@ content/browser/geolocation/geolocation_dispatcher_host.cc
- erg@ chrome/browser/profiles/* 
- scherkus@ chrome/browser/media/* content/browser/renderer_host/media/*
content/renderer/media/midi_dispatcher.cc

[Kibeom Kim (inactive), 2013-10-23 23:37:30]

https://codereview.chromium.org/38043004/diff/1/content/renderer/media/midi_d...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/38043004/diff/1/content/renderer/media/midi_d...
content/renderer/media/midi_dispatcher.cc:45: const WebMIDIPermissionRequest&
request) {
ddorwin@ or scherkus@ or xhwang@:
This is triggered by ExecutionContext::stopActiveDOMObjects(). In case of EME,
where should I request infobar cancellation?

[ddorwin, 2013-10-23 23:52:39]

https://codereview.chromium.org/38043004/diff/1/content/renderer/media/midi_d...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/38043004/diff/1/content/renderer/media/midi_d...
content/renderer/media/midi_dispatcher.cc:45: const WebMIDIPermissionRequest&
request) {
On 2013/10/23 23:37:31, Kibeom Kim wrote:
> ddorwin@ or scherkus@ or xhwang@:
> This is triggered by ExecutionContext::stopActiveDOMObjects(). In case of EME,
> where should I request infobar cancellation?

Can you provide more details on the exact path?

The currently shipping EME APIs go through HTMLMediaElement, which is also an
ActiveDOMObject with a stop() method [1]. stop() calls userCancelledLoad(),
which destroys the WebMediaPlayerImpl/WebMediaPlayerAndroid (Chromium side of
things). Should it go in the destructor of those classes? However, you could
have multiple <video> in a page, and destroying one shouldn't necessarily cancel
the infobar, so you'd need to know which one it was or somehow hook up to the
main stopActiveDOMObjects().

(The newer unprefixed APIs have a different path that we'll have to address
later.)

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Kibeom Kim (inactive), 2013-10-24 00:00:05]

https://codereview.chromium.org/38043004/diff/1/content/renderer/media/midi_d...
File content/renderer/media/midi_dispatcher.cc (right):

https://codereview.chromium.org/38043004/diff/1/content/renderer/media/midi_d...
content/renderer/media/midi_dispatcher.cc:45: const WebMIDIPermissionRequest&
request) {
On 2013/10/23 23:52:40, ddorwin wrote:
> ...
> Can you provide more details on the exact path?
> 

This is the call stack for MIDI.

Started by one of these four:
- WebCore::ExecutionContext::stopActiveDOMObjects() ->
WebCore::ContextLifecycleNotifier::notifyStoppingActiveDOMObjects()
- WebCore::MIDIAccess::~MIDIAccess()
- WebCore::MIDIAccessPromise::~MIDIAccessPromise()
- WebCore::MIDIPort::~MIDIPort()

Then goes:
WebCore::MIDIAccess::stop()
WebCore::MIDIController::cancelSysExPermissionRequest(MIDIAccess *)
WebKit::MIDIClientProxy::cancelSysExPermissionRequest(MIDIAccess *)
content::MIDIDispatcher::cancelSysExPermissionRequest(const
WebMIDIPermissionRequest &)

[Takashi Toyoshima, 2013-10-24 01:35:36]

Hi Kibeom, thank you for catching this problem.
lgtm as an original author.

By the way, this is just a question.
In this CL, you are going to introduce MIDIPermissionContext in content API
instead of just adding CancelMIDISysExPermission(). Do you have any reason?
In my first implementation, I introduced MIDIPermissionContext in the API
because I followed how geolocation works. But finally I removed it in the API
review because I can implement it without having context object in content side.
Actually, you don't have any additional information in chrome side context
implementation to cancel it. Arguments for CancelMIDISysExpermission, e.g.,
render_process_id, render_view_id, bridge_id, and requesting_frame, seem enough
to cancel the infobar.

Anyway, jam@ will review the API change in this CL, and show better idea.

Thanks.

[Kibeom Kim (inactive), 2013-10-24 02:12:12]

> In this CL, you are going to introduce MIDIPermissionContext in content API
> instead of just adding CancelMIDISysExPermission(). Do you have any reason?
> In my first implementation, I introduced MIDIPermissionContext in the API
> because I followed how geolocation works. But finally I removed it in the API
> review because I can implement it without having context object in content
side.

Actually, content::MidiPermissionContext API should be implemented by others
too, like, Android webview and content shell. I should have added those. I'll do
it in the following patch set.

[Bernhard Bauer, 2013-10-24 07:42:04]

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
chrome/browser/media/chrome_midi_permission_context.h:34: virtual void
CancelMIDISysExPermissionRequest(
Nit: at the beginning of a block of virtual methods, add a comment which class
they're from?

https://codereview.chromium.org/38043004/diff/1/content/public/browser/midi_p...
File content/public/browser/midi_permission_context.h (right):

https://codereview.chromium.org/38043004/diff/1/content/public/browser/midi_p...
content/public/browser/midi_permission_context.h:42: virtual
~MidiPermissionContext() {}
Nit: one less space

[bulach, 2013-10-24 13:06:56]

lgtm for content/browser/geolocation/geolocation_dispatcher_host.cc

btw, the change in that file was pure whitespace :)
whilst I appreciate the fix, your life would've been much easier with that file
on a patch on its own.

[jam, 2013-10-24 16:15:25]

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
chrome/browser/media/chrome_midi_permission_context.h:24: // Request to ask
users permission about MIDI.
nit: just put content::MidiPermissionContext implementation:, and don't
duplicate the method documentation from the interface

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
chrome/browser/media/chrome_midi_permission_context.h:31: OVERRIDE;
nit: override doesn't start a line per convention, also, this fits above fine

https://codereview.chromium.org/38043004/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/midi_dispatcher_host.cc (right):

https://codereview.chromium.org/38043004/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/midi_dispatcher_host.cc:42: if
(message.type() == MIDIHostMsg_RequestSysExPermission::ID)
if (IPC_MESSAGE_CLASS(msg) == MIDIMsgStart)
  *thread = BrowserThread::UI;

since the midi context interface is from BrowserContext (which is UI thread),
you should also make that interface be called on the UI thread. then you don't
need thread hopping in the chrome implementation

https://codereview.chromium.org/38043004/diff/1/content/public/browser/midi_p...
File content/public/browser/midi_permission_context.h (right):

https://codereview.chromium.org/38043004/diff/1/content/public/browser/midi_p...
content/public/browser/midi_permission_context.h:22: : public
base::RefCountedThreadSafe<MidiPermissionContext> {
does this have to be ref counted? please avoid ref counting where possible

[ddorwin, 2013-10-24 17:30:04]

On 2013/10/24 00:00:05, Kibeom Kim wrote:
>
https://codereview.chromium.org/38043004/diff/1/content/renderer/media/midi_d...
> File content/renderer/media/midi_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/38043004/diff/1/content/renderer/media/midi_d...
> content/renderer/media/midi_dispatcher.cc:45: const WebMIDIPermissionRequest&
> request) {
> On 2013/10/23 23:52:40, ddorwin wrote:
> > ...
> > Can you provide more details on the exact path?
> > 
> 
> This is the call stack for MIDI.
> 
> Started by one of these four:
> - WebCore::ExecutionContext::stopActiveDOMObjects() ->
> WebCore::ContextLifecycleNotifier::notifyStoppingActiveDOMObjects()
> - WebCore::MIDIAccess::~MIDIAccess()
> - WebCore::MIDIAccessPromise::~MIDIAccessPromise()
> - WebCore::MIDIPort::~MIDIPort()
> 
> Then goes:
> WebCore::MIDIAccess::stop()
> WebCore::MIDIController::cancelSysExPermissionRequest(MIDIAccess *)
> WebKit::MIDIClientProxy::cancelSysExPermissionRequest(MIDIAccess *)
> content::MIDIDispatcher::cancelSysExPermissionRequest(const
> WebMIDIPermissionRequest &)

I see, so there is explicit handling of the permissions request in Blink, which
is not the case for EME. We can probably still make it work along the same path
I described earlier; we just rely on the Chromium code to cancel rather than an
explicit cancel in Blink.

We'll also need to find a way to remember the identifier for the original
request so it can be canceled. Where is that identifier stored in the MIDI case?

[Elliot Glaysher, 2013-10-24 17:40:53]

profiles lgtm

[Kibeom Kim (inactive), 2013-10-24 17:49:23]

On 2013/10/24 17:30:04, ddorwin wrote:
> ...
> We'll also need to find a way to remember the identifier for the original
> request so it can be canceled. Where is that identifier stored in the MIDI
case?

In MIDI case, there is IDMap<WebMIDIPermissionRequest> in MIDIDispatcher . In
Geolocation case, it's WebGeolocationPermissionRequestManager class and
GeolocationDispatcher calls it. Personally, I prefer the MIDI way because it's
simpler, except that it's not a bidrectional map like GeolocationDispatcher, but
I guess it shouldn't matter in practice.

[Kibeom Kim (inactive), 2013-10-24 19:36:25]

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
chrome/browser/media/chrome_midi_permission_context.h:24: // Request to ask
users permission about MIDI.
On 2013/10/24 16:15:26, jam wrote:
> nit: just put content::MidiPermissionContext implementation:, and don't
> duplicate the method documentation from the interface

Done.

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
chrome/browser/media/chrome_midi_permission_context.h:31: OVERRIDE;
On 2013/10/24 16:15:26, jam wrote:
> nit: override doesn't start a line per convention, also, this fits above fine

Done.

https://codereview.chromium.org/38043004/diff/1/chrome/browser/media/chrome_m...
chrome/browser/media/chrome_midi_permission_context.h:34: virtual void
CancelMIDISysExPermissionRequest(
On 2013/10/24 07:42:04, Bernhard Bauer wrote:
> Nit: at the beginning of a block of virtual methods, add a comment which class
> they're from?

Done.

https://codereview.chromium.org/38043004/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/midi_dispatcher_host.cc (right):

https://codereview.chromium.org/38043004/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/midi_dispatcher_host.cc:42: if
(message.type() == MIDIHostMsg_RequestSysExPermission::ID)
On 2013/10/24 16:15:26, jam wrote:
> if (IPC_MESSAGE_CLASS(msg) == MIDIMsgStart)
>   *thread = BrowserThread::UI;
> 
> since the midi context interface is from BrowserContext (which is UI thread),
> you should also make that interface be called on the UI thread. then you don't
> need thread hopping in the chrome implementation

Done.

https://codereview.chromium.org/38043004/diff/1/content/public/browser/midi_p...
File content/public/browser/midi_permission_context.h (right):

https://codereview.chromium.org/38043004/diff/1/content/public/browser/midi_p...
content/public/browser/midi_permission_context.h:22: : public
base::RefCountedThreadSafe<MidiPermissionContext> {
On 2013/10/24 16:15:26, jam wrote:
> does this have to be ref counted? please avoid ref counting where possible

Done.

https://codereview.chromium.org/38043004/diff/1/content/public/browser/midi_p...
content/public/browser/midi_permission_context.h:42: virtual
~MidiPermissionContext() {}
On 2013/10/24 07:42:04, Bernhard Bauer wrote:
> Nit: one less space

Done.

[Kibeom Kim (inactive), 2013-10-24 20:42:33]

On 2013/10/24 02:12:12, Kibeom Kim wrote:
> > In this CL, you are going to introduce MIDIPermissionContext in content API
> > instead of just adding CancelMIDISysExPermission(). Do you have any reason?
> > In my first implementation, I introduced MIDIPermissionContext in the API
> > because I followed how geolocation works. But finally I removed it in the
API
> > review because I can implement it without having context object in content
> side.
> 
> Actually, content::MidiPermissionContext API should be implemented by others
> too, like, Android webview and content shell. I should have added those. I'll
do
> it in the following patch set.

Oh sorry, now I understand what you meant. I agree, I can't find any reason to
add MidiPermissionContext.

[Kibeom Kim (inactive), 2013-10-24 21:47:59]

PTAL.

Added BrowserContext::CancelMIDISysExPermissionRequest(...) instead of
MidiPermissionContext class. Additional modified files are all just stub
implementations of CancelMIDISysExPermissionRequest.

+joth for android_webview/browser/*

[joth, 2013-10-24 22:06:27]

aw/ lgtm rubberstamp

[jam, 2013-10-25 00:14:50]

content public lgtm

[jochen (gone - plz use gerrit), 2013-10-25 11:37:08]

https://codereview.chromium.org/38043004/diff/290001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/38043004/diff/290001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:199: const GURL&
requesting_frame) {
why do you need this empty method here?

[Kibeom Kim (inactive), 2013-10-28 09:43:17]

https://codereview.chromium.org/38043004/diff/290001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/38043004/diff/290001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.cc:199: const GURL&
requesting_frame) {
On 2013/10/25 11:37:09, jochen wrote:
> why do you need this empty method here?

Done. (Also moved other empty RequestMIDISysExPermission implementations to
headers)

[scherkus (not reviewing), 2013-10-28 20:48:53]

*/media/* lgtm w/ one nit

https://codereview.chromium.org/38043004/diff/450001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/38043004/diff/450001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:710: 
remove this extra blank line that seemed to sneak in

[Kibeom Kim (inactive), 2013-10-29 10:25:09]

palmer@ : Could you take a look at midi_messages.h? Thanks.

https://codereview.chromium.org/38043004/diff/450001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/38043004/diff/450001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:710: 
On 2013/10/28 20:48:54, scherkus wrote:
> remove this extra blank line that seemed to sneak in

Done.

[jochen (gone - plz use gerrit), 2013-10-29 14:17:05]

https://codereview.chromium.org/38043004/diff/680001/content/shell/browser/sh...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/38043004/diff/680001/content/shell/browser/sh...
content/shell/browser/shell_browser_context.h:56: const GURL& requesting_frame)
OVERRIDE {}
sorry for being not precise.

why do you override this method at all? The base class' implementation is
already empty, so you shouldn't need the method here at all.

[jochen (gone - plz use gerrit), 2013-10-29 19:23:52]

lgtm

[palmer, 2013-10-29 20:53:11]

lgtm

[commit-bot: I haz the power, 2013-10-29 21:59:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/38043004/1060001

[commit-bot: I haz the power, 2013-10-30 03:18:32]

Change committed as 231719