Bug 52296

Created attachment 78963 [details] Local Storage settings can be enable/disable from "Develop" menu Following local storage settings can be enable/disable from option menu (under "Develop" menu)for QtTestBrowser, also if these settings value are passed from command line the values(enable/disable) will reflect in option menu : LocalStorage OfflineStorageDatabase OfflineWebApplicationCache setOfflineStorageDefaultQuota(maxsize): provides an input box/dialog to enter the maxsize for default quota, if the value is passed from command line we won't show the dialog to enter the maxsize.

Attachment 78963 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/la..." exit_code: 1 Tools/QtTestBrowser/launcherwindow.cpp:805: Should have a space between // and comment [whitespace/comments] [4] Tools/QtTestBrowser/launcherwindow.cpp:806: Missing space before ( in if( [whitespace/parens] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 78965 [details] updated patch Took care of the style( space) issue

Attachment 78963 [details] did not build on qt: Build output: http://queues.webkit.org/results/7501046

Attachment 78965 [details] did not build on qt: Build output: http://queues.webkit.org/results/7544057

Comment on attachment 78963 [details] Local Storage settings can be enable/disable from "Develop" menu It appears that this attachment is obsoleted by the second attached patch.

Comment on attachment 78965 [details] updated patch It appears that this attachment does not compile: cc1plus: warnings being treated as errors ../../../../Tools/QtTestBrowser/launcherwindow.cpp: In member function ‘void LauncherWindow::createChrome()’: ../../../../Tools/QtTestBrowser/launcherwindow.cpp:281: error: unused variable ‘offlineStorageDefaultQuotaAction’

Created attachment 79195 [details] Modified the patch to take care of compiler issue Compiler issue has been taken care of

Comment on attachment 79195 [details] Modified the patch to take care of compiler issue View in context: https://bugs.webkit.org/attachment.cgi?id=79195&action=review > Tools/QtTestBrowser/launcherwindow.cpp:277 > + QAction* toggleOfflineWebApplicationCache = toolsMenu->addAction("Enable Offline WebApplication Cache", this, SLOT(toggleOfflineWebApplicationCache(bool))); "WebApplication" -> "Web Application" > Tools/QtTestBrowser/launcherwindow.cpp:281 > + QAction* offlineStorageDefaultQuotaAction = toolsMenu->addAction("Set Offline Storage Default Quota Maxsize", this, SLOT(setOfflineStorageDefaultQuota())); "Maxsize" -> "Max Size" > Tools/QtTestBrowser/launcherwindow.cpp:283 > + offlineStorageDefaultQuotaAction->setChecked((m_windowOptions.offlineStorageDefaultQuotaSize) ? true : false); The "? true : false" is unnecessary. > Tools/QtTestBrowser/launcherwindow.cpp:286 > + Whitespace. > Tools/QtTestBrowser/launcherwindow.cpp:787 > +void LauncherWindow::toggleLocalStorage(bool toggle) "toggle" is called "enable" in the declaration (and in the similar function above.) We should be consistent. > Tools/QtTestBrowser/launcherwindow.cpp:793 > +void LauncherWindow::toggleOfflineStorageDatabase(bool toggle) Ditto. > Tools/QtTestBrowser/launcherwindow.cpp:799 > +void LauncherWindow::toggleOfflineWebApplicationCache(bool toggle) Ditto. > Tools/QtTestBrowser/launcherwindow.cpp:807 > + // for command line execution, quota size is taken from command line and set it below Comments should start with a capital and end with a period. > Tools/QtTestBrowser/launcherwindow.cpp:809 > + page()->settings()->setOfflineStorageDefaultQuota(m_windowOptions.offlineStorageDefaultQuotaSize); Coding style, indentation should be 4 spaces, not 3 (this block) or 2 (the block below.) > Tools/QtTestBrowser/launcherwindow.cpp:811 > + QDialog* dialog = new QDialog(this); This variable should be on the stack. > Tools/QtTestBrowser/launcherwindow.cpp:814 > + dialog->setWindowTitle("Offline Storage Default Quota Maxsize"); Strange title, let's do "Maxsize" -> "Max Size" at least. > Tools/QtTestBrowser/launcherwindow.h:121 > + bool useLocalStorageEnabled; > + bool useOfflineStorageDatabaseEnabled; > + bool useOfflineWebApplicationCacheEnabled; > + quint64 offlineStorageDefaultQuotaSize; These variables should be initialized (in the WindowOptions constructor above.) Also, the "Enabled" suffix on the first 3 is redundant. > Tools/QtTestBrowser/launcherwindow.h:185 > - > + Whitespace. > Tools/QtTestBrowser/main.cpp:168 > + Whitespace.

Created attachment 79846 [details] Updated the patch according to suggested change Updated patch with suggested changes.

