Bug 26189 – DOM Storage: Changes for Android and macro guards

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Murdoch]

I've been working on enabling support for the DOM storage APIs in the android browser. The way that the browser's java implementation communicates the path to the database for local storage means that we need to reconfigure the local storage object created by the page group *after* it's been created. To achieve this I've added a new static method to PageGroup to iterate over the current set of PageGroups and call a new method on the LocalStorage object that updates the path to the database and restarts the local storage thread.

I have also taken this opportunity to add #if ENABLE(DOM_STORAGE) guards around the DOM storage source files.

[Ben Murdoch]

Created attachment 30949 [details]
Proposed patch

[Ben Murdoch]

Created attachment 30950 [details]
Proposed patch

[David Kilzer (:ddkilzer)]

Comment on attachment 30950 [details]
Proposed patch

Someone more familiar with local storage should review the core changes, but I have a couple minor comments.

I think the ENABLE(DOM_STORAGE) guards should be added in a separate patch.  (No need for a separate bug, though.)

>Index: WebCore/ChangeLog
>[...]
>+        * storage/LocalStorage.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/LocalStorageArea.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/LocalStorageArea.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/LocalStorageTask.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/LocalStorageTask.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/LocalStorageThread.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/LocalStorageThread.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/SessionStorage.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/SessionStorage.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/SessionStorageArea.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/SessionStorageArea.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/Storage.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/Storage.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/Storage.idl:  Adds Conditional=DOM_STORAGE
>+        * storage/StorageArea.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/StorageArea.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/StorageEvent.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/StorageEvent.h: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/StorageEvent.idl: Adds Conditional=DOM_STORAGE
>+        * storage/StorageMap.cpp: Adds #if ENABLE(DOM_STORAGE) guard.
>+        * storage/StorageMap.h: Adds #if ENABLE(DOM_STORAGE) guard.

Normally we just use "Ditto." instead of repeating the comment in these cases.

>Index: WebCore/page/PageGroup.cpp
>[...]
>+// Right now, this function is only needed by the Android platform as we may recieve notification
>+// from the Java side of the path to use for local storage databases *after* the page group is
>+// created (but before websites can use local storage). If that is the case, the local storage
>+// object will have an empty path and local storage data will not be persisted to disk. This
>+// function allows us to re-establish the local storage object with the correct path so when
>+// sites access local storage, the changes will be saved to disk.
>+void PageGroup::updateLocalStoragePath(const String& path)
>+{
>+#if ENABLE(DOM_STORAGE)
>+    if (!pageGroups)
>+        return;
>+
>+    PageGroupMap::iterator end = pageGroups->end();
>+
>+    for (PageGroupMap::iterator it = pageGroups->begin(); it != end; ++it) {
>+        if (LocalStorage* localStorage = it->second->localStorage())
>+            localStorage->updatePath(path);
>+    }
>+#endif
>+}

Should this have a #if PLATFORM(ANDROID)/#endif around it instead of the long comment?  If anyone is curious as to why the method is there, the could look for an explanation in this bug.

I'm split between adding more #if/#endif macros and compiling unused code on other platforms.  (I think replacing the long comment with the #if/#endif macros is a win, though.)

>Index: WebCore/page/PageGroup.h
>[...]
>+        static void updateLocalStoragePath(const String& path);

This would need PLATFORM(ANDROID) guards as well if the above change is made.

[Ben Murdoch]

