Bug 28836 - [Qt] Add a setting to turn SessionStorage on/off
Summary: [Qt] Add a setting to turn SessionStorage on/off
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-30 15:29 PDT by Laszlo Gombos
Modified: 2009-09-03 01:57 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch. (5.07 KB, patch)
2009-08-30 15:38 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
second try (5.09 KB, patch)
2009-08-31 05:52 PDT, Laszlo Gombos
eric: review-
Details | Formatted Diff | Diff
3. try (5.81 KB, patch)
2009-09-02 18:54 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-08-30 15:29:08 PDT
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.
Comment 1 Laszlo Gombos 2009-08-30 15:38:50 PDT
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.
Comment 2 Eric Seidel (no email) 2009-08-31 03:00:56 PDT
Comment on attachment 38796 [details]
Proposed patch.

What is:
 1213 void tst_QWebPage::disableEnable()

and why is it so poorly named?
Comment 3 Laszlo Gombos 2009-08-31 05:52:08 PDT
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.
Comment 4 Eric Seidel (no email) 2009-09-01 08:05:00 PDT
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.
Comment 5 Laszlo Gombos 2009-09-02 18:53:34 PDT
(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.
Comment 6 Laszlo Gombos 2009-09-02 18:54:42 PDT
Created attachment 38960 [details]
3. try

Address Eric's comments.
Comment 7 Eric Seidel (no email) 2009-09-03 01:45:43 PDT
Comment on attachment 38960 [details]
3. try

LGTM.
Comment 8 Eric Seidel (no email) 2009-09-03 01:57:40 PDT
Comment on attachment 38960 [details]
3. try

Clearing flags on attachment: 38960

Committed r48007: <http://trac.webkit.org/changeset/48007>
Comment 9 Eric Seidel (no email) 2009-09-03 01:57:44 PDT
All reviewed patches have been landed.  Closing bug.