Bug 33695

Summary: [Chromium] Adds EventSource bindings for V8
Product: WebKit Reporter: Marcus Bulach <bulach>
Component: PlatformAssignee: 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 Flags
Patch
eric: review-
Patch
dglazkov: review-
Patch none

Description Marcus Bulach 2010-01-14 15:53:25 PST
Adds EventSource bindings for V8
Must pass existing LayoutTests:
* LayoutTests/fast/eventsource
* LayoutTests/http/tests/eventsource
Comment 1 Marcus Bulach 2010-01-14 16:29:44 PST
Created attachment 46619 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Eric Seidel (no email) 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?
Comment 4 Marcus Bulach 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.
Comment 5 Nate Chapin 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?
Comment 6 Dimitri Glazkov (Google) 2010-01-15 09:22:51 PST
Comment on attachment 46626 [details]
Patch

r- based on Nate's comments.
Comment 7 Marcus Bulach 2010-01-15 11:28:09 PST
Created attachment 46692 [details]
Patch

Addressing Nate's comments.
Comment 8 Marcus Bulach 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.
Comment 9 Michael Nordman 2010-01-21 19:35:54 PST
nice to see the v8 binding for this feature going in :)
Comment 10 Marcus Bulach 2010-01-26 04:44:29 PST
thanks Michael!

Nate: would you mind taking another look, please?
Comment 11 Nate Chapin 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?
Comment 12 Dimitri Glazkov (Google) 2010-01-26 19:18:42 PST
Comment on attachment 46692 [details]
Patch

aight.
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2010-01-27 06:41:56 PST
All reviewed patches have been landed.  Closing bug.