(In reply to comment #3)

Thanks for taking a look at this David!

> I think the ENABLE(DOM_STORAGE) guards should be added in a separate patch. 
> (No need for a separate bug, though.)

OK, that's fair enough. I'll do that.

> Should this have a #if PLATFORM(ANDROID)/#endif around it instead of the long
> comment?  If anyone is curious as to why the method is there, the could look
> for an explanation in this bug.
> 
> I'm split between adding more #if/#endif macros and compiling unused code on
> other platforms.  (I think replacing the long comment with the #if/#endif
> macros is a win, though.)

I was hesitant about this too. I'm happy to add the platform guards if you'd like me to, but on the other hand the code doesn't use any Android specific APIs and works on all platforms. It's just only used by Android right now, perhaps the functionality would be useful for others in the future? And the same for the new function in LocalStorage. It's only used on Android at the moment, but it's platform independent and could be useful for others now or later. I can replace the comment with a link to this bug either way.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> > Should this have a #if PLATFORM(ANDROID)/#endif around it instead of the long
> > comment?  If anyone is curious as to why the method is there, the could look
> > for an explanation in this bug.
> > 
> > I'm split between adding more #if/#endif macros and compiling unused code on
> > other platforms.  (I think replacing the long comment with the #if/#endif
> > macros is a win, though.)
> 
> I was hesitant about this too. I'm happy to add the platform guards if you'd
> like me to, but on the other hand the code doesn't use any Android specific
> APIs and works on all platforms. It's just only used by Android right now,
> perhaps the functionality would be useful for others in the future? And the
> same for the new function in LocalStorage. It's only used on Android at the
> moment, but it's platform independent and could be useful for others now or
> later. I can replace the comment with a link to this bug either way.

Yes, a comment above PageGroup::updateLocalStoragePath() referring to the bug would probably be best.

The LocalStorage::updatePath() method was extracted from the constructor, though, so I'm not sure it needs a comment since the call to close() would be required if anyone was to call updatePath() outside of the constructor.

--

You know, after looking at these methods, they seem like a bit of a hack.  Android is only going to update the path once, correct?  Or could it update the database path multiple times while the browser is running?

Would it be possible instead to provide a callback mechanism when opening the LocalStorage databases so that Android could give the correct path "just in time"?

A callback function pointer could be passed/set somewhere, and it would only be used if not-null.  This might also mean that LocalStorage databases should only be opened lazily, but that may not be such a bad thing.

Again, someone more familiar with this code should review the patch.

[Ben Murdoch]

(In reply to comment #5)

Hi David, thanks again for the comments.

> A callback function pointer could be passed/set somewhere, and it would only be
> used if not-null.  This might also mean that LocalStorage databases should only
> be opened lazily, but that may not be such a bad thing.

I've tried a different approach with this today and loading the local storage object only when a page requests it seems to do the trick across all platforms, including android. This makes the changes to WebCore code minimal. I will upload two new patches: one with the ENABLE(DOM_STORAGE) guards, the other with the change for lazily creating the local storage object.

Thanks, Ben

[Ben Murdoch]

Created attachment 31053 [details]
ENABLE(DOM_STORAGE) guards.

Adds macro guards to dom storage source files.

[Ben Murdoch]

Created attachment 31054 [details]
Lazily create localstorage objects for PageGroups.

Lazily creates the local storage when it's requested rather than when a page is added to the page group.

[Eric Seidel (no email)]

Unrelated patches should have their own bugs to keep down review confusion. :(

[Ben Murdoch]

(In reply to comment #9)
> Unrelated patches should have their own bugs to keep down review confusion. :(
> 

In comment #3, ddkilzer recommended splitting the patch into two without creating a new bug.

[Eric Seidel (no email)]

Comment on attachment 31053 [details]
ENABLE(DOM_STORAGE) guards.

Assuming you watch the bots to make sure this doesn't break anybody, r=me.

[Eric Seidel (no email)]

Comment on attachment 31054 [details]
Lazily create localstorage objects for PageGroups.

Is it possible to call PageGroup::localStorage() when there are no pages in the group?  Like during destruction when pages have been removed from the group?  Are pages ever removed from a group?

In general this looks fine.

[Ben Murdoch]

(In reply to comment #12)

Thanks for the reviews and comments Eric!

> Is it possible to call PageGroup::localStorage() when there are no pages in the
> group?  Like during destruction when pages have been removed from the group? 
> Are pages ever removed from a group?

I looked into it today and as far as I can tell, the only point where there could be a problem is when WebKit is being shut down and the local storage objects are closed by the static PageGroup::closeLocalStorage() method. I've altered this method to only attempt to close the storage object if one has been created already.

New patch to follow.

[Ben Murdoch]

Created attachment 31089 [details]
Lazily create LocalStorage object in PageGroup.

Create local storage objects lazily and be sure to only attempt to close a local storage object that has been created.

[David Kilzer (:ddkilzer)]

Comment on attachment 31089 [details]
Lazily create LocalStorage object in PageGroup.

Good catch, Ben!

>-    for (PageGroupMap::iterator it = pageGroups->begin(); it != end; ++it) {
>-        if (LocalStorage* localStorage = it->second->localStorage())
>-            localStorage->close();
>-    }
>+    for (PageGroupMap::iterator it = pageGroups->begin(); it != end; ++it)
>+        it->second->closeLocalStorageInternal();
> #endif
> }
> 
>+#if ENABLE(DOM_STORAGE)
>+void PageGroup::closeLocalStorageInternal()
>+{
>+    // Only close the local storage object if we have one.
>+    if (m_localStorage)
>+        m_localStorage->close();
>+}
>+#endif

Instead of creating a closeLocalStorageInternal() method, I think it would be better to create a hasLocalStorage() method like this in PageGroup.h:

#if ENABLE(DOM_STORAGE)
bool hasLocalStorage() { return m_localStorage; }
#endif

Then the for loop would look like this:

#if ENABLE(DOM_STORAGE)
    for (PageGroupMap::iterator it = pageGroups->begin(); it != end; ++it) {
        if (it->second->hasLocalStorage())
            it->second->localStorage()->close();
    }
#endif

r- for this patch, but r+ with the above changes.

[Ben Murdoch]

(In reply to comment #15)

> r- for this patch, but r+ with the above changes.
> 

Thanks David, I made the changes and a new patch is on the way.

[Ben Murdoch]

Created attachment 31092 [details]
Lazily create LocalStorage object in PageGroup

Update patch based on ddkilzer's suggestions.

[Ben Murdoch]

Created attachment 31093 [details]
Lazily create LocalStorage object in PageGroup.

Fixes small typo in changelog.

[David Kilzer (:ddkilzer)]

Comment on attachment 31093 [details]
Lazily create LocalStorage object in PageGroup.

r=me!

[Adam Barth]

Will land.

[Adam Barth]

Sending        WebCore/ChangeLog
Sending        WebCore/storage/LocalStorage.cpp
Sending        WebCore/storage/LocalStorage.h
Sending        WebCore/storage/LocalStorageArea.cpp
Sending        WebCore/storage/LocalStorageArea.h
Sending        WebCore/storage/LocalStorageTask.cpp
Sending        WebCore/storage/LocalStorageTask.h
Sending        WebCore/storage/LocalStorageThread.cpp
Sending        WebCore/storage/LocalStorageThread.h
Sending        WebCore/storage/SessionStorage.cpp
Sending        WebCore/storage/SessionStorage.h
Sending        WebCore/storage/SessionStorageArea.cpp
Sending        WebCore/storage/SessionStorageArea.h
Sending        WebCore/storage/Storage.cpp
Sending        WebCore/storage/Storage.h
Sending        WebCore/storage/Storage.idl
Sending        WebCore/storage/StorageArea.cpp
Sending        WebCore/storage/StorageArea.h
Sending        WebCore/storage/StorageEvent.cpp
Sending        WebCore/storage/StorageEvent.h
Sending        WebCore/storage/StorageEvent.idl
Sending        WebCore/storage/StorageMap.cpp
Sending        WebCore/storage/StorageMap.h
Transmitting file data .......................
Committed revision 44662.

[Adam Barth]

Sending        WebCore/ChangeLog
Sending        WebCore/page/PageGroup.cpp
Sending        WebCore/page/PageGroup.h
Transmitting file data ...
Committed revision 44663.