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
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.
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.
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 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.
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.
I can help to land this with changes. Thanks Joe for the review !
Created attachment 76062 [details] uploading for commit with the requested minor changes
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 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
*** Bug 48194 has been marked as a duplicate of this bug. ***