Summary: | [Chromium] Adds EventSource bindings for V8 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marcus Bulach <bulach> | ||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, japhet, michaeln, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Marcus Bulach
2010-01-14 15:53:25 PST
Created attachment 46619 [details]
Patch
Attachment 46619 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/186791 Comment on attachment 46619 [details]
Patch
Breaking Chromium would be bad news bears. Does this depend on some other fix?
Created attachment 46626 [details]
Patch
Ops, my bad, sorry! Forgot to add the #if ENABLE guards in the new cpp files.
Fixed, uploading again.
Comment on attachment 46626 [details] Patch A couple of small nits... In addition to the inline comments, I think you need to add the new files to WebCore/Android.v8bindings.mk as well. > Index: ChangeLog > =================================================================== > --- ChangeLog (revision 53295) > +++ ChangeLog (working copy) > @@ -1,3 +1,25 @@ > +2010-01-14 Marcus Bulach <bulach@chromium.org> Remove the garbage before the date. > + * ../../WebCore.gypi: > + * ../../bindings/scripts/CodeGeneratorV8.pm: > + * ../../bindings/v8/DOMObjectsInclude.h: > + * ../../bindings/v8/DerivedSourcesAllInOne.cpp: > + * ../../bindings/v8/V8DOMWrapper.cpp: > + * ../../bindings/v8/V8DOMWrapper.h: > + * ../../bindings/v8/V8Index.cpp: > + * ../../bindings/v8/V8Index.h: > + * ../../bindings/v8/WorkerContextExecutionProxy.cpp: > + * ../../bindings/v8/custom/V8CustomBinding.h: > + * ../../bindings/v8/custom/V8EventSourceConstructor.cpp: Added. > + * ../../bindings/v8/custom/V8EventSourceCustom.cpp: Added. Remove the leading ../../ > +#if ENABLE(EVENTSOURCE) > + case V8ClassIndex::EVENTSOURCE: { > + descriptor->SetCallHandler(USE_CALLBACK(EventSourceConstructor)); > + break; > + } > +#endif Add this block down with the other SetCallHandler() calls. > + * Copyright 2009, The Android Open Source Project Here and in V8EventSourceConstructor.cpp, 2010 > +using namespace std; > +using namespace WTF; I don't think these are necessary? Comment on attachment 46626 [details]
Patch
r- based on Nate's comments.
Created attachment 46692 [details]
Patch
Addressing Nate's comments.
Thanks Nate! All comments addressed, would you please take another look? (In reply to comment #5) > (From update of attachment 46626 [details]) > A couple of small nits... > > In addition to the inline comments, I think you need to add the new files to > WebCore/Android.v8bindings.mk as well. Added. > > > Index: ChangeLog > > =================================================================== > > --- ChangeLog (revision 53295) > > +++ ChangeLog (working copy) > > @@ -1,3 +1,25 @@ > > +2010-01-14 Marcus Bulach <bulach@chromium.org> > > Remove the garbage before the date. ouch, sorry, some editor added the UTF8 BOM... removed. > > > + * ../../WebCore.gypi: > > + * ../../bindings/scripts/CodeGeneratorV8.pm: > > + * ../../bindings/v8/DOMObjectsInclude.h: > > + * ../../bindings/v8/DerivedSourcesAllInOne.cpp: > > + * ../../bindings/v8/V8DOMWrapper.cpp: > > + * ../../bindings/v8/V8DOMWrapper.h: > > + * ../../bindings/v8/V8Index.cpp: > > + * ../../bindings/v8/V8Index.h: > > + * ../../bindings/v8/WorkerContextExecutionProxy.cpp: > > + * ../../bindings/v8/custom/V8CustomBinding.h: > > + * ../../bindings/v8/custom/V8EventSourceConstructor.cpp: Added. > > + * ../../bindings/v8/custom/V8EventSourceCustom.cpp: Added. > > Remove the leading ../../ done > > > +#if ENABLE(EVENTSOURCE) > > + case V8ClassIndex::EVENTSOURCE: { > > + descriptor->SetCallHandler(USE_CALLBACK(EventSourceConstructor)); > > + break; > > + } > > +#endif > > Add this block down with the other SetCallHandler() calls. moved > > > + * Copyright 2009, The Android Open Source Project > > Here and in V8EventSourceConstructor.cpp, 2010 fixed. > > > +using namespace std; > > +using namespace WTF; > > I don't think these are necessary? removed. nice to see the v8 binding for this feature going in :) thanks Michael! Nate: would you mind taking another look, please? (In reply to comment #10) > thanks Michael! > > Nate: would you mind taking another look, please? LGTM Really sorry for the delay :-( Would someone with r+ please approve this? Comment on attachment 46692 [details]
Patch
aight.
Comment on attachment 46692 [details] Patch Clearing flags on attachment: 46692 Committed r53931: <http://trac.webkit.org/changeset/53931> All reviewed patches have been landed. Closing bug. |