[Webkit-unassigned] [Bug 27418] [V8] Factor V8ContextFactory out of V8Proxy

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27418





--- Comment #6 from Adam Barth <abarth at webkit.org>  2009-07-19 20:19:07 PDT ---
(In reply to comment #5)
> I tried to review this but the re-ordering of operations doesn't make this as
> mechanical as I would like (for me to review), so I'll just note the things I
> saw during my attempt and not do an r+ or r-.

Yes.  This one is much more complex than the other ones.  I tried to do this in
two steps, but this seemed easier.  I can try to break it up again...

> > +#include "V8ContextFactory.h"
> 
> This one should come first after config.h

Fixed.

> > +v8::Persistent<v8::Context> V8ContextFactory::create(Frame* frame, v8::Handle<v8::Object> global)
> > +{
> 
> > +    // Enter the next context.
> > +    v8::Context::Scope contextScope(context);
> > +
> > +    if (!installHiddenObjectPrototype(context) || !installDOMWindow(context, frame->domWindow())) {
> > +        context.Dispose();
> > +        context.Clear();
> > +    }
> 
> This change made it hardest for me to review because (to my v8 naive eyes) it
> look like this may cause different code to be executed in some cases.

Yes.  This changes the code path slightly.

> For example, previously it check  if (context.IsEmpty()) before doing this.

Yes.  I think that was missing in the new version.  Re-added.

> It also did the code in "// Store the first global object created so we can
> reuse " before doing this.  Now this code is executed first and may clear the
> context so that the "global object" code may not be executed.
> It may all be correct, but it was the least mechanical part of the changes so I
> couldn't easily determine its correctness.

I think this part is fine.  The code isn't written that well, but the intent is
for the creation function to be transactional.  If the context creating
succeed, great.  If not, the changes are supposed to be rolled back via
disposeContextHandles(), which, for example, clears out any m_global we might
have stored.

It's possible I should do another round to make the transactional nature more
clear / robust to changes.

> > +bool V8ContextFactory::installDOMWindow(v8::Handle<v8::Context> context, DOMWindow* window)
> ...
> > +    // Create a new JS window object and use it as the prototype for the  shadow global object.
> 
> It was there before but please get rid of the double space before shadow.

Fixed.

> > +void V8ContextFactory::ensureUtilityContext()
> > +{
> > +    if (!m_utilityContext.IsEmpty())
> > +        return;
> 
> This was an assert before. Why is it changing?

Because now we call this function unconditionally instead of putting the branch
in the caller.  This seems more robust.

> > Index: WebCore/bindings/v8/V8ContextFactory.h
> > +#include <v8.h>
> 
> I think it may be standard at this point to put this header first in these
> files but it goes against the standard ordering of these headers, so why not
> (fix this and) move it to where the other <> headers are?

I'm not sure exactly what order your asking for, but I've changed this to be
more like other files.  Let me know if this isn't what you have in mind.

> > +        // the entire process. Plan accoringly.
> typo: accoringly 

Fixed.

> 
> > +}
> 
> Change to "} // namespace WebCore"

Fixed.

> > -    v8::Handle<v8::String> implicitProtoString = v8::String::New("__proto__");
> > -    global->Set(implicitProtoString, windowWrapper);
> > -
> 
> Why is it ok to remove this? (it feels unrelated to the rest of your change.)

Basically, this code was doing installDOMWindow manually before.  By changing
this code to call V8ContextFactory::create, it get the same installDOMWindow
codepath as everyone else.  That means it doesn't need to do this __proto__
hack because the hack is centralized in in installDOMWindow.

> > Index: WebCore/bindings/v8/V8Proxy.h
> > +        static void reportUnsafeJavaScriptAccess(v8::Local<v8::Object> host, v8::AccessType type, v8::Local<v8::Value> data);
> 
> It doesn't look like the param name "type" adds any information.  Remove it?

Fixed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.