Bug 61536 – [Chromium] Add missing compile guards for WebWorkers in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Leandro Graciá Gil]

Some files in WebKit Chromium lack the appropriate #if ENABLED(WORKERS). This causes some files to fail when compiling if this feature is disabled.

[Leandro Graciá Gil]

Created attachment 94992 [details]
Patch

[Dmitry Titov]

Comment on attachment 94992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94992&action=review

Great! Couple of comments, lets do another iteration:

> Source/WebKit/chromium/src/DatabaseObserver.cpp:71
> +        return false;

Lets add ASSERT_NOT_REACHED() here, to document that we don't expect any other case but Document in this case.

> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:101
> +        allowed = false;

Same as above, ASSERT_NOT_REACHED() could be useful here.

> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:96
> +    m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path));

This duplicates a line of code... By placing #if/#endif around if/else, it's possible to avoid that.

[Leandro Graciá Gil]

Comment on attachment 94992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94992&action=review

>> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:96
>> +    m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path));
> 
> This duplicates a line of code... By placing #if/#endif around if/else, it's possible to avoid that.

Yes, but it may introduce style errors in the bots that don't enable workers. If you think this is ok I'll be happy to fix it like you propose.

[David Levin]

Comment on attachment 94992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94992&action=review

>>> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:96
>>> +    m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path));
>> 
>> This duplicates a line of code... By placing #if/#endif around if/else, it's possible to avoid that.
> 
> Yes, but it may introduce style errors in the bots that don't enable workers. If you think this is ok I'll be happy to fix it like you propose.

This could do 

OwnPtr<WebFileSystemCallbacksImpl> callbacks(this);

#if
if (m_context && m_context->isWorkerContext()) {
       m_callbacks->didOpenFileSystem(name, WorkerAsyncFileSystemChromium::create(m_context, m_type, path, m_synchronous));
   return;
}
#endif
m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path));

which addresses both issues.

[Leandro Graciá Gil]

Created attachment 95015 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 94992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94992&action=review

>> Source/WebKit/chromium/src/DatabaseObserver.cpp:71
>> +        return false;
> 
> Lets add ASSERT_NOT_REACHED() here, to document that we don't expect any other case but Document in this case.

Fixed.

>> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:101
>> +        allowed = false;
> 
> Same as above, ASSERT_NOT_REACHED() could be useful here.

Fixed.

>>>> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:96
>>>> +    m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path));
>>> 
>>> This duplicates a line of code... By placing #if/#endif around if/else, it's possible to avoid that.
>> 
>> Yes, but it may introduce style errors in the bots that don't enable workers. If you think this is ok I'll be happy to fix it like you propose.
> 
> This could do 
> 
> OwnPtr<WebFileSystemCallbacksImpl> callbacks(this);
> 
> #if
> if (m_context && m_context->isWorkerContext()) {
>        m_callbacks->didOpenFileSystem(name, WorkerAsyncFileSystemChromium::create(m_context, m_type, path, m_synchronous));
>    return;
> }
> #endif
> m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path));
> 
> which addresses both issues.

Thanks for the suggestion! Fixed.

However I just realized a compile error here when uploading the patch. Give me a minute and I'll fix it.

[Leandro Graciá Gil]

Created attachment 95018 [details]
Patch

Fixing build error caused for not using adoptPtr.

[Dmitry Titov]

Comment on attachment 95018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95018&action=review

One small thing:

> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:99
>      delete this;

I think this 'delete' is not needed now since there is OwnPtr for 'this'... While at this, lets add a comment to the OwnPtr about this object self-deleting on exit, to make it more explicit.

[Leandro Graciá Gil]

Created attachment 95020 [details]
Patch

Performing review fixes.

[Leandro Graciá Gil]

Comment on attachment 95018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95018&action=review

>> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:99
>>      delete this;
> 
> I think this 'delete' is not needed now since there is OwnPtr for 'this'... While at this, lets add a comment to the OwnPtr about this object self-deleting on exit, to make it more explicit.

Done.

[Dmitry Titov]

Comment on attachment 95020 [details]
Patch

You could just always do cq? with r? if you intend to use commit queue. Reviewer can flip both at one step if appropriate then.

[Leandro Graciá Gil]

(In reply to comment #11)
> (From update of attachment 95020 [details])
> You could just always do cq? with r? if you intend to use commit queue. Reviewer can flip both at one step if appropriate then.

I will, at least once the code seems ready to pass.
Thanks for the advice.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 95020 [details]:

inspector/debugger/debugger-scripts.html bug 59921 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 95020 [details]
Patch

Clearing flags on attachment: 95020

Committed r87463: <http://trac.webkit.org/changeset/87463>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.