Bug 27318 – Allow LocalStorage to be enabled without enabling SessionStorage

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

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.

[Jeremy Orlow]

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.

[Darin Fisher (:fishd, Google)]

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.

[Jeremy Orlow]

Created attachment 32897 [details]
v2

Cleaned up the way we access the settings per Darin F.

[Adam Barth]

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?

[Jeremy Orlow]

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

[Jeremy Orlow]

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.

[Adam Barth]

Comment on attachment 32905 [details]
v3

Looks great.  Thanks for the explanation.

[Adam Barth]

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