Reland 8688002: PPB_TCPSocket_Private/PPB_UDPSocket_Private are exposed to Browser

Reland 8688002: PPB_TCPSocket_Private/PPB_UDPSocket_Private are exposed to Browser

PPB_{TCP|UDP}Socket_Private interfaces are exposed to Browser process. Added shared (between NaCl and Pepper) tests for both interfaces.

BUG=105859
TEST=ui_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113172

[jam, 2011-12-05 18:07:48]

lgtm after switching PepperPluginDelegateImpl to be a RenderViewObserver (i.e.
there shouldn't be any changes to render_view_impl.cc)

http://codereview.chromium.org/8804006/diff/4001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8804006/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:604: // everything.
None of RenderViewObserver's virtual methods are pure by design, so that
implementors only have to override the methods they care about. Please use
RenderViewObserver here.

[Dmitry Polukhin, 2011-12-05 19:09:26]

http://codereview.chromium.org/8804006/diff/4001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8804006/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:604: // everything.
On 2011/12/05 18:07:49, John Abd-El-Malek wrote:
> None of RenderViewObserver's virtual methods are pure by design, so that
> implementors only have to override the methods they care about. Please use
> RenderViewObserver here.

Done but it's inefficient and creates cyclic dependency in RenderViewImpl
construction order. RenderViewObserver in constructor tries to add observer to
RenderViewImple but it is not fully constructed by that point (constructor for
observers vector was not called because it is declared below pepper_delegate_).
So I had to use NULL in RenderViewObserver and add observers manually.  I can
change member order in RenderViewImpl but it's still bad to have such
construction order dependency.

[tony, 2011-12-05 19:15:25]

rubber stamp lgtm for webkit/glue/webkit_glue.gypi.

[jam, 2011-12-05 19:24:43]

http://codereview.chromium.org/8804006/diff/4001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8804006/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:604: // everything.
On 2011/12/05 19:09:26, Dmitry Polukhin wrote:
> On 2011/12/05 18:07:49, John Abd-El-Malek wrote:
> > None of RenderViewObserver's virtual methods are pure by design, so that
> > implementors only have to override the methods they care about. Please use
> > RenderViewObserver here.
> 
> Done but it's inefficient

I think only profiling can show us that this is slow. The reason we've split off
as much code as possible into RenderViewObservers is to avoid the previous
status quo, which was everything was being dispatched in RenderView. This made
the code very hard to understand.

> and creates cyclic dependency in RenderViewImpl
> construction order. RenderViewObserver in constructor tries to add observer to
> RenderViewImple but it is not fully constructed by that point (constructor for
> observers vector was not called because it is declared below
pepper_delegate_).
> So I had to use NULL in RenderViewObserver and add observers manually.  I can
> change member order in RenderViewImpl but it's still bad to have such
> construction order dependency.


I think switching the member order is better in this case (it wasn't important
before, but now it matters for this).

[jam, 2011-12-05 19:26:35]

http://codereview.chromium.org/8804006/diff/4004/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8804006/diff/4004/content/renderer/render_view...
content/renderer/render_view_impl.cc:439: AddObserver(&pepper_delegate_);
nit: here and below, in general we try to avoid RenderViewImpl doing this for
observers, hence why I prefer to have it done automatically for
PepperPluginDelegate in its RenderViewObserver constructor

[Dmitry Polukhin, 2011-12-05 19:53:50]

http://codereview.chromium.org/8804006/diff/4004/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8804006/diff/4004/content/renderer/render_view...
content/renderer/render_view_impl.cc:439: AddObserver(&pepper_delegate_);
On 2011/12/05 19:26:36, John Abd-El-Malek wrote:
> nit: here and below, in general we try to avoid RenderViewImpl doing this for
> observers, hence why I prefer to have it done automatically for
> PepperPluginDelegate in its RenderViewObserver constructor

Changing members order is not very reliable because RenderViewImpl will not be
fully constructed when RenderViewObserver constructor is called so if later
AddObserver tries to use any other member it may not be constructed - need to be
careful in future. I think better way is to make pepper_delegate_ pointer and
init it in constructor when all members are ready for sure. But it will also
require to destroy it explicit in destructor to avoid the same problem in
destructor. But I don't have strong opinion and will do according to your
preference (pointer or change members order).

[jam, 2011-12-05 19:59:28]

On 2011/12/05 19:53:50, Dmitry Polukhin wrote:
>
http://codereview.chromium.org/8804006/diff/4004/content/renderer/render_view...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/8804006/diff/4004/content/renderer/render_view...
> content/renderer/render_view_impl.cc:439: AddObserver(&pepper_delegate_);
> On 2011/12/05 19:26:36, John Abd-El-Malek wrote:
> > nit: here and below, in general we try to avoid RenderViewImpl doing this
for
> > observers, hence why I prefer to have it done automatically for
> > PepperPluginDelegate in its RenderViewObserver constructor
> 
> Changing members order is not very reliable because RenderViewImpl will not be
> fully constructed when RenderViewObserver constructor is called so if later
> AddObserver tries to use any other member it may not be constructed - need to
be
> careful in future. I think better way is to make pepper_delegate_ pointer and
> init it in constructor when all members are ready for sure. But it will also
> require to destroy it explicit in destructor to avoid the same problem in
> destructor. But I don't have strong opinion and will do according to your
> preference (pointer or change members order).

either is fine with me, so up to you. if you prefer making it a pointer, sgtm

[brettw, 2011-12-05 21:35:31]

lgtm

[yzshen1, 2011-12-05 21:48:01]

In case you didn't see this in the other thread:

> Actually I think that TCP/UDP should be exposed to limited list of
> trusted plugins. I think it's dangerous to give these interfaces to all
> trusted plugins (some of these interface have Flash, i.e. it should be
> enforced that only Flash uses them).

I agree that maybe not every trusted plugin should be allowed to use TCP/UDP.

However, the current access control for TCP/UDP seems inconsistent and
confusing:
- in-process: only a selective set of origins are allowed to use TCP/UDP
interface, no matter the request is from a trusted plugin or a NaCl one. (NaCl
uses this path.)
- out-of-proces: no restriction. (Pepper Flash uses this path.)
Moreover, the way of checking page origins doesn't apply to trusted plugins such
as Pepper Flash: it can be on any page.

I wish this could be addressed.
For now, I am okay with adding a TODO (feel free to assign to me, if you would
like to) and leave it to a future CL.

[jam, 2011-12-06 06:46:47]

http://codereview.chromium.org/8804006/diff/10001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8804006/diff/10001/content/renderer/render_vie...
content/renderer/render_view_impl.h:1223: // AddObservers method of
RenrerViewImpl from c-tor.
nit: RenderViewImpl

[Dmitry Polukhin, 2011-12-06 08:43:46]

Reordered members and added override for OnDestruct() because default
implementation in RenderViewObserver does in 'delete this'.

Added TODO about in- and out-process restriction consistency.

Thank you for review!

http://codereview.chromium.org/8804006/diff/10001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8804006/diff/10001/content/renderer/render_vie...
content/renderer/render_view_impl.h:1223: // AddObservers method of
RenrerViewImpl from c-tor.
On 2011/12/06 06:46:47, John Abd-El-Malek wrote:
> nit: RenderViewImpl

Done.