Bug 28836 – [Qt] Add a setting to turn SessionStorage on/off

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Laszlo Gombos]

Az LocalStorage can be turned on/off with a public QtWebKit API and the WebCore internal API allows to turn on/off SessionStorage the same way as LocalStorage, it might make sense to expose a setting for SessionStorage as well.

[Laszlo Gombos]

Created attachment 38796 [details]
Proposed patch.

Also added a test case to make sure that the page content can detect if a particular feature is disabled/enabled runtime via a JS test.

[Eric Seidel (no email)]

Comment on attachment 38796 [details]
Proposed patch.

What is:
 1213 void tst_QWebPage::disableEnable()

and why is it so poorly named?

[Laszlo Gombos]

Created attachment 38809 [details]
second try

Updated the patch to change the test case name to testOptionalJSObjects(). The test is to make sure that turning off the feature will make the corresponding JS object undefined.

[Eric Seidel (no email)]

Comment on attachment 38809 [details]
second try

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.

[Laszlo Gombos]

(In reply to comment #4)
> (From update of attachment 38809 [details])

Eric, thanks for the review !

> Not to pick on the naming too much, but testOptionalJSObjects doesn't mention
> anything about storage maybe testOptionalStorageJSObjects?

I intentionally did not wanted to restrict the name of the test for Storage only. I was hoping that we could expand this test kater as more and more (HTML5) features and with that more and more JS objects become available.

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

We need two different instances of QWebPage. I will addressed this with a comment in the test case itself in the revised patch.

> 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)
> 

This is a valid point I will make this change. I think it is important to do the actual test (e.g. like QCOMPARE) outside of the helper functions, so that if the test fails it would give a meaningful line number for the failure. For this reason the test is not optimized for min lines of code but for most descriptive error message if the test fails.

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

Again valid point, will change it.

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

[Laszlo Gombos]

Created attachment 38960 [details]
3. try

Address Eric's comments.

[Eric Seidel (no email)]

Comment on attachment 38960 [details]
3. try

LGTM.

[Eric Seidel (no email)]

Comment on attachment 38960 [details]
3. try

Clearing flags on attachment: 38960

Committed r48007: <http://trac.webkit.org/changeset/48007>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.