Refactor extension bindings to share code, avoid exposing hidden variables

Refactor extension bindings to share code, avoid exposing hidden variables
globally, and avoid using the DOM load/unload events.
- moved callback handling into event_bindings.js (ports will use it).
- added chromeHidden, a V8 hidden value, to keep all internal variables that
  need to be accessible to native code.
- changed context registration to occur always at extension load, instead of
  DOM load.
- added an internal unload event that doesn't disable SuddenTermination.

This is a rework of my earlier CL http://codereview.chromium.org/125280 which was reverted because of a perf regression.  I believe the perf problem was caused by the call into javascript I did on page load to handle context registration - this CL avoids that.

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

[Aaron Boodman, 2009-06-23 19:40:02]

Nice cleanup.

http://codereview.chromium.org/147033/diff/1/4
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/147033/diff/1/4#newcode22
Line 22: class ExtensionBase : public v8::Extension {
Comments?

http://codereview.chromium.org/147033/diff/1/4#newcode70
Line 70: // Returns the ContextInfo item for
Comment is missing some words.

http://codereview.chromium.org/147033/diff/1/4#newcode73
Line 73: // Contains info relevant to a pending request.
Might want to s/request/API request/ (not clear from context that this doesn't
mean http request or something).

http://codereview.chromium.org/147033/diff/1/4#newcode94
Line 94: // special "chromeHidden" object, which is a hidden value not visible
outside of
Is it necessary to support any other top-level objects other than
"chromeHidden"? Perhaps it should just be assumed? (this would also help enforce
this pattern in future code).

http://codereview.chromium.org/147033/diff/1/6
File chrome/renderer/extensions/event_bindings.h (right):

http://codereview.chromium.org/147033/diff/1/6#newcode25
Line 25: // Notify script that the frame is closing.  We don't use the DOM
Nit: Looks like this comment paragraph can be re-wrapped.

http://codereview.chromium.org/147033/diff/1/18
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/147033/diff/1/18#newcode37
Line 37: chromeHidden.Port = {};
This should be "ports_".

Also, generally, I don't think we should be putting all random internal state
for the bindings on chromeHidden -- only the things that either the C++ needs to
call, or needs to be access across JavaScript files (eg sendRequest).

If we put everything in there, it increases the chance of conflict when multiple
files try to use the same name.

This can just be a local variable.

[Matt Perry, 2009-06-23 20:01:17]

http://codereview.chromium.org/147033/diff/1/4
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/147033/diff/1/4#newcode22
Line 22: class ExtensionBase : public v8::Extension {
On 2009/06/23 19:40:03, Aaron Boodman wrote:
> Comments? 

Done.

http://codereview.chromium.org/147033/diff/1/4#newcode70
Line 70: // Returns the ContextInfo item for
On 2009/06/23 19:40:03, Aaron Boodman wrote:
> Comment is missing some words.

Done.

http://codereview.chromium.org/147033/diff/1/4#newcode73
Line 73: // Contains info relevant to a pending request.
On 2009/06/23 19:40:03, Aaron Boodman wrote:
> Might want to s/request/API request/ (not clear from context that this doesn't
> mean http request or something).

Done.

http://codereview.chromium.org/147033/diff/1/4#newcode94
Line 94: // special "chromeHidden" object, which is a hidden value not visible
outside of
On 2009/06/23 19:40:03, Aaron Boodman wrote:
> Is it necessary to support any other top-level objects other than
> "chromeHidden"? Perhaps it should just be assumed? (this would also help
enforce
> this pattern in future code).

Done.

http://codereview.chromium.org/147033/diff/1/6
File chrome/renderer/extensions/event_bindings.h (right):

http://codereview.chromium.org/147033/diff/1/6#newcode25
Line 25: // Notify script that the frame is closing.  We don't use the DOM
On 2009/06/23 19:40:03, Aaron Boodman wrote:
> Nit: Looks like this comment paragraph can be re-wrapped.

Done.

http://codereview.chromium.org/147033/diff/1/18
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/147033/diff/1/18#newcode37
Line 37: chromeHidden.Port = {};
On 2009/06/23 19:40:03, Aaron Boodman wrote:
> This should be "ports_".
> 
> Also, generally, I don't think we should be putting all random internal state
> for the bindings on chromeHidden -- only the things that either the C++ needs
to
> call, or needs to be access across JavaScript files (eg sendRequest).
> 
> If we put everything in there, it increases the chance of conflict when
multiple
> files try to use the same name.
> 
> This can just be a local variable.

I'm not sure I understand this comment.  chromeHidden.Port is not the list of
ports - it's just a logical grouping for Port-related callbacks.  All the things
I'm putting on chromeHidden are needed by native code, or by another module.

[Aaron Boodman, 2009-06-23 21:34:37]

lgtm

http://codereview.chromium.org/147033/diff/1/18
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/147033/diff/1/18#newcode37
Line 37: chromeHidden.Port = {};
On 2009/06/23 20:01:17, Matt Perry wrote:
> On 2009/06/23 19:40:03, Aaron Boodman wrote:
> > This should be "ports_".
> > 
> > Also, generally, I don't think we should be putting all random internal
state
> > for the bindings on chromeHidden -- only the things that either the C++
needs
> to
> > call, or needs to be access across JavaScript files (eg sendRequest).
> > 
> > If we put everything in there, it increases the chance of conflict when
> multiple
> > files try to use the same name.
> > 
> > This can just be a local variable.
> 
> I'm not sure I understand this comment.  chromeHidden.Port is not the list of
> ports - it's just a logical grouping for Port-related callbacks.  All the
things
> I'm putting on chromeHidden are needed by native code, or by another module.

My mistake, when I read this at first, I thought that chrome.Port.ports_ in the
old code became chromeHidden.Port in the new code.

[Matt Perry, 2009-06-24 01:35:04]

Dmitri, can you look at v8_proxy.cpp and see if what I did there is kosher?  The
problem I am trying to solve is retrieving the WebFrame for the current V8
context, which originally didn't work during v8 extension loading, because that
happens before the DOMWINDOW property is set.

[Matt Perry, 2009-06-25 02:03:06]

On 2009/06/24 01:35:04, Matt Perry wrote:
> Dmitri, can you look at v8_proxy.cpp and see if what I did there is kosher? 
The
> problem I am trying to solve is retrieving the WebFrame for the current V8
> context, which originally didn't work during v8 extension loading, because
that
> happens before the DOMWINDOW property is set.

Nevermind, I need to take a different approach with this CL.

[dglazkov, 2009-06-25 17:30:05]

keep-up Fail.

[Matt Perry, 2009-06-30 00:00:11]

Sorry to make you keep reviewing this, but I checked in a webkit change
(http://codereview.chromium.org/147124) that gives us notifications about V8
contexts coming/going, which simplifies a bunch of what I was doing.  Can you
take another look?

[Aaron Boodman, 2009-06-30 01:55:06]

http://codereview.chromium.org/147033/diff/2001/3003
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/147033/diff/2001/3003#newcode63
Line 63: // Contains information about a single Javascript context.
Nit: JavaScript, or javascript if you're feeling lazy, but not Javascript.

http://codereview.chromium.org/147033/diff/2001/3004
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/147033/diff/2001/3004#newcode129
Line 129: void EventBindings::HandleContextCreated(WebFrame* frame) {
Add a "static" comment? (or take the others away). Having this one different
makes me think it isn't static.

http://codereview.chromium.org/147033/diff/2001/3012
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/147033/diff/2001/3012#newcode188
Line 188: chromeHidden.onUnload = new chrome.Event();
It seems like now that we have a good way to know when contexts go away, we no
longer need this JS event at all. All it ends up doing is deleting some JS vars
(unneeded -- the context is dying anyway), and detaching some events. We can
detach the events directly in C++.

[Matt Perry, 2009-06-30 17:53:17]

http://codereview.chromium.org/147033/diff/2001/3003
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/147033/diff/2001/3003#newcode63
Line 63: // Contains information about a single Javascript context.
On 2009/06/30 01:55:06, Aaron Boodman wrote:
> Nit: JavaScript, or javascript if you're feeling lazy, but not Javascript.

Not in chrome's codebase..

http://codereview.chromium.org/147033/diff/2001/3004
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/147033/diff/2001/3004#newcode129
Line 129: void EventBindings::HandleContextCreated(WebFrame* frame) {
On 2009/06/30 01:55:06, Aaron Boodman wrote:
> Add a "static" comment? (or take the others away). Having this one different
> makes me think it isn't static.

Done.

http://codereview.chromium.org/147033/diff/2001/3012
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/147033/diff/2001/3012#newcode188
Line 188: chromeHidden.onUnload = new chrome.Event();
On 2009/06/30 01:55:06, Aaron Boodman wrote:
> It seems like now that we have a good way to know when contexts go away, we no
> longer need this JS event at all. All it ends up doing is deleting some JS
vars
> (unneeded -- the context is dying anyway), and detaching some events. We can
> detach the events directly in C++.

I'm not using it yet, but onUnload will be used by ports to disconnect
themselves and notify the other other.

[Aaron Boodman, 2009-06-30 18:38:40]

lgtm

On 2009/06/30 17:53:17, Matt Perry wrote:
> http://codereview.chromium.org/147033/diff/2001/3003#newcode63
> Line 63: // Contains information about a single Javascript context.
> On 2009/06/30 01:55:06, Aaron Boodman wrote:
> > Nit: JavaScript, or javascript if you're feeling lazy, but not Javascript.
>
> Not in chrome's codebase..

I did a search and all three forms seem about equally prevalent in the  Chrome
codebase. And in any case, the name of the language is JavaScript. It isn't the
capitalization I would have chosen, but there it is.