Bug 176062 – [Win] Compile error, 'Cache' is not declared.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Release\derivedsources\webcore\JSCache.h(29): error C2065: 'Cache': undeclared identifier (compiling source file Release\DerivedSources\WebCore\JSCache.cpp)

[Per Arne Vollan]

Created attachment 319274 [details]
Patch

[Per Arne Vollan]

Created attachment 319276 [details]
Patch

[Per Arne Vollan]

Created attachment 319294 [details]
Patch

[Per Arne Vollan]

Created attachment 319299 [details]
Patch

[Build Bot]

Comment on attachment 319299 [details]
Patch

Attachment 319299 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4406565

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-storage.https.html
imported/w3c/web-platform-tests/service-workers/stub-4.6.2-cache.html
js/dom/global-constructors-attributes-dedicated-worker.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-storage.https.html

[Build Bot]

Created attachment 319316 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319299 [details]
Patch

Attachment 319299 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4406560

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-storage.https.html
imported/w3c/web-platform-tests/service-workers/stub-4.6.2-cache.html
js/dom/global-constructors-attributes-dedicated-worker.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-storage.https.html

[Build Bot]

Created attachment 319317 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Per Arne Vollan]

Created attachment 319365 [details]
Patch

[Per Arne Vollan]

Created attachment 319391 [details]
Patch

[Build Bot]

Comment on attachment 319391 [details]
Patch

Attachment 319391 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4412151

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-storage.https.html
imported/w3c/web-platform-tests/service-workers/stub-4.6.2-cache.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-storage.https.html
js/dom/global-constructors-attributes-dedicated-worker.html

[Build Bot]

Created attachment 319406 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319391 [details]
Patch

Attachment 319391 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4412197

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-storage.https.html
imported/w3c/web-platform-tests/service-workers/stub-4.6.2-cache.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-storage.https.html
js/dom/global-constructors-attributes-dedicated-worker.html

[Build Bot]

Created attachment 319412 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Per Arne Vollan]

I have attempted to fix the Windows build failure by renaming the Cache interface and using the ImplementedAs attribute, but this seem to cause test regressions on mac-wk2.

[Chris Dumez]

Comment on attachment 319391 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:38
> +    macro(CacheInternal) \

We usually use DOM preference, like DOMCache I believe.

[Chris Dumez]

Comment on attachment 319391 [details]
Patch

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

> Source/WebCore/Modules/cache/CacheInternal.idl:32
> +] interface Cache {

This should be [InterfaceName=Cache] interface DOMCache.

See DOMFileSystem.idl as example.

[Per Arne Vollan]

(In reply to Chris Dumez from comment #17)
> Comment on attachment 319391 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319391&action=review
> 
> > Source/WebCore/Modules/cache/CacheInternal.idl:32
> > +] interface Cache {
> 
> This should be [InterfaceName=Cache] interface DOMCache.
> 
> See DOMFileSystem.idl as example.

Thanks!

[Chris Dumez]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 319391 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319391&action=review
> 
> > Source/JavaScriptCore/runtime/CommonIdentifiers.h:38
> > +    macro(CacheInternal) \
> 
> We usually use DOM preference, like DOMCache I believe.

Youenn tells me there is already a namespace named DOMCache. My personal preference would be to:
- Rename DOMCache namespace to something else (e.g. CacheUtilities or CacheAPI)
- Rename Cache class to DOMCache

[Chris Dumez]

(In reply to Per Arne Vollan from comment #15)
> I have attempted to fix the Windows build failure by renaming the Cache
> interface and using the ImplementedAs attribute, but this seem to cause test
> regressions on mac-wk2.

The reason this fails on WK2 is likely that the Cache API is only enabled on WK2. Your IDL was wrong and I believe it was exposing CacheInternal name to the Web instead of Cache name.

[youenn fablet]

We should rename both Cache and CacheStorage classes for consistency.

There is already a DOMCache namespace, see DOMCache.h/.cpp, which contains various definitions used in WebCore/WebKit2.
We might want to rename it to DOMCacheEngine.
Maybe best done as a preliminary patch.

[youenn fablet]

Alternatively, would there be a way at some other level (build maybe?) to prevent these kind of issues.
It is annoying to use WTF::Function while we could use Function.

[Per Arne Vollan]

(In reply to youenn fablet from comment #21)
> We should rename both Cache and CacheStorage classes for consistency.
> 
> There is already a DOMCache namespace, see DOMCache.h/.cpp, which contains
> various definitions used in WebCore/WebKit2.
> We might want to rename it to DOMCacheEngine.
> Maybe best done as a preliminary patch.

Sounds good, thanks!

[Per Arne Vollan]

(In reply to youenn fablet from comment #22)
> Alternatively, would there be a way at some other level (build maybe?) to
> prevent these kind of issues.
> It is annoying to use WTF::Function while we could use Function.

We could also resolve this build failure on Windows by changing the include order, but I am not sure that is the best way.

[Per Arne Vollan]

Created attachment 319469 [details]
Patch

[Per Arne Vollan]

Created attachment 319502 [details]
Patch

[Build Bot]

Attachment 319502 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/DOMCacheEngine.h:50:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 319510 [details]
Patch

[youenn fablet]

Comment on attachment 319510 [details]
Patch

r=me once bots are happy

[Per Arne Vollan]

Created attachment 319516 [details]
Patch

[Per Arne Vollan]

Created attachment 319526 [details]
Patch

[Per Arne Vollan]

Created attachment 319563 [details]
Patch

[Build Bot]

Attachment 319563 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/DOMCacheEngine.h:50:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 319563 [details]
Patch

Clearing flags on attachment: 319563

Committed r221478: <http://trac.webkit.org/changeset/221478>

[Per Arne Vollan]

Created attachment 319616 [details]
Patch

[youenn fablet]

Comment on attachment 319616 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        * Modules/cache/DOMCache.cpp: Copied from Source/WebCore/Modules/cache/Cache.cpp.

LGTM.
For consistency, we should also move CacheStorage to DOMCacheStorage, in this patch or in a follow-up.

[Per Arne Vollan]

(In reply to youenn fablet from comment #36)
> Comment on attachment 319616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319616&action=review
> 
> > Source/WebCore/ChangeLog:24
> > +        * Modules/cache/DOMCache.cpp: Copied from Source/WebCore/Modules/cache/Cache.cpp.
> 
> LGTM.
> For consistency, we should also move CacheStorage to DOMCacheStorage, in
> this patch or in a follow-up.

If it is ok with you, I can do that in a follow-up.

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 319616 [details]
Patch

Rejecting attachment 319616 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 319616, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
d at 510 (offset 34 lines).
Hunk #19 succeeded at 542 (offset 34 lines).
2 out of 19 hunks FAILED -- saving rejects to file Source/WebCore/Modules/cache/DOMCache.cpp.rej
patching file Source/WebCore/Modules/cache/DOMCache.h
patching file Source/WebCore/Modules/cache/DOMCache.idl
patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Youenn Fablet']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4427913

[Per Arne Vollan]

Created attachment 319628 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 319628 [details]
Patch

Rejecting attachment 319628 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 319628, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4428341

[Per Arne Vollan]

Created attachment 319638 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 319638 [details]
Patch

Clearing flags on attachment: 319638

Committed r221498: <http://trac.webkit.org/changeset/221498>

[Radar WebKit Bug Importer]

<rdar://problem/34257236>