Bug 27737 – [V8] Remove parameterless frame/window retrieval methods from V8Proxy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dimitri Glazkov (Google)]

As part of cleaning up V8 bindings, I am removing parameterless retrieveFrame/retrieveWindow methods from V8Proxy to taper developers' temptation to use "the easiest-looking methods".

[Dimitri Glazkov (Google)]

Created attachment 33572 [details]
Remove parameterless frame/window retrieval methods, v1.

 WebCore/ChangeLog                                  |   34 ++++++++++++++++++++
 WebCore/bindings/v8/ScriptCallStack.cpp            |    2 +-
 WebCore/bindings/v8/V8NPUtils.cpp                  |    2 +-
 WebCore/bindings/v8/V8Proxy.cpp                    |   18 +---------
 WebCore/bindings/v8/V8Proxy.h                      |    5 ---
 WebCore/bindings/v8/custom/V8DatabaseCustom.cpp    |   30 +++++++----------
 .../v8/custom/V8HTMLAudioElementConstructor.cpp    |    8 +++-
 .../v8/custom/V8HTMLImageElementConstructor.cpp    |    8 +++-
 .../v8/custom/V8HTMLOptionElementConstructor.cpp   |    8 +++-
 .../v8/custom/V8MessageChannelConstructor.cpp      |    2 +-
 .../bindings/v8/custom/V8SQLTransactionCustom.cpp  |    2 +-
 WebCore/bindings/v8/custom/V8WorkerCustom.cpp      |    2 +-
 .../v8/custom/V8XMLHttpRequestConstructor.cpp      |   10 +++++-
 13 files changed, 80 insertions(+), 51 deletions(-)

[David Levin]

Comment on attachment 33572 [details]
Remove parameterless frame/window retrieval methods, v1.

Some minor things to fix on check-in.


> diff --git a/WebCore/bindings/v8/custom/V8DatabaseCustom.cpp b/WebCore/bindings/v8/custom/V8DatabaseCustom.cpp
> +    if (args.Length() == 0)

Since you're changing this line, may I suggest

 if (!args.Length())

> +        return throwError("Transaction callback is required.", V8Proxy::SyntaxError);
>  
> +        if (!args[1]->IsObject())
> +            throwError("Transaction success callback must be of valid type.");
Missing "return" before "throwError"




> diff --git a/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp b/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp
> index 8c447c6..a3e523c 100644
> --- a/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp
> +++ b/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp
> @@ -55,9 +55,15 @@ CALLBACK_FUNC_DECL(XMLHttpRequestConstructor)
>      WorkerContextExecutionProxy* proxy = WorkerContextExecutionProxy::retrieve();
>      if (proxy)
>          context = proxy->workerContext();
> -    else
> +    else {
> +#endif
> +        Frame* frame = V8Proxy::retrieveFrameForCurrentContext();
> +        if (!frame)
> +            return throwError("Frame associated with XMLHttpRequest constructor is not available.", V8Proxy::ReferenceError);

Why did you go with a different style of error message here?

This would match the format of the others:

 "XMLHttpRequest constructor associated frame is not available"

But actually I like the format in this one better.


Also, I think they would read better with a 's
 "XMLHttpRequest constructor's associated frame is not available"

[Dimitri Glazkov (Google)]

Thanks for review and comments, Dave!

Landed as http://trac.webkit.org/changeset/46435 with your comments addressed.