[webkit-reviews] review denied: [Bug 71968] Implement URL API : [Attachment 118687] Updated patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kaustubh Atrawalkar
<kaustubh at motorola.com>'s request for review:
Bug 71968: Implement URL API
https://bugs.webkit.org/show_bug.cgi?id=71968

Attachment 118687: Updated patch
https://bugs.webkit.org/attachment.cgi?id=118687&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118687&action=review


A few quick comments which are in no way comprehensive.

> Source/WebCore/html/DOMURL.cpp:36
> +#if ENABLE(BLOB)

Don't exclude headers using an"#if" (unless absolutely necessary in order to
keep some platforms building). I don't think that is the case here.

> Source/WebCore/html/DOMURL.cpp:58
> +	   : ContextDestructionObserver(scriptExecutionContext) { }

{} should be on the next lines.

> Source/WebCore/html/DOMURL.cpp:60
> +    virtual void contextDestroyed()

Should not be inlined in the class since we don't want this function inlined.

Note that this leaks in the same way the current class does.

contextDestroyed isn't guaranteed to be called before the destructor.

If the destructor is called first, then this clean up (for the registeries)
should run still.

> Source/WebCore/html/DOMURL.cpp:74
> +	   publicURLManagerMap().remove(context);

not threadsafe.

> Source/WebCore/html/DOMURL.cpp:92
> +    return staticPublicURLManagers;

Not threadsafe (which is bad because this can be used from workers).

> Source/WebCore/html/DOMURL.cpp:98
> +    OwnPtr<PublicURLManager>& manager = map.add(scriptExecutionContext,
nullptr).first->second;

not threadsafe.

> Source/WebCore/html/DOMURL.h:45
> +    static PassRefPtr<DOMURL> create(const String& url) { return
adoptRef(new DOMURL(url, emptyString())); }

Where are these called?

> Source/WebCore/html/DOMURL.idl:-32
> -	   NoStaticTables

Why is this being removed?

afaik, It needs to be here since this is exposed in Web Workers.