[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 #5 from David Levin <levin at chromium.org>  2009-07-19 18:47:23 PDT ---
(From update of attachment 33033)
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-.


> Index: WebCore/bindings/v8/V8ContextFactory.cpp
> +#include "config.h"
> +#include "Frame.h"
> +#include "DocumentLoader.h"
> +#include "DOMWindow.h"
> +#include "Page.h"

> +#include "V8ContextFactory.h"

This one should come first after config.h


> +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.

For example, previously it check  if (context.IsEmpty()) before doing this.
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.



> +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.


> +void V8ContextFactory::ensureUtilityContext()
> +{
> +    if (!m_utilityContext.IsEmpty())
> +        return;

This was an assert before. Why is it changing?

> 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?


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

> +}

Change to "} // namespace WebCore"

> Index: WebCore/bindings/v8/V8Proxy.cpp

> @@ -479,9 +466,6 @@ void V8Proxy::evaluateInNewContext(const
>  
>      v8::Handle<v8::Object> global = context->Global();
>  
> -    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.)

> 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?

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