Browser Plugin: Implement getProcessId

Browser Plugin: Implement getProcessId

When we commit a navigation, we forward the RenderProcessHost ID to the Browser Plugin in the embedder so that
we can query it via getProcessId()

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

[lazyboy, 2012-09-20 19:47:04]

Sending nits and one possible assert to add in tests.

https://chromiumcodereview.appspot.com/10965017/diff/1/content/browser/browse...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10965017/diff/1/content/browser/browse...
content/browser/browser_plugin/browser_plugin_guest.cc:254:
DCHECK(embedder_render_process_host());
Sorry, this is from before though: this check should be in
SendMessageToEmbedder() instead.

https://chromiumcodereview.appspot.com/10965017/diff/1/content/renderer/brows...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10965017/diff/1/content/renderer/brows...
content/renderer/browser_plugin/browser_plugin.h:38: // Get the process ID of
the current guest.
nit: Returns the process ID....

https://chromiumcodereview.appspot.com/10965017/diff/1/content/renderer/brows...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (left):

https://chromiumcodereview.appspot.com/10965017/diff/1/content/renderer/brows...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:316:
browser_plugin->DidNavigate(GURL(kGoogleURL));
ASSERT browser_plugin->getProcessId() == 0 if we haven't navigated to a page
yet?
I'm thinking if that's what we should expect though, or the behavior is just
undefined?

https://chromiumcodereview.appspot.com/10965017/diff/1/content/renderer/brows...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10965017/diff/1/content/renderer/brows...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:307:
"document.getElementById('browserplugin')."
nit: next line fits in this line

[Fady Samuel, 2012-09-20 21:58:05]

https://codereview.chromium.org/10965017/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10965017/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:254:
DCHECK(embedder_render_process_host());
On 2012/09/20 19:47:04, lazyboy wrote:
> Sorry, this is from before though: this check should be in
> SendMessageToEmbedder() instead.

Done.

https://codereview.chromium.org/10965017/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/10965017/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.h:38: // Get the process ID of
the current guest.
On 2012/09/20 19:47:04, lazyboy wrote:
> nit: Returns the process ID....

Done.

https://codereview.chromium.org/10965017/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (left):

https://codereview.chromium.org/10965017/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:316:
browser_plugin->DidNavigate(GURL(kGoogleURL));
On 2012/09/20 19:47:04, lazyboy wrote:
> ASSERT browser_plugin->getProcessId() == 0 if we haven't navigated to a page
> yet?
> I'm thinking if that's what we should expect though, or the behavior is just
> undefined?

I believe 0 is a valid RenderProcessHost ID. I think it's fine to leave this
undefined or perhaps -1? I'll let Charlie take a look at htis CL and get his
input on this.

https://codereview.chromium.org/10965017/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

https://codereview.chromium.org/10965017/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:307:
"document.getElementById('browserplugin')."
On 2012/09/20 19:47:04, lazyboy wrote:
> nit: next line fits in this line

Done.

[Charlie Reis, 2012-09-21 00:24:06]

LGTM with nits.

https://codereview.chromium.org/10965017/diff/3002/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10965017/diff/3002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:258:
render_view_host->GetProcess()->GetID()));
Interesting that we're piggy-backing on navigation commits, but I think that's a
reasonable time to do it.  I don't see a strong need for apps to be notified as
soon as the process is created, unless we later want to help them make very
fine-grained task managers.

I think this approach will work fine for the foreseeable future.

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:62: process_id_(0) {
-1, perhaps?  I believe 0 refers to the browser process.

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:38: // Returns the process ID
of the current guest.
nit: Chrome's process ID.
(Just want to distinguish between that and the OS process ID, which may change
after a crash.  Chrome's process ID stays stable, barring any cross-process
navigations.)

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_bindings.cc:42: const char
kGetProcessID[] = "getProcessId";
Ah, the lowercase "d" in ID.  How I hate that convention.  :)  It's correct,
though!

Maybe make the constant name consistent with it?

[Fady Samuel, 2012-09-21 14:48:46]

Fixed nits. Landing once tree is green.

https://codereview.chromium.org/10965017/diff/3002/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10965017/diff/3002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:258:
render_view_host->GetProcess()->GetID()));
On 2012/09/21 00:24:06, creis wrote:
> Interesting that we're piggy-backing on navigation commits, but I think that's
a
> reasonable time to do it.  I don't see a strong need for apps to be notified
as
> soon as the process is created, unless we later want to help them make very
> fine-grained task managers.
> 
> I think this approach will work fine for the foreseeable future.

Each navigation has the potential to be to a new process (in the future), so I
thought that we might as well do this.

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:62: process_id_(0) {
On 2012/09/21 00:24:06, creis wrote:
> -1, perhaps?  I believe 0 refers to the browser process.

Done.

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:38: // Returns the process ID
of the current guest.
On 2012/09/21 00:24:06, creis wrote:
> nit: Chrome's process ID.
> (Just want to distinguish between that and the OS process ID, which may change
> after a crash.  Chrome's process ID stays stable, barring any cross-process
> navigations.)

Done.

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/10965017/diff/3002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_bindings.cc:42: const char
kGetProcessID[] = "getProcessId";
On 2012/09/21 00:24:06, creis wrote:
> Ah, the lowercase "d" in ID.  How I hate that convention.  :)  It's correct,
> though!
> 
> Maybe make the constant name consistent with it?

Done.