Bug 26962

Summary: [V8] Move some simple utility functions in V8Proxy to V8Utilities
Product: WebKit Reporter: Adam Barth <abarth>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: dglazkov, eric, mbelshe
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
eric: review-
The whole file none

Description Adam Barth 2009-07-03 22:07:43 PDT
Patch forthcoming.
Comment 1 Adam Barth 2009-07-03 22:09:20 PDT
Created attachment 32252 [details]
Comment 2 Adam Barth 2009-07-03 22:11:42 PDT
Comment on attachment 32252 [details]

Nevermind.  That doesn't compile.  :(
Comment 3 Adam Barth 2009-07-06 02:55:17 PDT
Created attachment 32294 [details]
Comment 4 Adam Barth 2009-07-06 02:59:17 PDT
Created attachment 32295 [details]
The whole file

Eric suggested that I group the functions before moving them.  The diff was kind of hard to read, so I'm attaching the whole file.
Comment 5 Eric Seidel (no email) 2009-07-07 00:17:36 PDT
Comment on attachment 32294 [details]

Your comment format is a bit strange.  I would not have added all the crazy //////.

It would make sense to me to describe each of these new classes some, more than just putting their names there.

// Move to V8Constants, a class to hold all the constants used by V8?

// Move to V8EventListener, a subclass of EventListener with v8 specific methods.
Comment 6 Eric Seidel (no email) 2009-07-07 00:18:15 PDT
The r- is for not really knowing what all these new files do.  The change itself is fine in idea... but it seems like we should document where these are going (if you want a real review at least).
Comment 7 Adam Barth 2009-07-07 00:42:25 PDT
Ok.  This kind of patch seems hard to discuss via this system.  Maybe it will be better to talk with Dimitri and Mike in person and come up with a plan and then execute the plan.  Some of the groupings are really obvious.  For example, the GC methods have virtually no overlap in concepts with the rest of the class.  I think I went a bit overboard trying to find a home for everything in this patch.  Thanks for the review.
Comment 8 Eric Seidel (no email) 2009-07-07 00:45:46 PDT
Yeah, I wasn't really gonna review where you were gonna put stuff.  So much as just that whoever next reads those comments should have some more idea than just the new class name. :)

Also, I am alergic to *Utilities classes and *Helper classes :)
Comment 9 Adam Barth 2009-07-07 00:53:26 PDT
> Also, I am alergic to *Utilities classes and *Helper classes :)

Oddly, V8Utilities is the only one of the bunch that currently exists!  There might be a better place for the those exception idioms.  V8ExceptionIdioms.h?  :)
Comment 10 Dimitri Glazkov (Google) 2009-07-08 14:53:50 PDT
How about we start small?

* Move event-related stuff to V8CustomEventListener
* Move V8Executive-slated things to ScriptController?
Comment 11 Adam Barth 2009-07-08 14:57:39 PDT
(In reply to comment #10)
> How about we start small?

Ok.  I'm actually just working on the GCController stuff atm.  That looks pretty self-contained.
Comment 12 Dimitri Glazkov (Google) 2009-07-08 15:01:39 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > How about we start small?
> Ok.  I'm actually just working on the GCController stuff atm.  That looks
> pretty self-contained.

Sounds great!