Comment on attachment 79846 [details] Updated the patch according to suggested change View in context: https://bugs.webkit.org/attachment.cgi?id=79846&action=review > Tools/QtTestBrowser/launcherwindow.cpp:919 > + QDialog* dialog = new QDialog(this); This whole section could be replaced with a call to QInputDialog::getInt(), let's do that instead. > Tools/QtTestBrowser/main.cpp:167 > + windowOptions.useLocalStorage = true; Unnecessary whitespace. > Tools/QtTestBrowser/main.cpp:178 > + windowOptions.offlineStorageDefaultQuotaSize = maxSize; Ditto.

Created attachment 81067 [details] Modified code with QInputDialog::getInt() function call Instead of using QDialog box using QInputDialog::getInt() to get the quota size entered by user.

Comment on attachment 81067 [details] Modified code with QInputDialog::getInt() function call View in context: https://bugs.webkit.org/attachment.cgi?id=81067&action=review Much better, some small things remaining: > Tools/QtTestBrowser/launcherwindow.cpp:923 > + page()->settings()->setOfflineStorageDefaultQuota(quotaSize); What if 'quotaSize' is negative? > Tools/QtTestBrowser/launcherwindow.cpp:924 > + } This } is wrongly indented. > Tools/QtTestBrowser/main.cpp:177 > int maxSize = takeOptionValue(&args, setOfflineStorageDefaultQuotaIndex).toInt(); 'maxSize' should be of an unsigned type, to match WindowOptions::offlineStorageDefaultQuotaSize.

Created attachment 81103 [details] Took care of the indentation issue and also modified the maxsize to unsigned int Thanks for the review. To answer your question about "quotaSize negative", as I have made the min value to be zero, user can not enter a negative value for quotaSize. Please let me know if you have any other question about this value.

Comment on attachment 81103 [details] Took care of the indentation issue and also modified the maxsize to unsigned int Beautiful! r=me

Comment on attachment 81103 [details] Took care of the indentation issue and also modified the maxsize to unsigned int Rejecting attachment 81103 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2 Last 500 characters of output: Tools/QtTestBrowser/launcherwindow.cpp Hunk #1 succeeded at 264 (offset 5 lines). Hunk #2 succeeded at 920 (offset 28 lines). patching file Tools/QtTestBrowser/launcherwindow.h patch: **** malformed patch at line 38: patching file Tools/QtTestBrowser/main.cpp Hunk #1 FAILED at 164. 1 out of 1 hunk FAILED -- saving rejects to file Tools/QtTestBrowser/main.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Andreas Kling', u'--fo..." exit_code: 1 Full output: http://queues.webkit.org/results/7699045

Created attachment 81262 [details] Conflict removed from launcherwindow.h I had forgot to run svn update before I created the patch as a result there was conflict on launcherwindow.h.

Comment on attachment 81262 [details] Conflict removed from launcherwindow.h Clearing flags on attachment: 81262 Committed r77725: <http://trac.webkit.org/changeset/77725>

http://trac.webkit.org/changeset/77725 might have broken Qt Linux Release minimal

(In reply to comment #19) > http://trac.webkit.org/changeset/77725 might have broken Qt Linux Release minimal error message: ../../../Tools/QtTestBrowser/launcherwindow.cpp: In member function ‘void LauncherWindow::setOfflineStorageDefaultQuota()’: ../../../Tools/QtTestBrowser/launcherwindow.cpp:949: error: ‘QInputDialog’ has not been declared It seems you missed to add a QT_NO_INPUTDIALOG guard.

It was rolled out by http://trac.webkit.org/changeset/77737 to make buildbot happy for this weekend.

Created attachment 81620 [details] QT_NO_INPUTDIALOG guard is in place Modified code with QT_NO_INPUTDIALOG guard to resolve Qt Linux Release minimal build issue.

Comment on attachment 81620 [details] QT_NO_INPUTDIALOG guard is in place View in context: https://bugs.webkit.org/attachment.cgi?id=81620&action=review r=me, one thing: > Tools/QtTestBrowser/launcherwindow.cpp:947 > + bool ok; Should be inside the QT_NO_INPUTDIALOG guard.

Created attachment 81636 [details] local variable needs to be inside the QT_NO_INPUTDIALOG guard Silly mistake, forgot to put local boolean variable inside the QT_NO_INPUTDIALOG guard

Comment on attachment 81636 [details] local variable needs to be inside the QT_NO_INPUTDIALOG guard r=me.

The commit-queue encountered the following flaky tests while processing attachment 81636 [details]: http/tests/websocket/tests/handshake-challenge-randomness.html bug 53738 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 81636 [details] local variable needs to be inside the QT_NO_INPUTDIALOG guard Clearing flags on attachment: 81636 Committed r77947: <http://trac.webkit.org/changeset/77947>

Closing as it has been landed.