Bug 27737 - [V8] Remove parameterless frame/window retrieval methods from V8Proxy
Summary: [V8] Remove parameterless frame/window retrieval methods from V8Proxy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-27 15:05 PDT by Dimitri Glazkov (Google)
Modified: 2009-07-27 15:40 PDT (History)
1 user (show)

See Also:


Attachments
Remove parameterless frame/window retrieval methods, v1. (14.91 KB, patch)
2009-07-27 15:13 PDT, Dimitri Glazkov (Google)
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2009-07-27 15:05:23 PDT
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".
Comment 1 Dimitri Glazkov (Google) 2009-07-27 15:13:46 PDT
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(-)
Comment 2 David Levin 2009-07-27 15:28:28 PDT
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"
Comment 3 Dimitri Glazkov (Google) 2009-07-27 15:40:41 PDT
Thanks for review and comments, Dave!

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