[webkit-reviews] review granted: [Bug 22448] Create an abstraction for JSC::SourceCode : [Attachment 25433] v2.1 patch - now with xcode changes

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Fisher (Google)
<darin at chromium.org>'s request for review:
Bug 22448: Create an abstraction for JSC::SourceCode
https://bugs.webkit.org/show_bug.cgi?id=22448

Attachment 25433: v2.1 patch - now with xcode changes
https://bugs.webkit.org/attachment.cgi?id=25433&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
> +++ 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.