Refactor v8 extensions to make registration avoid having to use ChromiumBridg...

Refactor v8 extensions so that they aren't in the WebCore namespace, and can call functions in the rest of Chromium code without having to go through ChromiumBridge (which now lives upstream in the WebKit repository). 

R=darin,mbelshe



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

[Mike Belshe, 2009-03-04 22:45:04]

Wow this looks great - better than I expected!

LGTM

[darin (slow to review), 2009-03-04 22:51:28]

nice work... just some simple cleanup suggestions:

http://codereview.chromium.org/40132/diff/1/29
File chrome/renderer/render_process.cc (right):

http://codereview.chromium.org/40132/diff/1/29#newcode139
Line 139: WebKit::registerExtension(v8_extension::PlaybackExtension::Get());
unfortunately, this is sort-of running on the wrong thread and potentially too
soon.  By "sort-of", I mean... the WebKit API calls should only happen on the
RenderThread, and we should not call any WebKit API until after we have called
WebKit::initialize.

as it happens, i suspect that this code will work flawlessly because V8 can
handle having RegisterExtension called on any thread before the rest of V8 is in
use, and there really isn't anything special about WebKit::initialize yet...
yet!

it would probably be better to move this code to
RenderThread::EnsureWebKitInitialized.  you can easily inspect the command line
over there.

http://codereview.chromium.org/40132/diff/1/11
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/40132/diff/1/11#newcode36
Line 36: #include "webkit/extensions/v8/Gears.h"
it would be good to lowercase the names of the extensions to match google style.
 (sorry, i know that is just busy work)

http://codereview.chromium.org/40132/diff/1/19
File webkit/build/V8Bindings/SConscript (right):

http://codereview.chromium.org/40132/diff/1/19#newcode353
Line 353: '$PORT_DIR/extensions/v8/GCController.cpp',
it looks like this won't compile since the extensions directory is not in
$PORT_DIR

http://codereview.chromium.org/40132/diff/1/22
File webkit/extensions/v8/GCController.h (right):

http://codereview.chromium.org/40132/diff/1/22#newcode9
Line 9: #define GC_EXTENSION_H__
please fixup the include guards to match google style. 
WEBKIT_EXTENSIONS_V8_GCCONTROLLER_H_ <- should match the file name.. the class
name here should also match the file name.

http://codereview.chromium.org/40132/diff/1/21
File webkit/extensions/v8/Gears.cpp (right):

http://codereview.chromium.org/40132/diff/1/21#newcode8
Line 8: namespace v8_extension {
hmm... technically, the namespace should be extensions_v8 to match the directory
names.

http://codereview.chromium.org/40132/diff/1/23
File webkit/extensions/v8/Interval.cpp (right):

http://codereview.chromium.org/40132/diff/1/23#newcode7
Line 7: #include "wtf/CurrentTime.h"
please avoid dipping into WTF (internal WebKit stuff) from outside of WebKit. 
instead, this should just use base::Time::Now().ToDoubleT().

http://codereview.chromium.org/40132/diff/1/6
File webkit/glue/webkit_glue.h (left):

http://codereview.chromium.org/40132/diff/1/6#oldcode61
Line 61: void SetRecordPlaybackMode(bool value);
nice code removal!

[asargent_no_longer_on_chrome, 2009-03-05 15:30:19]

http://codereview.chromium.org/40132/diff/1/29
File chrome/renderer/render_process.cc (right):

http://codereview.chromium.org/40132/diff/1/29#newcode139
Line 139: WebKit::registerExtension(v8_extension::PlaybackExtension::Get());
On 2009/03/04 22:51:28, darin wrote:
> unfortunately, this is sort-of running on the wrong thread and potentially too
> soon.  By "sort-of", I mean... the WebKit API calls should only happen on the
> RenderThread, and we should not call any WebKit API until after we have called
> WebKit::initialize.
> 
> as it happens, i suspect that this code will work flawlessly because V8 can
> handle having RegisterExtension called on any thread before the rest of V8 is
in
> use, and there really isn't anything special about WebKit::initialize yet...
> yet!
> 
> it would probably be better to move this code to
> RenderThread::EnsureWebKitInitialized.  you can easily inspect the command
line
> over there.

Done.

http://codereview.chromium.org/40132/diff/1/11
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/40132/diff/1/11#newcode36
Line 36: #include "webkit/extensions/v8/Gears.h"
On 2009/03/04 22:51:28, darin wrote:
> it would be good to lowercase the names of the extensions to match google
style.
>  (sorry, i know that is just busy work)

Done.

http://codereview.chromium.org/40132/diff/1/19
File webkit/build/V8Bindings/SConscript (right):

http://codereview.chromium.org/40132/diff/1/19#newcode353
Line 353: '$PORT_DIR/extensions/v8/GCController.cpp',
On 2009/03/04 22:51:28, darin wrote:
> it looks like this won't compile since the extensions directory is not in
> $PORT_DIR

Done.

http://codereview.chromium.org/40132/diff/1/22
File webkit/extensions/v8/GCController.h (right):

http://codereview.chromium.org/40132/diff/1/22#newcode9
Line 9: #define GC_EXTENSION_H__
On 2009/03/04 22:51:28, darin wrote:
> please fixup the include guards to match google style. 
> WEBKIT_EXTENSIONS_V8_GCCONTROLLER_H_ <- should match the file name.. the class
> name here should also match the file name.

Done.

http://codereview.chromium.org/40132/diff/1/21
File webkit/extensions/v8/Gears.cpp (right):

http://codereview.chromium.org/40132/diff/1/21#newcode8
Line 8: namespace v8_extension {
On 2009/03/04 22:51:28, darin wrote:
> hmm... technically, the namespace should be extensions_v8 to match the
directory
> names.

Done.

http://codereview.chromium.org/40132/diff/1/23
File webkit/extensions/v8/Interval.cpp (right):

http://codereview.chromium.org/40132/diff/1/23#newcode7
Line 7: #include "wtf/CurrentTime.h"
On 2009/03/04 22:51:28, darin wrote:
> please avoid dipping into WTF (internal WebKit stuff) from outside of WebKit. 
> instead, this should just use base::Time::Now().ToDoubleT().

Done.

[darin (slow to review), 2009-03-05 17:42:25]

LGTM !

http://codereview.chromium.org/40132/diff/36/60
File webkit/extensions/v8/interval_extension.cc (right):

http://codereview.chromium.org/40132/diff/36/60#newcode6
Line 6: #include "webkit/extensions/v8/interval_extension.h"
minor style nit: foo.cc should list the foo.h include first.