WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch
(17.15 KB, patch)
2010-01-14 17:54 PST
,
Marcus Bulach
dglazkov
: review-
Details
Formatted Diff
Diff
Patch
(17.50 KB, patch)
2010-01-15 11:28 PST
,
Marcus Bulach
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Marcus Bulach
Comment 1
2010-01-14 16:29:44 PST
Created
attachment 46619
[details]
Patch
WebKit Review Bot
Comment 2
2010-01-14 16:48:00 PST
Attachment 46619
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/186791
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.
Top of Page
Format For Printing
XML
Clone This Bug