Web MIDI: implement permission infobar

Web MIDI: implement permission infobar

BUG=257618
TBR=jochen@chromium.org

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

[Takashi Toyoshima, 2013-07-31 14:58:49]

Hi Bernhard,

Can you take a look this CL?
This change handles infobar for Web MIDI. A screenshot for the infobar is
available in http://crbug.com/257618

Add scherkus@ as a OWNER for chrome/browser/media.
Because almost the entire code comes from content settings, so I think this
change should be reviewed by Bernhard firstly, and after that I'll ask scherkus
media OWNERS review again.

Thanks,

[Bernhard Bauer, 2013-07-31 15:35:39]

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
chrome/browser/content_settings/tab_specific_content_settings.cc:505: //
TODO(toyoshim): Bubble iconfor MIDI  is disabled for now.
Nit: "icon for MIDI"

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.cc:27:
DCHECK(!permission_queue_controller_.get());
You can leave out the .get(); scoped_ptr has an implicit conversion to bool.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.cc:35: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
I'm not a big fan of this construct. The calling code should know whether it's
on the UI thread or not, so it could call the right method which DCHECKs that.

Hmm, isn't this always called on the UI thread anyway?

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:18: // Chrome specific
implementation of MIDIPermissionContext; manages MIDI
What's MIDIPermissionContext? It's not a superclass of this class, and I can't
find it on codesearch.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:21: : public
base::RefCountedThreadSafe<ChromeMIDIPermissionContext> {
Why is this class refcounted thread safe?

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:35: virtual
~ChromeMIDIPermissionContext();
Is this class subclassed?

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:38: Profile* profile()
const { return profile_; }
Do you really need a private accessor? :)

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:42:
PermissionQueueController* QueueController();
Nit: GetQueueController() maybe?

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:71: // Create an
PermissionQueueController. overriden in derived classes to
Nit: "Overridden". Also, are these derived classes still forthcoming?

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:79: friend class
base::RefCountedThreadSafe<ChromeMIDIPermissionContext>;
Friend declarations come first in the private part.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context_factory.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context_factory.cc:14: class Service
: public BrowserContextKeyedService {
Instead of doing this, you could directly inherit from
RefcountedBrowserContextKeyedService.

[scherkus (not reviewing), 2013-07-31 18:09:55]

yeah I'm not really familiar with this area of the code, so I  have mostly nits
on comments 'n other stuff

I'll wait until bernhard wraps up the review

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
File chrome/browser/content_settings/permission_queue_controller.cc (left):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
chrome/browser/content_settings/permission_queue_controller.cc:94: //
TODO(toyoshim): Remove following dependency on geolocation.
is this TODO actually fixed?

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
chrome/browser/content_settings/tab_specific_content_settings.cc:513: //
TODO(toyoshim): Bubble iconfor MIDI  is disabled for now.
fix typos here

s/iconfor/icon for/
s/MIDI  is/MIDI is/

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
chrome/browser/content_settings/tab_specific_content_settings.cc:514: //
http://crbug.com/257618 .
remove trailing whitespace and period

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context_factory.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context_factory.h:26: //
|BrowserContextKeyedBaseFactory| methods:
nit: drop | | around references to classes -- AFAIK we only use that for
referencing variables

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/midi_permission_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/midi_permission_infobar_delegate.h:14: 
remove extra line

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/midi_permission_infobar_delegate.h:18: // TODO(toyoshim):
Much more code can be shared with GeolocationInfoBarDelegate.
do we have a bug for this?

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/midi_permission_infobar_delegate.h:51: // Call back to the
controller, to inform of the user's decision.
s/Call back/Callback/
s/controller, to/controller to/

[Takashi Toyoshima, 2013-08-01 07:43:09]

Upload Patch Set 5 containing changes based on comments #2 except for using
RefcountedBrowserContextKeyedService. I'll update again for
RefcountedBrowserContextKeyedService and #3

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
chrome/browser/content_settings/tab_specific_content_settings.cc:505: //
TODO(toyoshim): Bubble iconfor MIDI  is disabled for now.
On 2013/07/31 15:35:39, Bernhard Bauer wrote:
> Nit: "icon for MIDI"

Done.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.cc:27:
DCHECK(!permission_queue_controller_.get());
On 2013/07/31 15:35:39, Bernhard Bauer wrote:
> You can leave out the .get(); scoped_ptr has an implicit conversion to bool.

Done.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.cc:35: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
No. This function is always called from *IO* thread though the content API, and
here changed to UI thread.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:18: // Chrome specific
implementation of MIDIPermissionContext; manages MIDI
Oops. Sorry, this should be removed at cleanup.
Originally, I had a MIDIPermissionContext in the content API layer, and this
implements it.
But, I decided to omit it when I took a content API review.

Sorry for bothering you. This class still contains some remains of that.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:21: : public
base::RefCountedThreadSafe<ChromeMIDIPermissionContext> {
As we commented in the cc file, this class is used from IO thread and internally
works in UI thread.
It should be ref counted so that it works in both thread using PostTask.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:35: virtual
~ChromeMIDIPermissionContext();
Yes for RefCounted.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:38: Profile* profile()
const { return profile_; }
Good point. I remove this to use it directly :)

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:42:
PermissionQueueController* QueueController();
On 2013/07/31 15:35:39, Bernhard Bauer wrote:
> Nit: GetQueueController() maybe?

Done.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:71: // Create an
PermissionQueueController. overriden in derived classes to
Oops. This also may not happen now, so I'll remove the virtual.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:79: friend class
base::RefCountedThreadSafe<ChromeMIDIPermissionContext>;
On 2013/07/31 15:35:39, Bernhard Bauer wrote:
> Friend declarations come first in the private part.

Done.

[Bernhard Bauer, 2013-08-01 08:21:27]

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.cc:35: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
On 2013/08/01 07:43:09, Takashi Toyoshima (chromium) wrote:
> No. This function is always called from *IO* thread though the content API,
and
> here changed to UI thread.

Uhh... Isn't this called from
(OffTheRecord)ProfileImpl::RequestMIDISysExPermission()? BrowserContext lives on
the UI thread, so you'd either need to jump to the UI thread beforehand, or pass
the object to the IO thread. I can't find code calling
BrowserContext::RequestMIDISysExPermission() in codesearch, is that in a pending
CL somewhere? I'd be interested in finding out where you get the BrowserContext
pointer from.

(As an aside, this is why I'm not in favor of these catch-all thread hops: If
you document your assumptions with DCHECKs, you find out when they're not the
case :) ).

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:35: virtual
~ChromeMIDIPermissionContext();
On 2013/08/01 07:43:09, Takashi Toyoshima (chromium) wrote:
> Yes for RefCounted.

