Bug 96374 – [WK2][WKTR] TestRunner needs to implement dumpApplicationCacheDelegateCallbacks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

TestRunner needs to implement dumpApplicationCacheDelegateCallbacks.

This is needed by the following test cases:
  http/tests/appcache/origin-delete.html
  http/tests/appcache/origin-quota.html
  http/tests/appcache/origin-quota-continued-download.html
  http/tests/appcache/origin-quota-continued-download-multiple-manifests.html
  http/tests/appcache/origin-usage.html
  http/tests/appcache/origins-with-appcache.html

[Chris Dumez]

Created attachment 163531 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 163531 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp:254
> +void WKBundleResetApplicationCacheOriginQuota(WKBundleRef bundleRef, WKStringRef origin)

We always use strings for security origin?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:260
> +    WKBundlePageReachedAppCacheOriginQuotaCallback                      reachedApplicationCacheOriginQuota;

didReach? Look at what naming is used elsewhere in the C api

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1413
> +void InjectedBundlePage::reachedApplicationCacheOriginQuota(WKSecurityOriginRef origin, int64_t totalSpaceNeeded)

totalSpaceNeeded? bytes? mega bytes? not clear

[Chris Dumez]

Comment on attachment 163531 [details]
Patch

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

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp:254
>> +void WKBundleResetApplicationCacheOriginQuota(WKBundleRef bundleRef, WKStringRef origin)
> 
> We always use strings for security origin?

So far in all WKBundle ApplicationCache APIs, yes. We need to go through String representation to construct a WebCore::SecurityOrigin anyway.

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:260
>> +    WKBundlePageReachedAppCacheOriginQuotaCallback                      reachedApplicationCacheOriginQuota;
> 
> didReach? Look at what naming is used elsewhere in the C api

I chose to be consistent with both the ChromeClient naming and the Database equivalent:
"WKPageExceededDatabaseQuotaCallback exceededDatabaseQuota;" in WKPageUIClient

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1413
>> +void InjectedBundlePage::reachedApplicationCacheOriginQuota(WKSecurityOriginRef origin, int64_t totalSpaceNeeded)
> 
> totalSpaceNeeded? bytes? mega bytes? not clear

Ok. Will switch to totalBytesNeeded.

[Chris Dumez]

Created attachment 163537 [details]
Patch

Rename totalSpaceNeeded to totalBytesNeeded.

[Gyuyoung Kim]

Comment on attachment 163537 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1424
> +    InjectedBundle::shared().stringBuilder()->append("UI DELEGATE APPLICATION CACHE CALLBACK: exceededApplicationCacheOriginQuotaForSecurityOrigin:{");

IMO, appendLiteral is more efficient.
http://trac.webkit.org/wiki/EfficientStrings

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1426
> +    InjectedBundle::shared().stringBuilder()->append(", ");

ditto.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1428
> +    InjectedBundle::shared().stringBuilder()->append(", ");

ditto.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1430
> +    InjectedBundle::shared().stringBuilder()->append("} totalSpaceNeeded:~");

ditto.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1432
> +    InjectedBundle::shared().stringBuilder()->append("\n");

ditto.

[Chris Dumez]

(In reply to comment #5)
> (From update of attachment 163537 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163537&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1424
> > +    InjectedBundle::shared().stringBuilder()->append("UI DELEGATE APPLICATION CACHE CALLBACK: exceededApplicationCacheOriginQuotaForSecurityOrigin:{");
> 
> IMO, appendLiteral is more efficient.
> http://trac.webkit.org/wiki/EfficientStrings
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1426
> > +    InjectedBundle::shared().stringBuilder()->append(", ");
> 
> ditto.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1428
> > +    InjectedBundle::shared().stringBuilder()->append(", ");
> 
> ditto.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1430
> > +    InjectedBundle::shared().stringBuilder()->append("} totalSpaceNeeded:~");
> 
> ditto.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1432
> > +    InjectedBundle::shared().stringBuilder()->append("\n");
> 
> ditto.

Good point. I will fix this.

[Chris Dumez]

Created attachment 163540 [details]
Patch

Use appendLiteral() when possible.

[Kenneth Rohde Christiansen]

> > 
> > > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1432
> > > +    InjectedBundle::shared().stringBuilder()->append("\n");
> > 
> > ditto.
> 
> Good point. I will fix this.

->append('\n') is more effective than appendLiteral("\n")

[Chris Dumez]

Created attachment 163544 [details]
Patch

Take Kenneth's feedback into consideration (including the reached -> didReach renaming).

[WebKit Review Bot]

Comment on attachment 163544 [details]
Patch

Clearing flags on attachment: 163544

Committed r128280: <http://trac.webkit.org/changeset/128280>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 96498

[Chris Dumez]

Created attachment 163645 [details]
Patch

Export additional symbols on Windows to fix the following linking issue:
6>   Creating library C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\lib\InjectedBundle.lib and object C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\lib\InjectedBundle.exp
6>InjectedBundlePage.obj : error LNK2019: unresolved external symbol "public: static class WTF::String __cdecl WTF::String::number(__int64)" (?number@String@WTF@@SA?AV12@_J@Z) referenced in function "private: void __thiscall WTR::InjectedBundlePage::didReachApplicationCacheOriginQuota(struct OpaqueWKSecurityOrigin const *,__int64)" (?didReachApplicationCacheOriginQuota@InjectedBundlePage@WTR@@AAEXPBUOpaqueWKSecurityOrigin@@_J@Z)
6>C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\bin\InjectedBundle.dll : fatal error LNK1120: 1 unresolved externals

[Kenneth Rohde Christiansen]

Comment on attachment 163645 [details]
Patch

lets hope for the best

[WebKit Review Bot]

Comment on attachment 163645 [details]
Patch

Clearing flags on attachment: 163645

Committed r128326: <http://trac.webkit.org/changeset/128326>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.