Stricter default RequestMediaAccessPermission and CheckMediaAccessPermission implementations

Stricter default RequestMediaAccessPermission and CheckMediaAccessPermission implementations.

We should not end up in the default implementation and want to track if this happens.

* Add new media stream result, "not supported".
* Add an error log in RenderFrameHostDelegate and WebContentsDelegate default implementations of the above functions and in Request... also use the new result.

BUG=416233
NOTRY=true

Committed: https://crrev.com/dea7b93642a46bdaaecad4a438bba1bfc8cc8516
Cr-Commit-Position: refs/heads/master@{#296779}

[Henrik Grunell, 2014-09-20 11:57:25]

grunell@chromium.org changed reviewers:
+ jam@chromium.org, tommi@chromium.org

[Henrik Grunell, 2014-09-20 12:10:08]

jam@ as owner.
tommi@ as media request/check knowledgeable.

The idea behind this change is the assumption that if a  subclass that doesn't
implement the functions, we don't expect them to be called in that context. Now,
that might be wrong and if so let's discuss further.

For RenderFrameHostDelegate, the only class not implementing these functions is
InterstitialPage, I don't expect them being called in that context.

For WebContentsDelegate, there's more. I could find:

chrome/browser/chromeos/first_run/first_run_view.h
chrome/browser/chromeos/login/ui/simple_web_view_dialog.h
chrome/browser/devtools/devtools_window.h
chrome/browser/tab_contents/background_contents.h
chrome/browser/ui/autofill/autofill_dialog_sign_in_delegate.h
chrome/browser/ui/panels/panel_host.h
chrome/browser/ui/webui/chrome_web_contents_handler.h
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h
chrome/browser/ui/webui/signin/inline_login_handler_impl.h
chromecast/shell/browser/android/cast_window_android.h
components/dom_distiller/content/distiller_page_web_contents.h
components/web_contents_delegate_android/web_contents_delegate_android.h
content/shell/browser/shell.h
extensions/browser/guest_view/guest_view_base.h
ui/views/controls/webview/web_dialog_view.h
ui/views/controls/webview/webview.h
ui/web_dialogs/test/test_web_contents_handler.h
ui/web_dialogs/web_dialog_web_contents_delegate.h

Here I'd like some advice - I'm not familiar with many of these. Is there any of
these I should be careful with? (BTW, guest_view_base.h and maybe others seems
to have subclasses that may implement the functions.)

[tommi (sloooow) - chröme, 2014-09-20 14:25:38]

https://codereview.chromium.org/584383002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_delegate.cc (right):

https://codereview.chromium.org/584383002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_delegate.cc:42: NOTREACHED();
I'm not sure about this since there is an implementation here already.

If these methods are not expected to be called, can we skip implementing them
altogether and force subclasses to make this decision (implement vs
NOTREACHED())?

https://codereview.chromium.org/584383002/diff/20001/content/public/browser/w...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/584383002/diff/20001/content/public/browser/w...
content/public/browser/web_contents_delegate.h:441: // Delegates that expect
this to be ever called, must implement the function.
I'd prefer pure virtual here and below.

[Henrik Grunell, 2014-09-22 08:49:52]

Wow, that was a lot of classes inheriting from WebContentsDelegate. Given the
large amount, I'm not so sure anymore we should abandon the default
implementation. However, it's in place now, so either way is fine with me.

https://codereview.chromium.org/584383002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_delegate.cc (right):

https://codereview.chromium.org/584383002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_delegate.cc:42: NOTREACHED();
On 2014/09/20 14:25:38, tommi wrote:
> I'm not sure about this since there is an implementation here already.
> 
> If these methods are not expected to be called, can we skip implementing them
> altogether and force subclasses to make this decision (implement vs
> NOTREACHED())?

Yeah, Per and I discussed this before. I had that standpoint at first, then we
kind of came to the conclusion to do it here. Changed.

https://codereview.chromium.org/584383002/diff/20001/content/public/browser/w...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/584383002/diff/20001/content/public/browser/w...
content/public/browser/web_contents_delegate.h:441: // Delegates that expect
this to be ever called, must implement the function.
On 2014/09/20 14:25:38, tommi wrote:
> I'd prefer pure virtual here and below.

Done.

[jam, 2014-09-23 05:06:40]

this runs counter to the content api guidelines:
http://www.chromium.org/developers/content-module/content-api
which is that interfaces implemented by the embedder should have empty
implementations so that an embedder only implements what they need initially to
make it easy.

[tommi (sloooow) - chröme, 2014-09-23 08:45:43]

Sounds like you're saying that there are two things going against the
guidelines:
- pure virtual methods.
- non-empty implementations.

How would you prefer this to be done?

On Tue, Sep 23, 2014 at 7:06 AM, <jam@chromium.org> wrote:

> this runs counter to the content api guidelines:
> http://www.chromium.org/developers/content-module/content-api
> which is that interfaces implemented by the embedder should have empty
> implementations so that an embedder only implements what they need
> initially to
> make it easy.
>
> https://codereview.chromium.org/584383002/
>

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

[Henrik Grunell, 2014-09-23 13:05:09]

Hmm, it's clearly against the guidelines to have WebContentsDelegate functions
pure virtual.

Two options:
1. Go back to patch set 2 (to having a default implementation).
2. Go back to having a default implementation, but keep the implementations in
patch set 4 as well.

I suggest option 1 + use a new result state to indicate this if we want to catch
this in production.

On 2014/09/23 08:45:43, tommi wrote:
> Sounds like you're saying that there are two things going against the
> guidelines:
> - pure virtual methods.
> - non-empty implementations.
> 
> How would you prefer this to be done?
> 
> On Tue, Sep 23, 2014 at 7:06 AM, <mailto:jam@chromium.org> wrote:
> 
> > this runs counter to the content api guidelines:
> > http://www.chromium.org/developers/content-module/content-api
> > which is that interfaces implemented by the embedder should have empty
> > implementations so that an embedder only implements what they need
> > initially to
> > make it easy.
> >
> > https://codereview.chromium.org/584383002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tommi (sloooow) - chröme, 2014-09-23 13:16:58]

On 2014/09/23 13:05:09, Henrik Grunell wrote:
> Hmm, it's clearly against the guidelines to have WebContentsDelegate functions
> pure virtual.
> 
> Two options:
> 1. Go back to patch set 2 (to having a default implementation).
> 2. Go back to having a default implementation, but keep the implementations in
> patch set 4 as well.
> 
> I suggest option 1 + use a new result state to indicate this if we want to
catch
> this in production.
> 
> On 2014/09/23 08:45:43, tommi wrote:
> > Sounds like you're saying that there are two things going against the
> > guidelines:
> > - pure virtual methods.
> > - non-empty implementations.
> > 
> > How would you prefer this to be done?
> > 
> > On Tue, Sep 23, 2014 at 7:06 AM, <mailto:jam@chromium.org> wrote:
> > 
> > > this runs counter to the content api guidelines:
> > > http://www.chromium.org/developers/content-module/content-api
> > > which is that interfaces implemented by the embedder should have empty
> > > implementations so that an embedder only implements what they need
> > > initially to
> > > make it easy.
> > >
> > > https://codereview.chromium.org/584383002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

your suggestion sgtm

[Henrik Grunell, 2014-09-24 20:07:46]

grunell@chromium.org changed reviewers:
+ isherman@chromium.org

[Henrik Grunell, 2014-09-24 20:08:53]

Went back to patch set #2 and introduced new result "not supported".

Added isherman@ for histograms.xml.

Everyone: please review.

[jam, 2014-09-24 20:50:04]

https://codereview.chromium.org/584383002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/584383002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:129: // Delegates that
expect this to be ever called, must implement the function.
ditto

https://codereview.chromium.org/584383002/diff/100001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/584383002/diff/100001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:167: NOTREACHED();
here and in the other file, adding a notreached goes against the conventions
we've used everywhere else.the point is that an embedder can take content, and
within a few lines of code embed it. not every feature will work. you can add a
LOG(ERROR). but having dcheck or check run is not something we want

https://codereview.chromium.org/584383002/diff/100001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/584383002/diff/100001/content/public/browser/...
content/public/browser/web_contents_delegate.h:443: // Delegates that expect
this to be ever called, must implement the function.
nit: this comment and the one below are redundant.

[Ilya Sherman, 2014-09-24 23:15:38]

histograms.xml lgtm

[Henrik Grunell, 2014-09-25 18:28:38]

John, ptal.

https://codereview.chromium.org/584383002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/584383002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:129: // Delegates that
expect this to be ever called, must implement the function.
On 2014/09/24 20:50:04, jam wrote:
> ditto

Done.

https://codereview.chromium.org/584383002/diff/100001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/584383002/diff/100001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:167: NOTREACHED();
On 2014/09/24 20:50:04, jam wrote:
> here and in the other file, adding a notreached goes against the conventions
> we've used everywhere else.the point is that an embedder can take content, and
> within a few lines of code embed it. not every feature will work. you can add
a
> LOG(ERROR). but having dcheck or check run is not something we want

Done.

https://codereview.chromium.org/584383002/diff/100001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/584383002/diff/100001/content/public/browser/...
content/public/browser/web_contents_delegate.h:443: // Delegates that expect
this to be ever called, must implement the function.
On 2014/09/24 20:50:04, jam wrote:
> nit: this comment and the one below are redundant.

Done.

[jam, 2014-09-25 18:50:27]

lgtm with comments

https://codereview.chromium.org/584383002/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.cc (right):

https://codereview.chromium.org/584383002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.cc:42: LOG_F(ERROR) <<
"Not supported.";
this looks like a webrtc macro. please don't use that outside of webrtc, and
instead use LOG(ERROR)

https://codereview.chromium.org/584383002/diff/120001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/584383002/diff/120001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:167: LOG_F(ERROR) << "Not
supported.";
ditto here and below

[Henrik Grunell, 2014-09-25 18:54:11]

https://codereview.chromium.org/584383002/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.cc (right):

https://codereview.chromium.org/584383002/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.cc:42: LOG_F(ERROR) <<
"Not supported.";
On 2014/09/25 18:50:27, jam wrote:
> this looks like a webrtc macro. please don't use that outside of webrtc, and
> instead use LOG(ERROR)

Haha, slipped. Fixed.

https://codereview.chromium.org/584383002/diff/120001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/584383002/diff/120001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:167: LOG_F(ERROR) << "Not
supported.";
On 2014/09/25 18:50:27, jam wrote:
> ditto here and below

Done.

[Henrik Grunell, 2014-09-25 18:54:17]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-09-25 18:55:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584383002/140001

[Henrik Grunell, 2014-09-25 20:11:29]

The CQ bit was unchecked by grunell@chromium.org

[Henrik Grunell, 2014-09-25 20:11:41]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-09-25 20:13:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584383002/140001

[commit-bot: I haz the power, 2014-09-25 20:14:43]

Committed patchset #8 (id:140001) as b723372ef4505f5cd50f62ada92f0d3451994082

[commit-bot: I haz the power, 2014-09-25 20:15:22]

Patchset 8 (id:??) landed as
https://crrev.com/dea7b93642a46bdaaecad4a438bba1bfc8cc8516
Cr-Commit-Position: refs/heads/master@{#296779}