[webkit-reviews] review denied: [Bug 31569] Initial implementation of WebKitSharedScript and SharedScriptContext : [Attachment 43737] Patch updated for the name change.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 31569: Initial implementation of WebKitSharedScript and SharedScriptContext
https://bugs.webkit.org/show_bug.cgi?id=31569

Attachment 43737: Patch updated for the name change.
https://bugs.webkit.org/attachment.cgi?id=43737&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +	   Initial implementation of WebKitSharedScript and SharedScriptContext

> +	   https://bugs.webkit.org/show_bug.cgi?id=31569
> +
> +	   No new tests since there is no bindings yet (soon to come).

s/is/are/

> +	   * workers/WorkerScriptLoader.cpp:
> +	   (WebCore::WorkerScriptLoader::loadSynchronously): Changed ASSERT
because this loader can work with SharedScript as well.

Then the class is now misnamed.

> diff --git a/WebCore/shared-script/SharedScriptContext.cpp
b/WebCore/shared-script/SharedScriptContext.cpp

regarding "shared-script" -- There seem to be a mix of directory names either
NamedLikeThis or like-this (e.g. WebCore, WebKitTools), but in my cursory
examination NamedLikeThis seems more common.

> +#include "config.h"
> +
> +#if ENABLE(SHARED_SCRIPT)
> +
> +#include "SharedScriptContext.h"

> +#include "SharedScriptController.h"

This one is out of place.

> +#include "Event.h"
> +#include "EventException.h"
> +#include "NotImplemented.h"
> +#include "ScriptSourceCode.h"
> +#include "scriptValue.h"

Should be ScriptValue.h

