[MIDI] Add a permission bubble client for MIDI system control.

[MIDI] Add a permission bubble client for MIDI system control.

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

[Ami GONE FROM CHROMIUM, 2014-02-06 19:51:17]

I don't know anything about Chrome MIDI; you should get a review from a MIDI
author.

https://codereview.chromium.org/156173002/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/156173002/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13296: +      fully control your MIDI
devices.
I don't know grd but this doesn't look right.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:44:
ChromeMidiPermissionContext* context_;
This is a raw pointer; what guarantees it remains alive for the lifetime of this
object?

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:47: int
contents_unique_id_;
unused
(why didn't your compiler tell you this?)

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:83: return
l10n_util::GetStringUTF16(IDS_MIDI_SYSEX_DENY_BUTTON);
This seems pretty stock.  I wonder if PermissionBubbleDelegate ought to have a
ctor that takes these strings and implements these methods so each impl doesn't
have to repeat them.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:187: if
(PermissionBubbleManager::Enabled()) {
Can any code be deleted once this is always-on?
(please TODOify ifyes)

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:248: // TODO(gbillock):
Add support for cancellation to permission bubbles.
What is the usability impact of this lack?

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.h:44: // Called when the
permission decision is made. It may be by the
s/be by/be called by/ ?

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.h:45: // InfoBarDelegate to
notify permission has been set.
I'm no mind reader but I'm pretty sure you can rewrite this comment to be either
more informative or less verbose :)
(I realize the comment predates your CL)

[Greg Billock, 2014-02-07 18:20:55]

https://codereview.chromium.org/156173002/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/156173002/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13296: +      fully control your MIDI
devices.
On 2014/02/06 19:51:17, Ami Fischman wrote:
> I don't know grd but this doesn't look right.

In what sense?

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:44:
ChromeMidiPermissionContext* context_;
On 2014/02/06 19:51:17, Ami Fischman wrote:
> This is a raw pointer; what guarantees it remains alive for the lifetime of
this
> object?

The permission context is a profile keyed service, which will outlive any
windows (and tabs) associated with that profile. The infobar gets created with
an unretained raw pointer as well.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:47: int
contents_unique_id_;
On 2014/02/06 19:51:17, Ami Fischman wrote:
> unused
> (why didn't your compiler tell you this?)

not sure. Removing.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:83: return
l10n_util::GetStringUTF16(IDS_MIDI_SYSEX_DENY_BUTTON);
On 2014/02/06 19:51:17, Ami Fischman wrote:
> This seems pretty stock.  I wonder if PermissionBubbleDelegate ought to have a
> ctor that takes these strings and implements these methods so each impl
doesn't
> have to repeat them.

That's an option. The interface is pure right now, which is always nice... The
strings can be request-context-sensitive, but so far I haven't seen any that
aren't known at construct-time.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:187: if
(PermissionBubbleManager::Enabled()) {
On 2014/02/06 19:51:17, Ami Fischman wrote:
> Can any code be deleted once this is always-on?
> (please TODOify ifyes)

Will do. I have a bug tracking this.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:248: // TODO(gbillock):
Add support for cancellation to permission bubbles.
On 2014/02/06 19:51:17, Ami Fischman wrote:
> What is the usability impact of this lack?

Still investigating, but looks minimal, and will get fixed before the flag gets
flipped on.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
File chrome/browser/media/chrome_midi_permission_context.h (right):

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.h:44: // Called when the
permission decision is made. It may be by the
On 2014/02/06 19:51:17, Ami Fischman wrote:
> s/be by/be called by/ ?

reworded.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.h:45: // InfoBarDelegate to
notify permission has been set.
On 2014/02/06 19:51:17, Ami Fischman wrote:
> I'm no mind reader but I'm pretty sure you can rewrite this comment to be
either
> more informative or less verbose :)
> (I realize the comment predates your CL)

:-) yes. clarified.

[Ami GONE FROM CHROMIUM, 2014-02-07 18:25:34]

LGTM % nits & a review from someone who knows the MIDI code.

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:83: return
l10n_util::GetStringUTF16(IDS_MIDI_SYSEX_DENY_BUTTON);
On 2014/02/07 18:20:55, Greg Billock wrote:
> On 2014/02/06 19:51:17, Ami Fischman wrote:
> > This seems pretty stock.  I wonder if PermissionBubbleDelegate ought to have
a
> > ctor that takes these strings and implements these methods so each impl
> doesn't
> > have to repeat them.
> 
> That's an option. The interface is pure right now, which is always nice...

Yeah, nice for job security for the guy who re-implements the pure interface
every week! :-P

https://codereview.chromium.org/156173002/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/156173002/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13317: +      fully control your MIDI
devices.
Other entries look like
<message name="..." desc="...">
  Some text
</message>

but your entry looks like
<message name="..." desc="...">
  continuation of desc from above
</message>

https://codereview.chromium.org/156173002/diff/120001/chrome/browser/media/ch...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://codereview.chromium.org/156173002/diff/120001/chrome/browser/media/ch...
chrome/browser/media/chrome_midi_permission_context.cc:249: // TODO(gbillock):
Add support for cancellation to permission bubbles.
On 2014/02/07 18:20:55, Greg Billock wrote:
> On 2014/02/06 19:51:17, Ami Fischman wrote:
> > What is the usability impact of this lack?
> 
> Still investigating, but looks minimal, and will get fixed before the flag
gets
> flipped on.

Last clause makes me think there should be a comment near the flag (listing flip
blocker locations).

[Greg Billock, 2014-02-10 23:57:09]

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://codereview.chromium.org/156173002/diff/60001/chrome/browser/media/chr...
chrome/browser/media/chrome_midi_permission_context.cc:83: return
l10n_util::GetStringUTF16(IDS_MIDI_SYSEX_DENY_BUTTON);
On 2014/02/07 18:25:34, Ami Fischman wrote:
> On 2014/02/07 18:20:55, Greg Billock wrote:
> > On 2014/02/06 19:51:17, Ami Fischman wrote:
> > > This seems pretty stock.  I wonder if PermissionBubbleDelegate ought to
have
> a
> > > ctor that takes these strings and implements these methods so each impl
> > doesn't
> > > have to repeat them.
> > 
> > That's an option. The interface is pure right now, which is always nice...
> 
> Yeah, nice for job security for the guy who re-implements the pure interface
> every week! :-P

:-) We have eight of them. I guess that's perhaps cumulative 5 minutes of coding
time difference, but I'm not sure which direction... <troll :-)>

