Allow LocalStorage to be enabled without enabling SessionStorage at runtime. There is a settings class setting for localStorage, but not for sessionStorage. We want to be able to test one of these features without necessarily enabling the other. I think the solution is to add a sessionStorageEnabled flag that defaults to true. Chromium can then opt-out.
Created attachment 32815 [details] v1 Allow LocalStorage to be enabled without enabling SessionStorage at runtime. There is a settings class setting for localStorage, but not for sessionStorage. We want to be able to test one of these features without necessarily enabling the other.
Comment on attachment 32815 [details] v1 > Index: WebCore/page/DOMWindow.cpp > =================================================================== > --- WebCore/page/DOMWindow.cpp (revision 45916) > +++ WebCore/page/DOMWindow.cpp (working copy) > @@ -552,6 +552,9 @@ Storage* DOMWindow::sessionStorage() con > return 0; > > Document* document = m_frame->document(); > + Settings* settings = document->settings(); > + if (!settings || !settings->sessionStorageEnabled()) You should just use page->settings() here which cannot be null. Otherwise, looks good to me.
Created attachment 32897 [details] v2 Cleaned up the way we access the settings per Darin F.
Comment on attachment 32897 [details] v2 Looks good, but two nits: > - Document* document = m_frame->document(); > + Settings* settings = page->settings(); > + if (!settings->sessionStorageEnabled()) > + return 0; If settings isn't used by the rest of this function, then you don't need to make a variable for it. > , m_localStorageEnabled(false) > + , m_sessionStorageEnabled(true) Why do these have different defaults?
(In reply to comment #4) > (From update of attachment 32897 [details]) > Looks good, but two nits: > > > - Document* document = m_frame->document(); > > + Settings* settings = page->settings(); > > + if (!settings->sessionStorageEnabled()) > > + return 0; > > If settings isn't used by the rest of this function, then you don't need to > make a variable for it. I'll change it. > > , m_localStorageEnabled(false) > > + , m_sessionStorageEnabled(true) > > Why do these have different defaults? LocalStorage is off by default for security reasons since it allows you to write information to the disk. It makes sense for it to require an explicit opt-in. SessionStorage has always been on in the past, so it seems wrong to change its default. In addition, I don't think it's controversial at all in terms of security. The only reason we want to turn it off in Chromium is because it's currently broken and LocalStorage is a higher priority for us.
Created attachment 32905 [details] v3 We now do page->settings()-> rather than saving page->settings() to a local variable. I also updated the change log to explain why SessionStorage is on by default.
Comment on attachment 32905 [details] v3 Looks great. Thanks for the explanation.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/page/DOMWindow.cpp M WebCore/page/Settings.cpp M WebCore/page/Settings.h Committed r46001