Bug 96374

Summary:

[WK2][WKTR] TestRunner needs to implement dumpApplicationCacheDelegateCallbacks

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

WebKit2

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

gyuyoung.kim, kenneth, rakuco, ryuan.choi, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

96379, 96498

Bug Blocks:

96496

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Description Chris Dumez 2012-09-11 04:16:00 PDT

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

Comment 1 Chris Dumez 2012-09-12 00:19:57 PDT

Created attachment 163531 [details]
Patch

Comment 2 Kenneth Rohde Christiansen 2012-09-12 00:47:32 PDT

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

Comment 3 Chris Dumez 2012-09-12 00:55:47 PDT

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.

Comment 4 Chris Dumez 2012-09-12 01:02:51 PDT

Created attachment 163537 [details]
Patch

Rename totalSpaceNeeded to totalBytesNeeded.

Comment 5 Gyuyoung Kim 2012-09-12 01:11:50 PDT

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.

Comment 6 Chris Dumez 2012-09-12 01:21:36 PDT

(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.

Comment 7 Chris Dumez 2012-09-12 01:23:33 PDT

Created attachment 163540 [details]
Patch

Use appendLiteral() when possible.

Comment 8 Kenneth Rohde Christiansen 2012-09-12 01:24:02 PDT

> > 
> > > 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")

Comment 9 Chris Dumez 2012-09-12 01:37:37 PDT

Created attachment 163544 [details]
Patch

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

Comment 10 WebKit Review Bot 2012-09-12 02:17:02 PDT

Comment on attachment 163544 [details]
Patch

Clearing flags on attachment: 163544

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

Comment 11 WebKit Review Bot 2012-09-12 02:17:06 PDT

All reviewed patches have been landed.  Closing bug.

Comment 12 WebKit Review Bot 2012-09-12 04:55:35 PDT

Re-opened since this is blocked by 96498

Comment 13 Chris Dumez 2012-09-12 09:04:09 PDT

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

Comment 14 Kenneth Rohde Christiansen 2012-09-12 09:05:20 PDT

Comment on attachment 163645 [details]
Patch

lets hope for the best

Comment 15 WebKit Review Bot 2012-09-12 09:37:55 PDT

Comment on attachment 163645 [details]
Patch

Clearing flags on attachment: 163645

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

Comment 16 WebKit Review Bot 2012-09-12 09:38:00 PDT

All reviewed patches have been landed.  Closing bug.