Pepper: specify the security origin when enumerating media devices.

Pepper: specify the security origin when enumerating media devices.

BUG=312841
TEST=None
R=avi@chromium.org, bbudge@chromium.org, joi@chromium.org, wjia@chromium.org

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

[yzshen1, 2013-10-30 23:46:18]

Hi, Wei and Bill.

Would you please take a look? Thanks!

[bbudge, 2013-10-31 00:31:41]

https://codereview.chromium.org/48743008/diff/50001/content/public/renderer/r...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/48743008/diff/50001/content/public/renderer/r...
content/public/renderer/renderer_ppapi_host.h:134: virtual GURL
GetDocumentURL(PP_Instance instance) const = 0;
Can't you detect if the instance is valid using GetPluginInstance and access the
document URL through the plugin container? In other words, couldn't you
implement this here rather than making it pure virtual?

[yzshen1, 2013-10-31 00:52:02]

https://codereview.chromium.org/48743008/diff/50001/content/public/renderer/r...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/48743008/diff/50001/content/public/renderer/r...
content/public/renderer/renderer_ppapi_host.h:134: virtual GURL
GetDocumentURL(PP_Instance instance) const = 0;
On 2013/10/31 00:31:42, bbudge1 wrote:
> Can't you detect if the instance is valid using GetPluginInstance and access
the
> document URL through the plugin container? In other words, couldn't you
> implement this here rather than making it pure virtual?

I think this is a pure interface, we don't provide implementation for any of
these methods here. What do you think? Thanks!

[bbudge, 2013-10-31 01:10:02]

https://codereview.chromium.org/48743008/diff/50001/content/public/renderer/r...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/48743008/diff/50001/content/public/renderer/r...
content/public/renderer/renderer_ppapi_host.h:134: virtual GURL
GetDocumentURL(PP_Instance instance) const = 0;
On 2013/10/31 00:52:03, yzshen1 wrote:
> On 2013/10/31 00:31:42, bbudge1 wrote:
> > Can't you detect if the instance is valid using GetPluginInstance and access
> the
> > document URL through the plugin container? In other words, couldn't you
> > implement this here rather than making it pure virtual?
> 
> I think this is a pure interface, we don't provide implementation for any of
> these methods here. What do you think? Thanks!

It looks like a "convenience method" so I think it's better to implement it
somewhere in terms of this interface. It could be a static method declared and
defined here or somewhere else. Otherwise, every implementor of this interface
has to code it.

I think the Google C++ style guide allows static methods on pure interfaces:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces

But we should double check with a content/ OWNER.

[yzshen1, 2013-11-01 17:00:00]

Thanks, Bill!

https://codereview.chromium.org/48743008/diff/50001/content/public/renderer/r...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/48743008/diff/50001/content/public/renderer/r...
content/public/renderer/renderer_ppapi_host.h:134: virtual GURL
GetDocumentURL(PP_Instance instance) const = 0;
There are a few other methods in this interface are the same as
GetDocumentURL():
GetRenderViewForInstance
GetContainerForInstance

I would like them to be consistent. (Either do GetDocumentURL in the current way
or change all of them.)

I think using static methods has one issue: we will have to pass in a
RendererPpapiHost pointer as "this".

What do you think? Thanks!


On 2013/10/31 01:10:02, bbudge1 wrote:
> On 2013/10/31 00:52:03, yzshen1 wrote:
> > On 2013/10/31 00:31:42, bbudge1 wrote:
> > > Can't you detect if the instance is valid using GetPluginInstance and
access
> > the
> > > document URL through the plugin container? In other words, couldn't you
> > > implement this here rather than making it pure virtual?
> > 
> > I think this is a pure interface, we don't provide implementation for any of
> > these methods here. What do you think? Thanks!
> 
> It looks like a "convenience method" so I think it's better to implement it
> somewhere in terms of this interface. It could be a static method declared and
> defined here or somewhere else. Otherwise, every implementor of this interface
> has to code it.
> 
> I think the Google C++ style guide allows static methods on pure interfaces:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces
> 
> But we should double check with a content/ OWNER.

[yzshen1, 2013-11-01 20:34:52]

Hi, Bill.

I looked at cleaning up the interface and think that it would be better to do it
in a separate CL.
I have added a TODO for it.

Thanks for the suggestions!

[bbudge, 2013-11-01 22:22:42]

lgtm

