[webkit-reviews] review denied: [Bug 52296] [Qt]Add local storage settings to QtTestBrowser option menu : [Attachment 79195] Modified the patch to take care of compiler issue

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied suchi <suchi.kundu at nokia.com>'s
request for review:
Bug 52296: [Qt]Add local storage settings to QtTestBrowser option menu
https://bugs.webkit.org/show_bug.cgi?id=52296

Attachment 79195: Modified the patch to take care of compiler issue
https://bugs.webkit.org/attachment.cgi?id=79195&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
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.offlineStorageDef
aultQuotaSize) ? 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.offlineStorag
eDefaultQuotaSize);

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.