Bug 52296 – [Qt]Add local storage settings to QtTestBrowser option menu

• Component: Tools / Tests
• Status: RESOLVED FIXED

[suchi]

Following local storage settings can be enable/disable from option menu (under Developer menu)for QtTestBrowser: 

LocalStorage

OfflineStorageDatabase

OfflineWebApplicationCache

setOfflineStorageDefaultQuota(maxsize): provides an input box to enter the maxsize for default quota.

[suchi]

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.

[WebKit Review Bot]

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.

[suchi]

Created attachment 78965 [details]
updated patch

Took care of the style( space) issue

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Simon Hausmann]

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.

[Simon Hausmann]

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’

[suchi]

Created attachment 79195 [details]
Modified the patch to take care of compiler issue

Compiler issue has been taken care of

[Andreas Kling]

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.

[suchi]

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

Updated patch with suggested changes.

[Andreas Kling]

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.

[suchi]

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.

[Andreas Kling]

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.

[suchi]

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.

[Andreas Kling]

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

Beautiful! r=me

[WebKit Commit Bot]

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

[suchi]

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.

[WebKit Commit Bot]

Comment on attachment 81262 [details]
Conflict removed from launcherwindow.h

Clearing flags on attachment: 81262

Committed r77725: <http://trac.webkit.org/changeset/77725>

[WebKit Review Bot]

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

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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

[suchi]

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.

[Andreas Kling]

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.

[suchi]

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

[Laszlo Gombos]

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

r=me.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 81636 [details]:

http/tests/websocket/tests/handshake-challenge-randomness.html bug 53738 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[Laszlo Gombos]

Closing as it has been landed.