Bug 43455 – [Qt]: Implement Application Cache Quotas

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Joseph Pecoraro]

Skipped the following test:
http/tests/appcache/origin-quota.html

Mac implementation:
<http://webkit.org/b/40627> Limit ApplicationCache Total and Per-Origin Storage Capacity (Quotas)

Created attachment 75413 [details]
Patch to fix skipped layout test origin-quota.html

Implemented the empty stubs created to fix skipped appcache layout test origin-quota.html

[Joseph Pecoraro]

Comment on attachment 75413 [details]
Patch to fix skipped layout test origin-quota.html

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

Looks good to me, although I'm not familiar with the Qt's "emit" syntax. I think the
tests were disabled for qt. If you enable the tests, do they now pass?

> WebKit/qt/Api/qwebpage.h:52
> +class QWebSecurityOrigin;

Nit: follow alphabetical order.

> WebKit/qt/Api/qwebpage.h:315
> +    bool supportsContentType(const QString& mimeType) const;    

Nit: Extra whitespace at the end of the line should be removed.

> WebKit/qt/Api/qwebpage.h:391
> +    void applicationCacheDatabaseQuotaExceeded(QWebSecurityOrigin* origin, quint64 defaultOriginQuota);

Any particular reason these new functions are named "applicationCacheDatabase*" instead
of just "applicationCache*". The "Database" part is an implementation detail that should
probably be left out. This is in multiple places throughout this patch.

> WebKit/qt/Api/qwebpage.h:430
> +    friend class QWebInspector;   

Nit: whitespace again.

> WebKitTools/ChangeLog:8
> +        Introduce funtions to allow new appcache layout test origin-quota

Typo: "funtions" => "functions"

> WebKitTools/ChangeLog:16
> +        Introduce funtions to allow new appcache layout test

Typo: "funtions" => "functions"

> WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h:253
> +
> +
> +

Nit: no need for the extra newlines

> WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h:260
> +private:    

Nit: whitespace again.

Created attachment 75711 [details]
Patch to fix skipped layout test origin-quota.html

submitting patch 2, taking into account the comments from Joseph Pecaoraro.

[Joseph Pecoraro]

I forgot to ask, does the test pass when unskipped? If so it should be unskipped as part of this patch!

Created attachment 75819 [details]
Patch to fix skipped layout test origin-quota.html

Yes , the patch does fix the origin-quota.html layout test. I have removed the test from the Skipped list. Resubmitting the patch. Apologize for missing it out earlier.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


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

[Joseph Pecoraro]

Comment on attachment 75819 [details]
Patch to fix skipped layout test origin-quota.html

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

r- only because LayoutTests is missing a ChangeLog entry. Otherwise it looks good
to me. Please attach a new patch with that ChangeLog and the style fix!

You should be able to generate the ChangeLog entry by changing directory
into the LayoutTests directory and running:

    WebKitTools/Scripts/prepare-Changelog -b 43455

Note that it will have a slightly different format than your existing
ChangeLog entries:

         Bug 43455 -  [Qt]: Implement Application Cache Quotas
         https://bugs.webkit.org/show_bug.cgi?id=43455

will probably be just:

         [Qt]: Implement Application Cache Quotas
         https://bugs.webkit.org/show_bug.cgi?id=43455

I'd recommend the second approach, but as long as they are consistent
it'll be fine. Thanks for fixing this!

> WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:1030
> +void DumpRenderTree::dumpApplicationCacheQuota(QWebSecurityOrigin* origin, quint64 defaultOriginQuota)
> +{
> +
> +    if (!m_controller->shouldDumpApplicationCacheDelegateCallbacks())

Nit: remove the newline here.

Created attachment 75961 [details]
Patch to fix skipped layout test origin-quota.html

Comment on attachment 75961 [details]
Patch to fix skipped layout test origin-quota.html

Have made the requisite changes. Ran check-webkit-style script with zero errors.

Thank you for being patient and reviewing my patches.

[Joseph Pecoraro]

Comment on attachment 75961 [details]
Patch to fix skipped layout test origin-quota.html

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

> WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:1030
> +void DumpRenderTree::dumpApplicationCacheQuota(QWebSecurityOrigin* origin, quint64 defaultOriginQuota)
> +{
> +
> +    if (!m_controller->shouldDumpApplicationCacheDelegateCallbacks())

Doh. I meant the newline above!

r=me. Wrong newline removed, but that is okay. It would be unnecessary
to have you attach a new patch for just a newline. Do you have someone who
can land these for you (and possibly fix this newline) or do you typically
use the commit queue? I can help you with either.

[Laszlo Gombos]

I can help to land this with changes. Thanks Joe for the review !

[Laszlo Gombos]

Created attachment 76062 [details]
uploading for commit with the requested minor changes

[WebKit Commit Bot]

Comment on attachment 76062 [details]
uploading for commit with the requested minor changes 

Rejecting patch 76062 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1
ERROR: Working directory has local commits, pass --force-clean to continue.

Full output: http://queues.webkit.org/results/6892023

[Chang Shu]

Comment on attachment 76062 [details]
uploading for commit with the requested minor changes 

landed the patch manually, r73789
http://trac.webkit.org/changeset/73789
clean up flags

[Robert Hogan]

*** Bug 48194 has been marked as a duplicate of this bug. ***