WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
27997
[V8] Style cleaning for WorkerContextExecutionProxy.
https://bugs.webkit.org/show_bug.cgi?id=27997
Summary
[V8] Style cleaning for WorkerContextExecutionProxy.
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
Details
Formatted Diff
Diff
Add info to ChangeLog
(24.17 KB, patch)
2009-08-04 21:32 PDT
,
Jian Li
dglazkov
: review-
Details
Formatted Diff
Diff
Proposed Patch
(24.32 KB, patch)
2009-08-06 13:18 PDT
,
Jian Li
dglazkov
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed as
http://trac.webkit.org/changeset/46859
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug