Bug 156713

Summary: [Win][IndexedDB] Fix build errors.
Product: WebKit Reporter: peavo
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description peavo 2016-04-18 13:23:29 PDT
There are currently a few compile and link errors when building WinCairo with IndexedDB enabled.
Comment 1 peavo 2016-04-18 13:58:04 PDT
Created attachment 276664 [details]
Patch
Comment 2 peavo 2016-04-18 14:16:19 PDT
Created attachment 276667 [details]
Patch
Comment 3 WebKit Commit Bot 2016-04-18 14:17:58 PDT
Attachment 276667 [details] did not pass style-queue:


ERROR: Source/WebCore/PlatformWin.cmake:189:  There should be exactly one empty line instead of 0 between "Modules/indexeddb" and "Modules/indexeddb/client".  [list/emptyline] [5]
ERROR: Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1188:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Alex Christensen 2016-04-18 14:50:58 PDT
Comment on attachment 276667 [details]
Patch

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

cool!

> Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:44
> -    return std::make_unique<MemoryBackingStoreTransaction>(backingStore, info);
> +    return std::unique_ptr<MemoryBackingStoreTransaction>(new MemoryBackingStoreTransaction(backingStore, info));

This shouldn't be needed.

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:48
> -    return std::make_unique<MemoryIDBBackingStore>(identifier);
> +    return std::unique_ptr<MemoryIDBBackingStore>(new MemoryIDBBackingStore(identifier));