?

Oh, my question might have been worded ambiguously. What I meant to ask was, is
there a subclass of this class? If not, you can make the destructor private.

https://chromiumcodereview.appspot.com/20990005/diff/17001/chrome/browser/med...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/17001/chrome/browser/med...
chrome/browser/media/chrome_midi_permission_context.cc:37: LOG(ERROR) << "Thread
ID: " << tid;
Remove this log statement before committing.

[Takashi Toyoshima, 2013-08-01 08:27:38]

Upload Patch Set 6 based on review #3.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
File chrome/browser/content_settings/permission_queue_controller.cc (left):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
chrome/browser/content_settings/permission_queue_controller.cc:94: //
TODO(toyoshim): Remove following dependency on geolocation.
OK, I keep this TODO here and will handle later in another CL.
Maybe I can remove this by defining a delegate class for creating them.
Also, GeolocationInfoBar and MIDIPermissionInfoBar may be able to share more
code.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
chrome/browser/content_settings/tab_specific_content_settings.cc:513: //
TODO(toyoshim): Bubble iconfor MIDI  is disabled for now.
On 2013/07/31 18:09:55, scherkus wrote:
> fix typos here
> 
> s/iconfor/icon for/
> s/MIDI  is/MIDI is/

Done.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/cont...
chrome/browser/content_settings/tab_specific_content_settings.cc:514: //
http://crbug.com/257618 .
On 2013/07/31 18:09:55, scherkus wrote:
> remove trailing whitespace and period

Done.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context_factory.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context_factory.h:26: //
|BrowserContextKeyedBaseFactory| methods:
On 2013/07/31 18:09:55, scherkus wrote:
> nit: drop | | around references to classes -- AFAIK we only use that for
> referencing variables

Done.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/midi_permission_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/midi_permission_infobar_delegate.h:14: 
On 2013/07/31 18:09:55, scherkus wrote:
> remove extra line