> +#include "SecurityOrigin.h"
> +#include "WebKitSharedScriptRepository.h"
> +
> +namespace WebCore {
> +
> +// Interval to allow orphaned SharedScriptContext to gain potential new
owner.
> +// This can happen on navigation when both pages connect to the same
SharedScript.
> +static const double destructionInterval = 5.0;

Is this part of the feature in dispute right now?

> +SharedScriptContext::SharedScriptContext(const String& name, const KURL&
url, PassRefPtr<SecurityOrigin> origin)
> +    : m_name(name)
> +    , m_url(url)
> +    , m_script(new SharedScriptController(this))
> +    , m_destructionTimer(this, &SharedScriptContext::destructionTimerFired)
> +    , m_loaded(false)
> +{
> +    setSecurityOrigin(origin);
> +    ref(); // Matching deref is in destructionTimer callback.

s/destructionTimer/destructionTimerFired/ ?

> +void SharedScriptContext::addMessage(MessageDestination destination,
MessageSource source, MessageType type, MessageLevel level, const String&
message, unsigned lineNumber, const String& sourceURL)
> +{
> +    // FIXME: figure out console/incspector story for SharedScript. Maybe
similar to SharedWorkers.

typo: incspector (throughout).

> +bool SharedScriptContext::matches(const String& name,
PassRefPtr<SecurityOrigin> origin, const KURL& urlToMatch) const

Since this method doesn't take ownership of origin, it should be a
SecurityOrigin* instead of a PassRefPtr<SecurityOrigin>.
Actually SecurityOrigin& would be even better since this method always wants it
to be non-0.

> +{
...
> +    // If the names are both empty, compares the URLs instead per the Web
Workers spec.

s/per/like/
I feel that you don't need to do something per the Web Workers spec since this
isn't Web Workers but it is good to have a consistent pattern.
(It should be per the SharedScript spec.)

> +void SharedScriptContext::addToDocumentsList(ScriptExecutionContext*
context)

Why does it say Document but take a ScriptExecutionContext?

I guess you don't need everything a document provides and the SEC is sufficient
but it seems like a mixed message. I guess you could take a Document* but store
it as an SEC.

> +void SharedScriptContext::removeFromDocumentList(ScriptExecutionContext*
context)

Why not only start the timer when "if (!m_documentList.size())"?
> +    m_destructionTimer.startOneShot(destructionInterval);

> +void SharedScriptContext::postTask(PassOwnPtr<Task> task)
> +{
> +    // FIXME: Need to implement ScriptExecutionContext::postTaskToMainThread
to share between Document and SharedScritpContext.

typo: SharedScritpContext.

> +ScriptExecutionContext* SharedScriptContext::scriptExecutionContext() const

Why is this const? It doesn't look like a const operation?

> +{
> +    return const_cast<SharedScriptContext*>(this);
> +}

> diff --git a/WebCore/shared-script/SharedScriptContext.h
b/WebCore/shared-script/SharedScriptContext.h

> +namespace WebCore {
> +
> +class Document;
> +class SharedScriptController;
> +class KURL;

KURL is out of sort order.

> +class String;

> +class SharedScriptContext : public RefCounted<SharedScriptContext>, public
ScriptExecutionContext, public EventTarget {
> +public:

> +    virtual ~SharedScriptContext();
Does the destructor need to be public? (Since this is ref counted, I expect
that it should be either private or protected ).

> +    // JS wrapper and EventTaret support.

typo: EventTaret

> +    bool matches(const String& name, PassRefPtr<SecurityOrigin> origin,
const KURL& urlToMatch) const;

remove param name "origin"


> diff --git a/WebCore/shared-script/WebKitSharedScript.cpp
b/WebCore/shared-script/WebKitSharedScript.cpp

> +#include "Event.h"
> +#include "EventException.h"
> +#include "SharedScriptContext.h"

#include "Shared.. is out of order.

> +WebKitSharedScript::WebKitSharedScript(const String& url, const String&
name, ScriptExecutionContext* context, ExceptionCode& ec)

> +    // FIXME: This should use the dynamic global scope (bug #27887)

Add "." to end of comment.

> diff --git a/WebCore/shared-script/WebKitSharedScript.h
b/WebCore/shared-script/WebKitSharedScript.h

> +	   void setContext(PassRefPtr<SharedScriptContext> context);
> +
> +	   // When fired, will set the innerContext into this
WebKitSharedScript and dispatch 'load' event.

s/will/this will/


> diff --git a/WebCore/shared-script/WebKitSharedScriptRepository.cpp
b/WebCore/shared-script/WebKitSharedScriptRepository.cpp

> +#include "ActiveDOMObject.h"
> +#include "Document.h"
> +#include "Event.h"
> +#include "EventNames.h"
> +#include "ExceptionCode.h"
> +#include "SharedScriptContext.h"

Shared... is out of order.

> +#include "SecurityOrigin.h"

> +// Helper class to load the initial script on behalf of a SharedScript.
> +class ScriptLoader : public RefCounted<ScriptLoader>, public
ActiveDOMObject, private WorkerScriptLoaderClient {

Should this be "SharedScriptLoader"? It doesn't load all scripts, so
ScriptLoader seems a little too broad.

> +void ScriptLoader::notifyFinished()
> +{
> +    if (m_scriptLoader->failed())
> +	   m_sharedScript->dispatchEvent(Event::create(eventNames().errorEvent,
false, true));
> +    else {
> +	   // If another loader have not yet initialized the
SharedScriptContext - do so.

s/have/has/
s/-/,/

> +    m_sharedScript->unsetPendingActivity(m_sharedScript.get());
> +    unsetPendingActivity(this); // This frees this object - must be the last
action in this function.

s/-/so it/

> +void WebKitSharedScriptRepository::documentDetached(Document* document)
> +{
> +    UNUSED_PARAM(document);

document is used.

> +    WebKitSharedScriptRepository& repository = instance();
> +    for (unsigned i = 0; i < repository.m_sharedScriptContexts.size(); i++)
> +	  
repository.m_sharedScriptContexts[i]->removeFromDocumentList(document);
> +}

> +void
WebKitSharedScriptRepository::connectToSharedScript(PassRefPtr<WebKitSharedScri
pt> sharedScript, const KURL& url, const String& name, ExceptionCode& ec)
> +{
> +   
ASSERT(sharedScript->scriptExecutionContext()->securityOrigin()->canAccess(Secu
rityOrigin::create(url).get()));
> +    RefPtr<SharedScriptContext> innerContext = getSharedScriptContext(name,
url);
> +
> +   
innerContext->addToDocumentsList(sharedScript->scriptExecutionContext());

Should this document get added before the url check is done?

> +    if (innerContext->url() != url) {
> +	   // SharedScript with same name but different URL already exists -
return an error.
> +	   ec = URL_MISMATCH_ERR;
> +	   return;
> +    }
> +    // If SharedScriptContext is already running, just schedule a load event
- otherwise, kick off a loader to load the script.
> +    if (innerContext->loaded())
> +	   sharedScript->scheduleLoadEvent(innerContext);
> +    else {
> +	   RefPtr<ScriptLoader> loader = adoptRef(new
ScriptLoader(sharedScript, innerContext.release()));

Ideally there would be a create method (to match the general pattern followed
for RefCounted objects).

> +	   loader->load(url); // Pending activity will keep the loader alive.

This has the potential to do multiple loads of the same url for the same
SharedScript. Then, it could fail for the first one and succeed later for
another, which leaves me confused. A document gets some notification that the
SharedScript failed to load but then it does load.

It would be better to only do one load. (You could track this by changing from
a bool to track "loaded" to and enum to track when the laod starts as well or
when it failed. It seems that if a shared script fails to load, then another
document tries to get it, it shouldn't just appear.


> diff --git a/WebCore/shared-script/WebKitSharedScriptRepository.h
b/WebCore/shared-script/WebKitSharedScriptRepository.h

> +namespace WebCore {
> +
> +typedef int ExceptionCode;
> +
> +class Document;
> +class KURL;
> +class SharedScriptContext;
> +class WebKitSharedScript;
> +class String;

Please sort.