Implement midi permissions on top of the new common permission classes

Implement midi permissions on top of the new common permission classes

- move all the generic permission settings from gcm to content_settings
- delete the midi implementation in favour of this one.

Some important changes in midi.

Clicking Yes/No on Bubbles did not save the permission for future uses,
while when using infobars it was saved. Now it is saved in both cases

The existing midi implementation had some renderer crashes that have now
been fixed. It also had a browser race when tring to run the result
callback. Both issues are fixed in this implementation.

The midi cancel closure is not implemented, it does not seem to be used
anywhere and the paradigm was a bit convoluted
(passing it as an out pointer in RequestMidiSysExPermission)

The old midi infobar used to say Allow/Deny but the bubble said Allow/Block,
now it says Allow/Block in both cases.

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

[Miguel Garcia, 2014-07-03 18:10:54]

Initial non owner review

[Miguel Garcia, 2014-07-04 09:21:41]

Bernhard can you have a quick look a the content changes here? That way I can
get this LGTM out of the way quickly

[Takashi Toyoshima, 2014-07-04 09:37:56]

lgtm for midi
Thanks!

[Bernhard Bauer, 2014-07-04 10:24:13]

Mostly formatting nits:

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_bubble_request_impl.cc:37: icon_id =
IDR_ALLOWED_MIDI_SYSEX;
Directly return the icon ID?

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_bubble_request_impl.cc:63:
message_id,
Align four spaces.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:70: break;
Could you return here, so we can move the ASK case out of this switch statement?

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:91:
base::Bind(&PermissionContextBase::CleanUpBubble,
This should be aligned with the previous base::Bind.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:144:
UpdateContentSetting(requesting_origin, embedder_origin, allowed);
Nit: braces are unnecessary for single-line bodies.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:152:
base::ScopedPtrHashMap<std::string, PermissionBubbleRequest>::iterator it;
I would probably simplify this to:

  bool success = pending_bubbles_.erase(id.ToString());
  DCHECK(success);

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:31: //      future
uses (for the domain that requested it).
Mini-nit: the alignment for continuing indented paragraphs is off. I would
suggest indenting two spaces with regards to the dash here and below (i.e.
removing one space here).

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:33: //   - this one
request
No dash

[Michael van Ouwerkerk, 2014-07-04 10:34:32]

Nice!

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:49:
web_contents->GetURL().GetOrigin(),
Use GetLastCommittedURL as GetURL is deprecated and does not do what you want!

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:169:
ContentSettingsPattern::FromURLNoWildcard(requesting_origin.GetOrigin()),
origin.GetOrigin() looks redundant. Is any code passing in something other than
the actual origin? Maybe add a DCHECK for origin.spec() !=
origin.GetOrigin().spec() at the entry point.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_infobar_delegate.cc:51:
InfoBarService::WebContentsFromInfoBar(infobar())->GetURL().GetOrigin(),
Also GetLastCommittedURL here.

[Miguel Garcia, 2014-07-04 12:09:53]

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_bubble_request_impl.cc:37: icon_id =
IDR_ALLOWED_MIDI_SYSEX;
On 2014/07/04 10:24:13, Bernhard Bauer wrote:
> Directly return the icon ID?

I did it this way to be consistent with the other three methods since everyone
adding a new permission will need to copy paste it..

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_bubble_request_impl.cc:63:
message_id,
On 2014/07/04 10:24:13, Bernhard Bauer wrote:
> Align four spaces.

Done.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:49:
web_contents->GetURL().GetOrigin(),
On 2014/07/04 10:34:32, Michael van Ouwerkerk wrote:
> Use GetLastCommittedURL as GetURL is deprecated and does not do what you want!

True, you pointed this out before, thanks for spotting it!

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:70: break;
On 2014/07/04 10:24:13, Bernhard Bauer wrote:
> Could you return here, so we can move the ASK case out of this switch
statement?

Done.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:91:
base::Bind(&PermissionContextBase::CleanUpBubble,
On 2014/07/04 10:24:13, Bernhard Bauer wrote:
> This should be aligned with the previous base::Bind.

Done.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:144:
UpdateContentSetting(requesting_origin, embedder_origin, allowed);
On 2014/07/04 10:24:13, Bernhard Bauer wrote:
> Nit: braces are unnecessary for single-line bodies.

Done.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:152:
base::ScopedPtrHashMap<std::string, PermissionBubbleRequest>::iterator it;
On 2014/07/04 10:24:13, Bernhard Bauer wrote:
> I would probably simplify this to:
> 
>   bool success = pending_bubbles_.erase(id.ToString());
>   DCHECK(success);

Good idea, done.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:169:
ContentSettingsPattern::FromURLNoWildcard(requesting_origin.GetOrigin()),
On 2014/07/04 10:34:32, Michael van Ouwerkerk wrote:
> origin.GetOrigin() looks redundant. Is any code passing in something other
than
> the actual origin? Maybe add a DCHECK for origin.spec() !=
> origin.GetOrigin().spec() at the entry point.

Good idea,added the DCHECK

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:31: //      future
uses (for the domain that requested it).
On 2014/07/04 10:24:13, Bernhard Bauer wrote:
> Mini-nit: the alignment for continuing indented paragraphs is off. I would
> suggest indenting two spaces with regards to the dash here and below (i.e.
> removing one space here).

Done.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:33: //   - this one
request
On 2014/07/04 10:24:13, Bernhard Bauer wrote:
> No dash

Done.

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/365123003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_infobar_delegate.cc:51:
InfoBarService::WebContentsFromInfoBar(infobar())->GetURL().GetOrigin(),
On 2014/07/04 10:34:32, Michael van Ouwerkerk wrote:
> Also GetLastCommittedURL here.

Done.

[Miguel Garcia, 2014-07-04 12:10:40]

+fgorski for the gcm changes

[Michael van Ouwerkerk, 2014-07-04 12:18:26]

lgtm with nit

https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:162:
DCHECK(requesting_origin == requesting_origin.GetOrigin());
Sorry I made an inaccurate suggestion, we should probably use DCHECK_EQ:
DCHECK_EQ(requesting_origin, requesting_origin.GetOrigin());

[Miguel Garcia, 2014-07-04 12:24:03]

On 2014/07/04 12:18:26, Michael van Ouwerkerk wrote:
> lgtm with nit
> 
>
https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
> File chrome/browser/content_settings/permission_context_base.cc (right):
> 
>
https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
> chrome/browser/content_settings/permission_context_base.cc:162:
> DCHECK(requesting_origin == requesting_origin.GetOrigin());
> Sorry I made an inaccurate suggestion, we should probably use DCHECK_EQ:
> DCHECK_EQ(requesting_origin, requesting_origin.GetOrigin());

It does not matter match since GURL implements equals but ok.

[Miguel Garcia, 2014-07-04 12:27:24]

+dale for media owners

[Bernhard Bauer, 2014-07-04 12:34:51]

content_settings/ LGTM

On 2014/07/04 12:24:03, Miguel Garcia wrote:
> On 2014/07/04 12:18:26, Michael van Ouwerkerk wrote:
> > lgtm with nit
> > 
> >
>
https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
> > File chrome/browser/content_settings/permission_context_base.cc (right):
> > 
> >
>
https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
> > chrome/browser/content_settings/permission_context_base.cc:162:
> > DCHECK(requesting_origin == requesting_origin.GetOrigin());
> > Sorry I made an inaccurate suggestion, we should probably use DCHECK_EQ:
> > DCHECK_EQ(requesting_origin, requesting_origin.GetOrigin());
> 
> It does not matter match since GURL implements equals but ok.

The difference is that DCHECK_EQ will give you a nicer error message if the
check fails ("expected <foo>, got <bar>").

https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:39: //   new
permission.
Nit: Missed this one.

[fgorski, 2014-07-07 18:19:00]

gcm/ lgtm

[Miguel Garcia, 2014-07-08 10:37:32]

Tommi, would you mind having a look? I need a rubber stamp lgtm for media and
dale seems to be OOO.

https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/365123003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:39: //   new
permission.
On 2014/07/04 12:34:51, Bernhard Bauer wrote:
> Nit: Missed this one.

Done.

[tommi (sloooow) - chröme, 2014-07-08 12:38:55]

only took a look at */media/* and those lgtm assuming the nits below will be
fixed. (great to see progress on midi btw!)

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
File chrome/browser/media/midi_permission_context.cc (right):

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context.cc:12: :
PermissionContextBase(profile, CONTENT_SETTINGS_TYPE_MIDI_SYSEX){
nit: 4 spaces

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context.cc:20: const GURL&
requesting_frame,
fix indent

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context.cc:24: id.render_view_id());
indent off

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context.cc:27:
content_settings->OnMidiSysExAccessed(requesting_frame);
indent

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
File chrome/browser/media/midi_permission_infobar_delegate.h (right):

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_infobar_delegate.h:21: virtual
~MidiPermissionInfoBarDelegate();
document why the dtor now needs to be public?

[Miguel Garcia, 2014-07-08 13:54:30]

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
File chrome/browser/media/midi_permission_context.cc (right):

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context.cc:12: :
PermissionContextBase(profile, CONTENT_SETTINGS_TYPE_MIDI_SYSEX){
On 2014/07/08 12:38:54, tommi wrote:
> nit: 4 spaces

Done.

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context.cc:20: const GURL&
requesting_frame,
On 2014/07/08 12:38:54, tommi wrote:
> fix indent

Done.

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context.cc:24: id.render_view_id());
On 2014/07/08 12:38:54, tommi wrote:
> indent off

Done.

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_context.cc:27:
content_settings->OnMidiSysExAccessed(requesting_frame);
On 2014/07/08 12:38:54, tommi wrote:
> indent

Done.

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
File chrome/browser/media/midi_permission_infobar_delegate.h (right):

https://codereview.chromium.org/365123003/diff/100001/chrome/browser/media/mi...
chrome/browser/media/midi_permission_infobar_delegate.h:21: virtual
~MidiPermissionInfoBarDelegate();
On 2014/07/08 12:38:54, tommi wrote:
> document why the dtor now needs to be public?

It actually does not.

[Miguel Garcia, 2014-07-08 13:54:36]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-07-08 13:55:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/365123003/120001

[Miguel Garcia, 2014-07-08 15:19:04]

The CQ bit was unchecked by miguelg@chromium.org

[Miguel Garcia, 2014-07-08 15:19:25]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-07-08 15:19:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/365123003/140001

[commit-bot: I haz the power, 2014-07-08 20:07:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-08 23:14:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 23:14:44]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[Miguel Garcia, 2014-07-09 08:55:39]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-07-09 08:56:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/365123003/160001

[commit-bot: I haz the power, 2014-07-09 10:55:53]

Change committed as 282003