RESOLVED FIXED 33695
[Chromium] Adds EventSource bindings for V8
https://bugs.webkit.org/show_bug.cgi?id=33695
Summary [Chromium] Adds EventSource bindings for V8
Marcus Bulach
Reported 2010-01-14 15:53:25 PST
Adds EventSource bindings for V8 Must pass existing LayoutTests: * LayoutTests/fast/eventsource * LayoutTests/http/tests/eventsource
Attachments
Patch (17.03 KB, patch)
2010-01-14 16:29 PST, Marcus Bulach
eric: review-
Patch (17.15 KB, patch)
2010-01-14 17:54 PST, Marcus Bulach
dglazkov: review-
Patch (17.50 KB, patch)
2010-01-15 11:28 PST, Marcus Bulach
no flags
Marcus Bulach
Comment 1 2010-01-14 16:29:44 PST
WebKit Review Bot
Comment 2 2010-01-14 16:48:00 PST
Eric Seidel (no email)
Comment 3 2010-01-14 17:18:05 PST
Comment on attachment 46619 [details] Patch Breaking Chromium would be bad news bears. Does this depend on some other fix?
Marcus Bulach
Comment 4 2010-01-14 17:54:03 PST
Created attachment 46626 [details] Patch Ops, my bad, sorry! Forgot to add the #if ENABLE guards in the new cpp files. Fixed, uploading again.
Nate Chapin
Comment 5 2010-01-15 09:20:25 PST
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?
Dimitri Glazkov (Google)
Comment 6 2010-01-15 09:22:51 PST
Comment on attachment 46626 [details] Patch r- based on Nate's comments.
Marcus Bulach
Comment 7 2010-01-15 11:28:09 PST
Created attachment 46692 [details] Patch Addressing Nate's comments.
Marcus Bulach
Comment 8 2010-01-15 11:30:02 PST
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.
Michael Nordman
Comment 9 2010-01-21 19:35:54 PST
nice to see the v8 binding for this feature going in :)
Marcus Bulach
Comment 10 2010-01-26 04:44:29 PST
thanks Michael! Nate: would you mind taking another look, please?
Nate Chapin
Comment 11 2010-01-26 17:54:20 PST
(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?
Dimitri Glazkov (Google)
Comment 12 2010-01-26 19:18:42 PST
Comment on attachment 46692 [details] Patch aight.
WebKit Commit Bot
Comment 13 2010-01-27 06:41:48 PST
Comment on attachment 46692 [details] Patch Clearing flags on attachment: 46692 Committed r53931: <http://trac.webkit.org/changeset/53931>
WebKit Commit Bot
Comment 14 2010-01-27 06:41:56 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.