Done.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/midi_permission_infobar_delegate.h:18: // TODO(toyoshim):
Much more code can be shared with GeolocationInfoBarDelegate.
On 2013/07/31 18:09:55, scherkus wrote:
> do we have a bug for this?

Done.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/midi_permission_infobar_delegate.h:51: // Call back to the
controller, to inform of the user's decision.
On 2013/07/31 18:09:55, scherkus wrote:
> s/Call back/Callback/
> s/controller, to/controller to/

Done.

[Takashi Toyoshima, 2013-08-01 09:29:45]

Patch Set 7.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.cc:35: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
The caller code is introduced in this change.
Please see the diff of profile_impl.cc and off_the_record_profile_impl.cc.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.h:35: virtual
~ChromeMIDIPermissionContext();
Ah, I see. I misunderstood your question.
Done.

https://chromiumcodereview.appspot.com/20990005/diff/17001/chrome/browser/med...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/17001/chrome/browser/med...
chrome/browser/media/chrome_midi_permission_context.cc:37: LOG(ERROR) << "Thread
ID: " << tid;
Oops. Sorry, I added this to confirm the thread id and forgot to remove :)
Yep, this LOG shows 6 (BrowserThread::IO), and 0 (BrowserThread::UI).

Removed.

[Bernhard Bauer, 2013-08-01 11:14:02]

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context.cc:35: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
On 2013/08/01 09:29:45, Takashi Toyoshima (chromium) wrote:
> The caller code is introduced in this change.
> Please see the diff of profile_impl.cc and off_the_record_profile_impl.cc.

Yes, but who calls BrowserContext::RequestMIDISysExPermission() (which is the
method overridden in ProfileImpl and OffTheRecordProfileImpl)?

https://chromiumcodereview.appspot.com/20990005/diff/29001/chrome/browser/med...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/29001/chrome/browser/med...
chrome/browser/media/chrome_midi_permission_context.cc:36:
content::BrowserThread::GetCurrentThreadIdentifier(&tid);
This isn't necessary now either.

[Takashi Toyoshima, 2013-08-01 11:19:24]

> Yes, but who calls BrowserContext::RequestMIDISysExPermission() (which is the
> method overridden in ProfileImpl and OffTheRecordProfileImpl)?

from content/browser/renderer_host/media/midi_dispatcher_host.cc
In this dispatcher host, the content API is called when it is requested via IPC.

[Bernhard Bauer, 2013-08-01 11:38:12]

On Thu, Aug 1, 2013 at 1:19 PM, <toyoshim@chromium.org> wrote:

