[webkit-reviews] review granted: [Bug 41547] Turn on adoptRef assertion for RefCounted : [Attachment 60415] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 41547: Turn on adoptRef assertion for RefCounted
https://bugs.webkit.org/show_bug.cgi?id=41547

Attachment 60415: Patch
https://bugs.webkit.org/attachment.cgi?id=60415&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
A bunch of minor comments below.  Really glad to see this patch!  Thanks for
picking up this project and running with it.

WebCore/html/FileStreamProxy.cpp:60
 +	proxy->ref();
This seems like a bad design.  The way we solved this problem in Chrome is by
having specialized Task objects that understood how to ref/deref the objects
that they operated upon.  It took a few iterations of the design to get it
right, but we got rid of almost all the instances of this (error-prone)
pattern.

WebCore/page/EventSource.cpp:71
 +  PassRefPtr<EventSource> EventSource::create(const String& url,
ScriptExecutionContext* context, ExceptionCode& ec)
This change is slightly complex, but looks like a nice win in terms of
readability.

WebCore/page/EventSource.h: 
 +	    EventSource(const String& url, ScriptExecutionContext* context,
ExceptionCode& ec);
Yeah, having the ec in the constructor seems like nonsense semantically. 
Constructors that can fail don't make much sense.

WebCore/storage/StorageAreaSync.h:44
 +	    static PassRefPtr<StorageAreaSync>
create(PassRefPtr<StorageSyncManager>, PassRefPtr<StorageAreaImpl>, const
String& databaseIdentifier);
We should improve the linter to find Strings passed by value.

WebCore/workers/SharedWorker.cpp: 
 +	ASSERT(!ec);
What happend to this assert?

WebCore/workers/SharedWorker.cpp:60
 +	KURL scriptURL = worker->resolveURL(url, ec);
Can we get an ec without scriptURL being empty?  Do we need to return like the
old code did?

WebCore/workers/Worker.cpp:63
 +	KURL scriptURL = worker->resolveURL(url, ec);
Same ec question.

WebCore/workers/Worker.cpp:67
 +	worker->m_scriptLoader = new
WorkerScriptLoader(ResourceRequestBase::TargetIsWorker);
Naked new?

WebCore/workers/Worker.h:57
 +	    virtual ~Worker();
!