Move broker creation out of PepperHelperImpl to PPB_Broker_Impl in the effort to eliminate PepperHe…

Move broker creation out of PepperHelperImpl to PPB_Broker_Impl in the effort to eliminate PepperHelperImpl. This also simplifies things as we can eliminate the bookkeeping and race condition handling.

BUG=263054
R=dmichael@chromium.org

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

[dmichael (off chromium), 2013-08-06 18:43:31]

lgtm

https://codereview.chromium.org/22300003/diff/54003/content/browser/renderer_...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/22300003/diff/54003/content/browser/renderer_...
content/browser/renderer_host/render_message_filter.cc:189: int plugin_child_id)
OVERRIDE {
nit: I'd prefer you left it commented out if it's still unused.

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
File content/renderer/pepper/ppb_broker_impl.cc (right):

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
content/renderer/pepper/ppb_broker_impl.cc:95: broker_->AddPendingConnect(this);
The comment is not quite right now; broker_ is just a raw pointer, and isn't a
stack variable.

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
content/renderer/pepper/ppb_broker_impl.cc:147:
broker_->OnBrokerPermissionResult(this, result);
nit: indentation is off

[jam, 2013-08-06 19:34:09]

https://codereview.chromium.org/22300003/diff/54003/content/browser/renderer_...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/22300003/diff/54003/content/browser/renderer_...
content/browser/renderer_host/render_message_filter.cc:189: int plugin_child_id)
OVERRIDE {
On 2013/08/06 18:43:31, dmichael wrote:
> nit: I'd prefer you left it commented out if it's still unused.

this isn't really a pattern that we use in chrome code. but i'll change it back
cause i'm just applying my preference to existing code

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
File content/renderer/pepper/ppb_broker_impl.cc (right):

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
content/renderer/pepper/ppb_broker_impl.cc:95: broker_->AddPendingConnect(this);
On 2013/08/06 18:43:31, dmichael wrote:
> The comment is not quite right now; broker_ is just a raw pointer, and isn't a
> stack variable.

I believe that comment (which I moved) is referring to the ref which is taken
inside the AddPendingConnect method

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
content/renderer/pepper/ppb_broker_impl.cc:147:
broker_->OnBrokerPermissionResult(this, result);
On 2013/08/06 18:43:31, dmichael wrote:
> nit: indentation is off

Done.

[dmichael(do not use this one), 2013-08-06 19:48:04]

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
File content/renderer/pepper/ppb_broker_impl.cc (right):

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
content/renderer/pepper/ppb_broker_impl.cc:95: broker_->AddPendingConnect(this);
On 2013/08/06 19:34:09, jam wrote:
> On 2013/08/06 18:43:31, dmichael wrote:
> > The comment is not quite right now; broker_ is just a raw pointer, and isn't
a
> > stack variable.
> 
> I believe that comment (which I moved) is referring to the ref which is taken
> inside the AddPendingConnect method
The old comment referred to a stack variable named |broker| which was a
scoped_refptr, which held the only reference, so would cause a delete if
AddPendingConnect wasn't called before exiting scope. Now, there's nothing that
releases a reference on leaving scope, and broker is broker_, so the comment
should probably be something like:
// Adds a reference, ensuring that the broker is not deleted until
// we call Disconnect.

[jam, 2013-08-06 19:55:45]

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
File content/renderer/pepper/ppb_broker_impl.cc (right):

https://codereview.chromium.org/22300003/diff/54003/content/renderer/pepper/p...
content/renderer/pepper/ppb_broker_impl.cc:95: broker_->AddPendingConnect(this);
On 2013/08/06 19:48:05, dmichael(do not use this one) wrote:
> On 2013/08/06 19:34:09, jam wrote:
> > On 2013/08/06 18:43:31, dmichael wrote:
> > > The comment is not quite right now; broker_ is just a raw pointer, and
isn't
> a
> > > stack variable.
> > 
> > I believe that comment (which I moved) is referring to the ref which is
taken
> > inside the AddPendingConnect method
> The old comment referred to a stack variable named |broker| which was a
> scoped_refptr, which held the only reference, so would cause a delete if
> AddPendingConnect wasn't called before exiting scope. Now, there's nothing
that
> releases a reference on leaving scope, and broker is broker_, so the comment
> should probably be something like:
> // Adds a reference, ensuring that the broker is not deleted until
> // we call Disconnect.


ah, I missed that. i'll update in my next cl, thanks