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.
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 on attachment 38796 [details] Proposed patch. What is: 1213 void tst_QWebPage::disableEnable() and why is it so poorly named?
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 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.
(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.
Created attachment 38960 [details] 3. try Address Eric's comments.
Comment on attachment 38960 [details] 3. try LGTM.
Comment on attachment 38960 [details] 3. try Clearing flags on attachment: 38960 Committed r48007: <http://trac.webkit.org/changeset/48007>
All reviewed patches have been landed. Closing bug.