[Webkit-unassigned] [Bug 28836] [Qt] Add a setting to turn SessionStorage on/off

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28836





--- Comment #5 from Laszlo Gombos <laszlo.1.gombos at nokia.com>  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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.