Bug 27997

Summary: [V8] Style cleaning for WorkerContextExecutionProxy.
Product: WebKit Reporter: Jian Li <jianli>
Component: WebCore Misc.Assignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed Patch
none
Add info to ChangeLog
dglazkov: review-
Proposed Patch dglazkov: review+

Jian Li
Reported 2009-08-04 14:36:35 PDT
Style cleaning for WorkerContextExecutionProxy.
Attachments
Proposed Patch (23.64 KB, patch)
2009-08-04 15:13 PDT, Jian Li
no flags
Add info to ChangeLog (24.17 KB, patch)
2009-08-04 21:32 PDT, Jian Li
dglazkov: review-
Proposed Patch (24.32 KB, patch)
2009-08-06 13:18 PDT, Jian Li
dglazkov: review+
Jian Li
Comment 1 2009-08-04 15:13:57 PDT
Created attachment 34092 [details] Proposed Patch
Eric Seidel (no email)
Comment 2 2009-08-04 18:59:11 PDT
Comment on attachment 34092 [details] Proposed Patch More ChangeLog information would help me understand what all you're changing here.
Jian Li
Comment 3 2009-08-04 21:32:00 PDT
Created attachment 34116 [details] Add info to ChangeLog
Dimitri Glazkov (Google)
Comment 4 2009-08-06 11:34:50 PDT
Comment on attachment 34116 [details] Add info to ChangeLog This looks good overall. A couple of comments: > diff --git a/WebCore/bindings/v8/ScheduledAction.cpp b/WebCore/bindings/v8/ScheduledAction.cpp > index 298f6b1..ce4ecf1 100644 > --- a/WebCore/bindings/v8/ScheduledAction.cpp > +++ b/WebCore/bindings/v8/ScheduledAction.cpp > @@ -134,7 +134,7 @@ void ScheduledAction::execute(WorkerContext* workerContext) > > if (!m_function.IsEmpty() && m_function->IsFunction()) { > v8::HandleScope handleScope; > - v8::Local<v8::Context> v8Context = scriptController->proxy()->GetContext(); > + v8::Local<v8::Context> v8Context = scriptController->proxy()->context(); Be careful, abarth is working on change to remove V8Proxy::context(), talk w/him. > +v8::Local<v8::Function> V8DOMWrapper::getConstructor(V8ClassIndex::V8WrapperType type, v8::Handle<v8::Context> context) > +{ > + // Enter the scope for this context to get the correct constructor. > + v8::Context::Scope scope(context); > + > + return getConstructor(type, V8Proxy::getHiddenObjectPrototype(context)); > +} Can we have a better, more specific name for this method and perhaps make it private? It shouldn't be used outside of this class.
Adam Barth
Comment 5 2009-08-06 11:52:31 PDT
(In reply to comment #4) > > - v8::Local<v8::Context> v8Context = scriptController->proxy()->GetContext(); > > + v8::Local<v8::Context> v8Context = scriptController->proxy()->context(); V8Proxy::context() must die. It's not worth renaming it at this point.
Jian Li
Comment 6 2009-08-06 13:18:18 PDT
Created attachment 34221 [details] Proposed Patch
Jian Li
Comment 7 2009-08-06 13:19:48 PDT
(In reply to comment #4) > (From update of attachment 34116 [details]) > This looks good overall. A couple of comments: > > > diff --git a/WebCore/bindings/v8/ScheduledAction.cpp b/WebCore/bindings/v8/ScheduledAction.cpp > > index 298f6b1..ce4ecf1 100644 > > --- a/WebCore/bindings/v8/ScheduledAction.cpp > > +++ b/WebCore/bindings/v8/ScheduledAction.cpp > > @@ -134,7 +134,7 @@ void ScheduledAction::execute(WorkerContext* workerContext) > > > > if (!m_function.IsEmpty() && m_function->IsFunction()) { > > v8::HandleScope handleScope; > > - v8::Local<v8::Context> v8Context = scriptController->proxy()->GetContext(); > > + v8::Local<v8::Context> v8Context = scriptController->proxy()->context(); > > Be careful, abarth is working on change to remove V8Proxy::context(), talk > w/him. > Per discussion, we keep it as context() since this is for WorkerContextExecutionProxy. > > > +v8::Local<v8::Function> V8DOMWrapper::getConstructor(V8ClassIndex::V8WrapperType type, v8::Handle<v8::Context> context) > > +{ > > + // Enter the scope for this context to get the correct constructor. > > + v8::Context::Scope scope(context); > > + > > + return getConstructor(type, V8Proxy::getHiddenObjectPrototype(context)); > > +} > > Can we have a better, more specific name for this method and perhaps make it > private? It shouldn't be used outside of this class. Fixed.
Dimitri Glazkov (Google)
Comment 8 2009-08-06 13:19:59 PDT
Comment on attachment 34221 [details] Proposed Patch r=me.
Jian Li
Comment 9 2009-08-06 14:32:17 PDT
Note You need to log in before you can comment on or make changes to this bug.