Bug 43455

Summary: [Qt]: Implement Application Cache Quotas
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: WebKit QtAssignee: krithigassree.sambamurthy
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, cshu, joepeck, laszlo.gombos, robert, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch to fix skipped layout test origin-quota.html
none
Patch to fix skipped layout test origin-quota.html
none
Patch to fix skipped layout test origin-quota.html
joepeck: review-
Patch to fix skipped layout test origin-quota.html
joepeck: review+
uploading for commit with the requested minor changes none

Description Joseph Pecoraro 2010-08-03 19:06:11 PDT
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)
Comment 1 krithigassree.sambamurthy 2010-12-02 13:53:51 PST
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
Comment 2 Joseph Pecoraro 2010-12-02 14:39:59 PST
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.
Comment 3 krithigassree.sambamurthy 2010-12-06 10:24:22 PST
Created attachment 75711 [details]
Patch to fix skipped layout test origin-quota.html

submitting patch 2, taking into account the comments from Joseph Pecaoraro.
Comment 4 Joseph Pecoraro 2010-12-06 18:16:02 PST
I forgot to ask, does the test pass when unskipped? If so it should be unskipped as part of this patch!
Comment 5 krithigassree.sambamurthy 2010-12-07 08:56:06 PST
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.
Comment 6 WebKit Review Bot 2010-12-07 21:43:31 PST
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.
Comment 7 Joseph Pecoraro 2010-12-08 13:36:01 PST
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.
Comment 8 krithigassree.sambamurthy 2010-12-08 14:18:50 PST
Created attachment 75961 [details]
Patch to fix skipped layout test origin-quota.html
Comment 9 krithigassree.sambamurthy 2010-12-08 14:22:03 PST
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.
Comment 10 Joseph Pecoraro 2010-12-08 14:43:08 PST
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.
Comment 11 Laszlo Gombos 2010-12-09 04:34:02 PST
I can help to land this with changes. Thanks Joe for the review !
Comment 12 Laszlo Gombos 2010-12-09 07:33:20 PST
Created attachment 76062 [details]
uploading for commit with the requested minor changes
Comment 13 WebKit Commit Bot 2010-12-09 19:42:51 PST
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
Comment 14 Chang Shu 2010-12-10 13:03:30 PST
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
Comment 15 Robert Hogan 2010-12-14 13:51:55 PST
*** Bug 48194 has been marked as a duplicate of this bug. ***