[webkit-reviews] review denied: [Bug 88338] IndexedDB: should make the LevelDB persistant to the directory indicated in PageGroupSettings::indexedDBDataBasePath : [Attachment 149924] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Charles Wei
<charles.wei at torchmobile.com.cn>'s request for review:
Bug 88338: IndexedDB: should make the LevelDB persistant to the directory
indicated in PageGroupSettings::indexedDBDataBasePath
https://bugs.webkit.org/show_bug.cgi?id=88338

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

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


> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:97
> +	   m_backend->open(name, request.get(), context->securityOrigin(),
frame, document->page()->group().groupSettings()->indexedDBDatabasePath());

Do we need to add a check for page() being null here?

> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:173
> +    return document->page()->group().groupSettings();

Why don't you need to check for page being null here?

> Source/WebCore/workers/WorkerMessagingProxy.cpp:276
> +    Document* document =
static_cast<Document*>(m_scriptExecutionContext.get());

You should add an assert that it is a document. When nested workers are
supported one day, it would be nice to get asserts in places immediately where
there are problems.

ASSERT(m_scriptExecutionContext->isDocument());

> Source/WebCore/workers/WorkerMessagingProxy.cpp:279
> +	   settings = document->page()->group().groupSettings();

How do we know if page is not null that group is not null.

> Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:372
> +    setWorkerThread(SharedWorkerThread::create(name, url, userAgent,
static_cast<Document*>(m_loadingDocument.get())->page()->group().groupSettings(
),

Why is there no check for page() being null here?

(And add an assert about this being a document.)

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:93
> +    GroupSettings* settings =
static_cast<Document*>(m_scriptExecutionContext.get())->page()->group().groupSe
ttings();

Why is there no check for page being null here?