> Yes, but who calls BrowserContext::**RequestMIDISysExPermission() (which
>> is the
>> method overridden in ProfileImpl and OffTheRecordProfileImpl)?
>>
>
> from content/browser/renderer_host/**media/midi_dispatcher_host.cc
> In this dispatcher host, the content API is called when it is requested
> via IPC.
>

Ah, thanks!

So, using BrowserContext on the IO thread like that is wrong. The contract
for the class says that it should only be used on the UI thread, and indeed
you don't even have a guarantee that the object is alive at this point!
Likewise, getting a BrowserContextKeyedService on a non-UI thread is wrong,
for the same reason (I'm frankly surprised you don't run into a DCHECK
somewhere).

The easiest thing to do would probably be to make the MIDIDispatcherHost
receive the message on the UI thread already. You do that by overriding
BrowserMessageFilter::OverrideThreadForMessage. Then you don't need to jump
threads yourself at all, so the ChromeMIDIPermissionContext doesn't need to
be refcounted.


>
https://chromiumcodereview.**appspot.com/20990005/<https://chromiumcodereview...
>

[Takashi Toyoshima, 2013-08-01 11:43:16]

Thank you for great advice.
I try it.

[Takashi Toyoshima, 2013-08-01 15:08:12]

Thanks!
Please take another look again.

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
File chrome/browser/media/chrome_midi_permission_context_factory.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/5001/chrome/browser/medi...
chrome/browser/media/chrome_midi_permission_context_factory.cc:14: class Service
: public BrowserContextKeyedService {
On 2013/07/31 15:35:39, Bernhard Bauer wrote:
> Instead of doing this, you could directly inherit from
> RefcountedBrowserContextKeyedService.

Done.

[Bernhard Bauer, 2013-08-01 15:28:41]

https://chromiumcodereview.appspot.com/20990005/diff/47001/content/browser/re...
File content/browser/renderer_host/media/midi_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/47001/content/browser/re...
content/browser/renderer_host/media/midi_dispatcher_host.cc:61:
RenderViewHostImpl::FromID(render_process_id_, render_view_id);
I don't think getting the RenderViewHostImpl is necessary here; you could just
directly call Send().

[Takashi Toyoshima, 2013-08-01 15:36:25]

https://chromiumcodereview.appspot.com/20990005/diff/47001/content/browser/re...
File content/browser/renderer_host/media/midi_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/47001/content/browser/re...
content/browser/renderer_host/media/midi_dispatcher_host.cc:61:
RenderViewHostImpl::FromID(render_process_id_, render_view_id);
I did like this because the message couldn't be routed to the right renderer
when I call Send().

[Bernhard Bauer, 2013-08-01 22:24:32]

LGTM because the remaining issue might just be me being confused, and if it's
not, we can still do it in a follow-up.

https://chromiumcodereview.appspot.com/20990005/diff/47001/content/browser/re...
File content/browser/renderer_host/media/midi_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/47001/content/browser/re...
content/browser/renderer_host/media/midi_dispatcher_host.cc:61:
RenderViewHostImpl::FromID(render_process_id_, render_view_id);
On 2013/08/01 15:36:26, Takashi Toyoshima (chromium) wrote:
> I did like this because the message couldn't be routed to the right renderer
> when I call Send().

As in, to the right render process? I thought this object belongs to a single
process (which is the one identified by render_process_id_)?

[scherkus (not reviewing), 2013-08-01 22:44:46]

lgtm

[Takashi Toyoshima, 2013-08-02 04:20:20]

Thanks.
I try to check Send() code separately with reviewers for the area later.

[commit-bot: I haz the power, 2013-08-02 04:21:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/20990005/47001

[commit-bot: I haz the power, 2013-08-02 04:35:10]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Takashi Toyoshima, 2013-08-02 05:51:08]

Add TBR=jochen for trivial changes on geolocation and profiles.

[commit-bot: I haz the power, 2013-08-02 05:52:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/20990005/47001

[Peter Kasting, 2013-08-02 06:00:57]

Drive-by

https://chromiumcodereview.appspot.com/20990005/diff/47001/chrome/browser/med...
File chrome/browser/media/midi_permission_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/47001/chrome/browser/med...
chrome/browser/media/midi_permission_infobar_delegate.cc:71:
net::FormatUrl(requesting_frame_.GetOrigin(), display_languages_));
Nit: All lines of args must be aligned (i.e. move
IDS_MIDI_SYSEX_INFOBAR_QUESTION to its own line)

https://chromiumcodereview.appspot.com/20990005/diff/47001/chrome/browser/med...
chrome/browser/media/midi_permission_infobar_delegate.cc:90: void
MIDIPermissionInfoBarDelegate::SetPermission(bool update_content_setting,
Why does this take a first bool if it's always true?

https://chromiumcodereview.appspot.com/20990005/diff/47001/chrome/browser/med...
File chrome/browser/media/midi_permission_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/20990005/diff/47001/chrome/browser/med...
chrome/browser/media/midi_permission_infobar_delegate.h:25: // Creates a infobar
delegate and adds it to |infobar_service|.
Nit: Grammar; say "a MIDI permission infobar delegate"

[Takashi Toyoshima, 2013-08-02 07:22:23]

Hi Peter,

I'll fix them in the next CL including GeolocationInfoBarDelegate.
I'll ask you a review later.

Thanks!

https://chromiumcodereview.appspot.com/20990005/diff/47001/chrome/browser/med...
File chrome/browser/media/midi_permission_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/20990005/diff/47001/chrome/browser/med...
chrome/browser/media/midi_permission_infobar_delegate.cc:90: void
MIDIPermissionInfoBarDelegate::SetPermission(bool update_content_setting,
Agreed...
GeolocationInfoBarDelegate has the same problem.
Maybe we should call it with (false, false) when the infobar is closed without
pushing ack and nack.

[commit-bot: I haz the power, 2013-08-02 08:42:39]

List of reviewers changed. pkasting@chromium.org did a drive-by without
LGTM'ing!

[commit-bot: I haz the power, 2013-08-02 09:00:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/20990005/47001

[commit-bot: I haz the power, 2013-08-02 09:06:47]

Change committed as 215257