Bug 22448 – Create an abstraction for JSC::SourceCode

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Create an abstraction for JSC::SourceCode

FrameLoader::executeScript was modified to take a JSC::SourceCode parameter.  I propose defining a ScriptSourceCode class in bindings/js that is a thin wrapper around JSC::SourceCode.  This will allow us to abstract away JSC dependencies within a number of WebCore files, including FrameLoader.cpp, which will make it easier to support V8.

[Darin Fisher (:fishd, Google)]

Created attachment 25418 [details]
v1 patch

Hi Sam, since you introduced JSC::SourceCode into WebCore, I thought I should ask you to review this change.

From the JSC perspective, I think this change is nice because it means that you only need to include one header file (ScriptSourceCode.h) in order to get at the two different ways of calling makeSource.  I'm curious to know what you think.

[Darin Fisher (:fishd, Google)]

(I will add corresponding XCode project changes if everything else looks good.)

[Darin Fisher (:fishd, Google)]

Created attachment 25419 [details]
v2 patch

It turns out that more files were affected.

[Darin Fisher (:fishd, Google)]

Created attachment 25433 [details]
v2.1 patch - now with xcode changes

[Sam Weinig]

Comment on attachment 25433 [details]
v2.1 patch - now with xcode changes

> +++ bindings/js/ScriptController.cpp	(working copy)
> @@ -38,6 +38,7 @@
>  #include "PageGroup.h"
>  #include "PausedTimeouts.h"
>  #include "runtime_root.h"
> +#include "ScriptSourceCode.h"
>  #include "ScriptValue.h"
>  #include "Settings.h"
>  #include "StringSourceProvider.h"
This include can be removed.

> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (c) 2008, Google Inc.
> + * All rights reserved.
You probably want the All rights reserved on the same line.

> +
> +namespace WebCore {
> +
> +class ScriptSourceCode {
> +public:
> +    ScriptSourceCode(const String& source, const KURL& url = KURL(), int startLine = 1)
> +        : m_code(makeSource(source, url.isNull() ? String() : url.string(), startLine)) {}
> +    ScriptSourceCode(CachedScript* cs)
> +        : m_code(makeSource(cs)) {}
I like the braces to be on there own lines like.

+    ScriptSourceCode(const String& source, const KURL& url = KURL(), int startLine = 1)
+        : m_code(makeSource(source, url.isNull() ? String() : url.string(), startLine))
+    {
+    }



> +
> +    int length() const { return m_code.length(); }
Since the only use of this is for a null check, perhaps we can rename this to isEmpty().

> +
> +    const JSC::SourceCode& jsSourceCode() const { return m_code; }
> +
> +private:
> +    JSC::SourceCode m_code;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // ScriptSourceCode_h
> Index: bindings/js/WorkerScriptController.cpp
> ===================================================================
> --- bindings/js/WorkerScriptController.cpp	(revision 38710)
> +++ bindings/js/WorkerScriptController.cpp	(working copy)
> @@ -32,6 +32,8 @@
>  
>  #include "JSDOMBinding.h"
>  #include "JSWorkerContext.h"
> +#include "ScriptSourceCode.h"
> +#include "ScriptValue.h"
>  #include "WorkerContext.h"
>  #include "WorkerMessagingProxy.h"
>  #include "WorkerThread.h"
Presumably you can remove the #include <parser/SourceCode.h>

> Index: dom/ScriptElement.cpp
> ===================================================================
> --- dom/ScriptElement.cpp	(revision 38710)
> +++ dom/ScriptElement.cpp	(working copy)
> @@ -25,21 +25,19 @@
>  #include "ScriptElement.h"
>  
>  #include "CachedScript.h"
> -#include "CachedScriptSourceProvider.h"
>  #include "DocLoader.h"
>  #include "Document.h"
>  #include "Frame.h"
>  #include "FrameLoader.h"
>  #include "MIMETypeRegistry.h"
>  #include "ScriptController.h"
> +#include "ScriptSourceCode.h"
>  #include "ScriptValue.h"
>  #include "StringHash.h"
>  #include "StringSourceProvider.h"
Can we remove this #include too?

> +++ dom/WorkerThread.cpp	(working copy)
> @@ -31,14 +31,13 @@
>  #include "WorkerThread.h"
>  
>  #include "JSWorkerContext.h"
> -#include "StringSourceProvider.h"
> +#include "ScriptSourceCode.h"
> +#include "ScriptValue.h"
Why is this include necessary?

>  namespace WebCore {
>  
>  PassRefPtr<WorkerThread> WorkerThread::create(const KURL& scriptURL, const String& sourceCode, WorkerMessagingProxy* messagingProxy)

In the WorkerThread constructor we still have the line 
    , m_scriptURL(scriptURL.string().copy())

perhaps, we should add a KURL::copy() method to avoid reparsing.

> +++ dom/XMLTokenizer.cpp	(working copy)
> @@ -48,6 +48,7 @@
>  #include "ResourceRequest.h"
>  #include "ResourceResponse.h"
>  #include "ScriptController.h"
> +#include "ScriptSourceCode.h"
>  #include "ScriptValue.h"
>  #include "StringSourceProvider.h"
>  #include "TextResourceDecoder.h"
You can remove #include "CachedScriptSourceProvider.h" here.

> +++ loader/FrameLoader.cpp	(working copy)
> @@ -80,6 +80,7 @@
>  #include "ResourceHandle.h"
>  #include "ResourceRequest.h"
>  #include "ScriptController.h"
> +#include "ScriptSourceCode.h"
>  #include "ScriptValue.h"
>  #include "SecurityOrigin.h"
>  #include "SegmentedString.h"
You can remove #include "StringSourceProvider.h" here.

r=me, but please consider some of these changes.

[Mark Rowe (bdash)]

Comment on attachment 25433 [details]
v2.1 patch - now with xcode changes

> Index: bindings/js/ScriptSourceCode.h
> ===================================================================
> --- bindings/js/ScriptSourceCode.h	(revision 0)
> +++ bindings/js/ScriptSourceCode.h	(revision 0)
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (c) 2008, Google Inc.
> + * All rights reserved.
> + * 
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + * 
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + * 
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

This license header looks a little bit untidy.  Can we please move the "All rights reserved" onto the same line as the copyright notice and fix the indentation of the list.  See <http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/Threading.h#L1> for a tidier interpretation.

I'm not entirely sure that the third clause of the license makes sense in this context either: what exactly is a contributor to Google Inc.?  You may want to consider using the two-clause license, which is what Apple uses for new WebKit code such as in <http://trac.webkit.org/browser/trunk/WebCore/storage/Storage.h#L1>.

[Darin Fisher (:fishd, Google)]

Created attachment 25443 [details]
v2.2 patch - revised per review

Revised per review feedback.  Nothing exciting.  I will add KURL::copy() in a separate change list.

[Darin Fisher (:fishd, Google)]

Thanks for the observations Mark.  I will get back to you.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38729

As discussed on #webkit with bdash, I will revise the license headers in a separate change list (so that I can update all of them together) once I have had a chance to figure out what the header should change to.

[Darin Fisher (:fishd, Google)]

Qt build bustage fixed:
http://trac.webkit.org/changeset/38731