Bug 25084 – Upstream changes to support V8 bindings for XHR in worker context.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jian Li]

Upstream changes to support V8 bindings for XHR in worker context.

[Jian Li]

Created attachment 29319 [details]
Proposed Patch

[Eric Seidel (no email)]

Comment on attachment 29319 [details]
Proposed Patch

A more descriptive ChangeLog would be helpful.

It's strange to leave "FindOrCreate..." with a leading capital letter.

We use 0 instead of NULL in WebKit C++ code, see http://webkit.org/coding/coding-style.html

A comment documenting when it's OK for global.IsEmpty() to return true would be nice.  Maybe there is still an ASSERT we should add there?  Or is it always true that a dom wrapper lookup can fail for the worker context now?

findOrCreateEventListenerHelper
Helper isn't very descriptive.  Why are these helpers and not listeners?  Don't they inherit from V8EventListener?

[Jian Li]

Created attachment 29342 [details]
Proposed Patch

[Jian Li]

(In reply to comment #2)
> (From update of attachment 29319 [details] [review])
> A more descriptive ChangeLog would be helpful.
> 
What do I need to add to ChangeLog. Maybe something like the following:
  Upstream changes to V8 bindings for XHR so that it can work under either DOMWindow or WorkerContext.

How do you think? If agreed, I will update the ChangeLog.
> It's strange to leave "FindOrCreate..." with a leading capital letter.
> 
Yes, this is strange but I have to keep that unchanged because all these FindOrCreate*** are used by some files that have not been upstreamed yet (you can see the comment there).

For the new methods I have added, I use the WebKit style name.

> We use 0 instead of NULL in WebKit C++ code, see
> http://webkit.org/coding/coding-style.html
> 
Thanks for reminding. Fixed.
> A comment documenting when it's OK for global.IsEmpty() to return true would be
> nice.  Maybe there is still an ASSERT we should add there?  Or is it always
> true that a dom wrapper lookup can fail for the worker context now?
> 
I added the comment in .h file. I think we should not have ASSERT for WorkerContextExecutionProxy::retrieve() because we rely on it to return 0 to indicate if it is in worker context.
> findOrCreateEventListenerHelper
> Helper isn't very descriptive.  Why are these helpers and not listeners?  Don't
> they inherit from V8EventListener?
> 
I agree that Helper is not really a good name. Do you have any better name to suggest? The reason for using this Helper is to put the common code in this Helper so that it could be called by either FindOrCreateEventListener or findOrCreateObjectEventListener.

V8WorkerContextEventListener inherits from V8EventListener and V8WorkerContextObjectEventListener inherits from V8WorkerContextEventListener. They could only be found or created by the methods in WorkerContextExecutionProxy.

[Eric Seidel (no email)]

Comment on attachment 29342 [details]
Proposed Patch

Seems OK to me.  I'm not the be-all, end-all expert here, but this also looks like an "intermediate step" towards better bindings, since not all is upstreamed yet.  Having commetns closer to the actual logic would be more helpful in some case, like the if (global.isEmpty()) having a comment right before it saying *why* we return 0 there.

Or having a comment right next to the funny function names Find vs. find in the .cpp file which explains that the Find version shoudl be removed ASAP.

In general, looks fine to my eyes.

[Jian Li]

Created attachment 29373 [details]
Proposed Patch

This is the new patch with one more comment added per last feedback.

[Dimitri Glazkov (Google)]

Comment on attachment 29373 [details]
Proposed Patch

Looks good.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/42373