[webkit-reviews] review denied: [Bug 28836] [Qt] Add a setting to turn SessionStorage on/off : [Attachment 38809] second try

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Laszlo Gombos
<laszlo.1.gombos at nokia.com>'s request for review:
Bug 28836: [Qt] Add a setting to turn SessionStorage on/off
https://bugs.webkit.org/show_bug.cgi?id=28836

Attachment 38809: second try
https://bugs.webkit.org/attachment.cgi?id=38809&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Not to pick on the naming too much, but testOptionalJSObjects doesn't mention
anything about storage maybe testOptionalStorageJSObjects?

Why does testOptionalJSObjects use two web views?  Seems it doesn't need to.

Seems like we could make a simple inline function to do all of these tests
easier:

testStorageFlag(QWebSettings::SessionStorageEnabled, "sessionStorage", false);

static inline testStorageFlag(QString settingName, QString jsObjectName, bool
settingValue)

Why use:
QCOMPARE(webPage1->mainFrame()->evaluateJavaScript("if(window.sessionStorage ==
undefined) {2}").toInt(), 2);
instead of something like this:
QCOMPARE(webPage1->mainFrame()->evaluateJavaScript("window.sessionStorage ==
undefined").toInt(), true);
Then it would be easier to compare with teh bool value passed into your little
helper function.

Turning the "sessionStorage" stirng passed into your helper into the JS string
will require a bit of printf magic. :)

I think this change is fine.  I'm OK to r+ this, but since I expect you'll read
my fantastically convincing arguments above and want to change the test to be
more super awesome, I'll r- this.  If you disagree that the test should be so
awsome, feel free to r? this again as-is.