https://codereview.chromium.org/156173002/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/156173002/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13317: +      fully control your MIDI
devices.
On 2014/02/07 18:25:35, Ami Fischman wrote:
> Other entries look like
> <message name="..." desc="...">
>   Some text
> </message>
> 
> but your entry looks like
> <message name="..." desc="...">
>   continuation of desc from above
> </message>

I think this is just clipping in the UI. The syntax in the file is fine, and it
compiles OK.

https://codereview.chromium.org/156173002/diff/120001/chrome/browser/media/ch...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://codereview.chromium.org/156173002/diff/120001/chrome/browser/media/ch...
chrome/browser/media/chrome_midi_permission_context.cc:249: // TODO(gbillock):
Add support for cancellation to permission bubbles.
On 2014/02/07 18:25:35, Ami Fischman wrote:
> On 2014/02/07 18:20:55, Greg Billock wrote:
> > On 2014/02/06 19:51:17, Ami Fischman wrote:
> > > What is the usability impact of this lack?
> > 
> > Still investigating, but looks minimal, and will get fixed before the flag
> gets
> > flipped on.
> 
> Last clause makes me think there should be a comment near the flag (listing
flip
> blocker locations).

There's a fair amount of work left before flag flip, so I've added a bug to
track this cancel requirement.

[Takashi Toyoshima, 2014-02-12 07:29:37]

Thank you for working on this.
I'm not sure how this new grd text, IDS_MIDI_SYSEX_PERMISSION_FRAGMENT, are
shown.
But as far as I checked other IDS_*_PERMISSION_FRAGMENT entries, it looks
aligned with others.

Can you share a screenshot, or let me know a flag to enable this new UI?
I'll check description later.

Anyway, LGTM for now.

[Greg Billock, 2014-02-12 18:19:48]

Thanks. Here's a screenshot, which doesn't show this prompt, but other
parallel fragments.

Also, this API is almost certain to evolve a bit. The latest thinking is
that it probably will not use the existing prompt at all, and that the
fragment will be something more like "MIDI devices".


On Tue, Feb 11, 2014 at 11:29 PM, <toyoshim@chromium.org> wrote:

> Thank you for working on this.
> I'm not sure how this new grd text, IDS_MIDI_SYSEX_PERMISSION_FRAGMENT,
> are
> shown.
> But as far as I checked other IDS_*_PERMISSION_FRAGMENT entries, it looks
> aligned with others.
>
> Can you share a screenshot, or let me know a flag to enable this new UI?
> I'll check description later.
>
> Anyway, LGTM for now.
>
> https://codereview.chromium.org/156173002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Greg Billock, 2014-02-12 18:19:53]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-02-12 18:21:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/156173002/180001

[commit-bot: I haz the power, 2014-02-12 22:56:52]

Change committed as 250820