Bug 96374 - [WK2][WKTR] TestRunner needs to implement dumpApplicationCacheDelegateCallbacks
Summary: [WK2][WKTR] TestRunner needs to implement dumpApplicationCacheDelegateCallbacks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 96379 96498
Blocks: 96496
  Show dependency treegraph
 
Reported: 2012-09-11 04:16 PDT by Chris Dumez
Modified: 2012-09-12 09:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (25.90 KB, patch)
2012-09-12 00:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.90 KB, patch)
2012-09-12 01:02 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.93 KB, patch)
2012-09-12 01:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.94 KB, patch)
2012-09-12 01:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (27.04 KB, patch)
2012-09-12 09:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.