https://codereview.chromium.org/48743008/diff/360001/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/48743008/diff/360001/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:136: // interface. Consider change
them to static helpers.
s/change/changing

[wjia(left Chromium), 2013-11-01 22:23:18]

lgtm

[yzshen1, 2013-11-01 22:33:44]

Thanks, Bill.

https://codereview.chromium.org/48743008/diff/360001/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/48743008/diff/360001/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:136: // interface. Consider change
them to static helpers.
On 2013/11/01 22:22:42, bbudge1 wrote:
> s/change/changing

Done.

[commit-bot: I haz the power, 2013-11-01 22:54:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/48743008/420001

[commit-bot: I haz the power, 2013-11-01 23:35:21]

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

[yzshen1, 2013-11-01 23:38:50]

On 2013/11/01 23:35:21, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

+jam

Hi, John.
Would you please do an owner review for
content/public/renderer/renderer_ppapi_host.h ?

Thanks!

[yzshen1, 2013-11-04 18:29:47]

On 2013/11/04 18:27:11, yzshen1 wrote:

+avi since jam is OOO

Hi, Avi.

Would you please do an OWNER review for
content/public/renderer/renderer_ppapi_host.h?
Thanks!

(Sorry for bothering multiple owners. This is blocking perkj's CL 34393006 and
he wishes he could land it before the branching point.)

[Avi (use Gerrit), 2013-11-04 19:59:44]

LGTM

https://codereview.chromium.org/48743008/diff/420001/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/48743008/diff/420001/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:134: // TODO(yzshen): Some methods
such as this one don't need to be pure virutal.
s/virutal/virtual/

[yzshen1, 2013-11-04 20:04:16]

Thanks, Avi!

https://codereview.chromium.org/48743008/diff/420001/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/48743008/diff/420001/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:134: // TODO(yzshen): Some methods
such as this one don't need to be pure virutal.
On 2013/11/04 19:59:45, Avi wrote:
> s/virutal/virtual/

Done.

[commit-bot: I haz the power, 2013-11-04 20:12:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/48743008/530001

[Jói, 2013-11-04 21:16:46]

LGTM for content/public

[commit-bot: I haz the power, 2013-11-04 23:22:05]

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

[commit-bot: I haz the power, 2013-11-05 01:36:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/48743008/870001

[commit-bot: I haz the power, 2013-11-05 03:04:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/48743008/870001

[commit-bot: I haz the power, 2013-11-05 03:59:53]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-05 06:49:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/48743008/870001

[commit-bot: I haz the power, 2013-11-05 06:49:04]

Failed to apply patch for content/public/renderer/renderer_ppapi_host.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/public/renderer/renderer_ppapi_host.h
  Hunk #1 FAILED at 14.
  Hunk #2 succeeded at 136 with fuzz 2 (offset 8 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/public/renderer/renderer_ppapi_host.h.rej

Patch:       content/public/renderer/renderer_ppapi_host.h
Index: content/public/renderer/renderer_ppapi_host.h
diff --git a/content/public/renderer/renderer_ppapi_host.h
b/content/public/renderer/renderer_ppapi_host.h
index
88a863fd2d72d6313fdc517d34a223cd0291caf2..123257d4b170eb2e5efe66df42067c09863acdeb
100644
--- a/content/public/renderer/renderer_ppapi_host.h
+++ b/content/public/renderer/renderer_ppapi_host.h
@@ -14,6 +14,7 @@
 #include "content/common/content_export.h"
 #include "ipc/ipc_platform_file.h"
 #include "ppapi/c/pp_instance.h"
+#include "url/gurl.h"
 
 namespace base {
 class FilePath;
@@ -128,6 +129,13 @@ class RendererPpapiHost {
       const std::vector<IPC::Message>& nested_msgs,
       const base::Callback<void(const std::vector<int>&)>& callback) const = 0;
 
+  // Gets the URL of the document containing the given PP_Instance.
+  // Returns an empty URL if the instance is invalid.
+  // TODO(yzshen): Some methods such as this one don't need to be pure virtual.
+  // Instead, they could be directly implemented using other methods in this
+  // interface. Consider changing them to static helpers.
+  virtual GURL GetDocumentURL(PP_Instance instance) const = 0;
+
  protected:
   virtual ~RendererPpapiHost() {}
 };

[yzshen1, 2013-11-05 06:58:26]

Committed patchset #6 manually as r232926 (presubmit successful).