This shouldn't be needed.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1188
> -    auto callback = [this, self, refTransaction](const IDBError& error) {
> +    ErrorCallback callback = [this, self, refTransaction](const IDBError& error) {

This shouldn't be needed.

> Source/WebKit/win/storage/WebDatabaseProvider.cpp:45
> +    sprintf_s(databaseDirectory, MAX_PATH, "%s\\%s", appDataDirectory, executableName);

On Mac we use .../WebKit/Databases/___IndexedDB.  We should probably do something similar here.
Comment 5 Alex Christensen 2016-04-18 14:55:17 PDT
Comment on attachment 276667 [details]
Patch

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

> Source/WebKit/win/WebView.cpp:5062
> +    RuntimeEnabledFeatures::sharedFeatures().setWebkitIndexedDBEnabled(true);

This should probably be false by default, then set to true for DumpRenderTree and MiniBrowser.
Comment 6 peavo 2016-04-18 15:08:14 PDT
(In reply to comment #4)
> Comment on attachment 276667 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276667&action=review
> 
> cool!
> 

Thanks for reviewing :)

> > Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:44
> > -    return std::make_unique<MemoryBackingStoreTransaction>(backingStore, info);
> > +    return std::unique_ptr<MemoryBackingStoreTransaction>(new MemoryBackingStoreTransaction(backingStore, info));
> 
> This shouldn't be needed.
> 

I get a compile error without this:

error C2248: 'WebCore::IDBServer::MemoryBackingStoreTransaction::MemoryBackingStoreTransaction': cannot access private member declared in class 'WebCore::IDBServer::MemoryBackingStoreTransaction

Maybe we should fix this in another way?

> 
> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1188
> > -    auto callback = [this, self, refTransaction](const IDBError& error) {
> > +    ErrorCallback callback = [this, self, refTransaction](const IDBError& error) {
> 
> This shouldn't be needed.
> 

I get an error without this:

error C2668: 'WebCore::IDBServer::UniqueIDBDatabase::storeCallback': ambiguous call to overloaded function

> > Source/WebKit/win/storage/WebDatabaseProvider.cpp:45
> > +    sprintf_s(databaseDirectory, MAX_PATH, "%s\\%s", appDataDirectory, executableName);
> 
> On Mac we use .../WebKit/Databases/___IndexedDB.  We should probably do
> something similar here.

I will update the path :)
Comment 7 peavo 2016-04-19 14:08:29 PDT
Created attachment 276755 [details]
Patch
Comment 8 WebKit Commit Bot 2016-04-19 14:10:41 PDT
Attachment 276755 [details] did not pass style-queue:


ERROR: Source/WebCore/PlatformWin.cmake:189:  There should be exactly one empty line instead of 0 between "Modules/indexeddb" and "Modules/indexeddb/client".  [list/emptyline] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 peavo 2016-04-19 14:12:26 PDT
(In reply to comment #4)
> Comment on attachment 276667 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276667&action=review
> 
> cool!
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:44
> > -    return std::make_unique<MemoryBackingStoreTransaction>(backingStore, info);
> > +    return std::unique_ptr<MemoryBackingStoreTransaction>(new MemoryBackingStoreTransaction(backingStore, info));
> 
> This shouldn't be needed.
> 

I am afraid I didn't find a better way to fix these compile errors.
The main problem seems to be that MSVC does not accept that std::make_unique is declared as a friend of a class.
Comment 10 Alex Christensen 2016-04-19 14:14:55 PDT
Comment on attachment 276755 [details]
Patch

Just make the constructor public.
Comment 11 peavo 2016-04-19 14:34:39 PDT
Created attachment 276760 [details]
Patch
Comment 12 peavo 2016-04-19 14:35:20 PDT
(In reply to comment #10)
> Comment on attachment 276755 [details]
> Patch
> 
> Just make the constructor public.

Thanks :) Updated patch.
Comment 13 WebKit Commit Bot 2016-04-19 14:37:03 PDT
Attachment 276760 [details] did not pass style-queue:


ERROR: Source/WebCore/PlatformWin.cmake:189:  There should be exactly one empty line instead of 0 between "Modules/indexeddb" and "Modules/indexeddb/client".  [list/emptyline] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Alex Christensen 2016-04-19 22:46:05 PDT
Comment on attachment 276760 [details]
Patch

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

Looks good.  I think this needs another iteration, though.

> Source/WebCore/PlatformWin.cmake:189
>      Modules/indexeddb
> +    Modules/indexeddb/client

Style bot wants a space here.

> Source/WebCore/bindings/js/JSDOMWindowBase.h:23
> +#include "DOMWindow.h"

Is this really necessary?  What error does this prevent?  Can we not forward declare things?  Including more headers in a header like this makes the build slower.

> Source/WebCore/bindings/js/JSDOMWrapper.h:25
> +#include "DOMWrapperWorld.h"

Ditto.

> Source/WebKit/win/storage/WebDatabaseProvider.cpp:41
> +        return String("");

This isn't used in WebKit.  I think this is what emptyString() is for.

> Source/WebKit/win/storage/WebDatabaseProvider.cpp:50
> +        return String("");

ditto.
Comment 15 peavo 2016-04-21 07:31:46 PDT
Created attachment 276924 [details]
Patch
Comment 16 peavo 2016-04-21 08:25:32 PDT
(In reply to comment #14)
> Comment on attachment 276760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276760&action=review
> 

Thanks for reviewing!

> 
> > Source/WebCore/bindings/js/JSDOMWindowBase.h:23
> > +#include "DOMWindow.h"
> 
> Is this really necessary?  What error does this prevent?  Can we not forward
> declare things?  Including more headers in a header like this makes the
> build slower.
> 

I get the error:

wtf/PassRefPtr.h(42): error C2027: use of undefined type 'WebCore::DOMWindow'

PassRefPtr<DOMWindow> is used in JSDOMWindowBase.h, but this fails to compile since the class DOMWindow is only forward declared here.
Comment 17 WebKit Commit Bot 2016-04-25 11:37:55 PDT
Comment on attachment 276924 [details]
Patch

Clearing flags on attachment: 276924

Committed r200036: <http://trac.webkit.org/changeset/200036>
Comment 18 WebKit Commit Bot 2016-04-25 11:38:01 PDT
All reviewed patches have been landed.  Closing bug.