Bug 26189

Summary: DOM Storage: Changes for Android and macro guards
Product: WebKit Reporter: Ben Murdoch <benm>
Component: WebCore Misc.Assignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andreip, beidson, ddkilzer, eric, jorlow, mjs
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Other   
Attachments:
Description Flags
Proposed patch
none
Proposed patch
none
ENABLE(DOM_STORAGE) guards.
eric: review+
Lazily create localstorage objects for PageGroups.
none
Lazily create LocalStorage object in PageGroup.
ddkilzer: review-
Lazily create LocalStorage object in PageGroup
none
Lazily create LocalStorage object in PageGroup. ddkilzer: review+

Description Ben Murdoch 2009-06-04 10:12:12 PDT
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.
Comment 1 Ben Murdoch 2009-06-04 10:12:43 PDT
Created attachment 30949 [details]
Proposed patch
Comment 2 Ben Murdoch 2009-06-04 10:16:28 PDT
Created attachment 30950 [details]
Proposed patch
Comment 3 David Kilzer (:ddkilzer) 2009-06-05 08:02:44 PDT
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.
Comment 4 Ben Murdoch 2009-06-05 08:16:32 PDT
(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.
Comment 5 David Kilzer (:ddkilzer) 2009-06-05 08:48:54 PDT
(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.
Comment 6 Ben Murdoch 2009-06-08 11:03:54 PDT
(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
Comment 7 Ben Murdoch 2009-06-08 11:07:52 PDT
Created attachment 31053 [details]
ENABLE(DOM_STORAGE) guards.

Adds macro guards to dom storage source files.
Comment 8 Ben Murdoch 2009-06-08 11:16:56 PDT
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.
Comment 9 Eric Seidel (no email) 2009-06-08 13:09:29 PDT
Unrelated patches should have their own bugs to keep down review confusion. :(
Comment 10 Ben Murdoch 2009-06-08 14:41:20 PDT
(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.
Comment 11 Eric Seidel (no email) 2009-06-08 18:06:35 PDT
Comment on attachment 31053 [details]
ENABLE(DOM_STORAGE) guards.

Assuming you watch the bots to make sure this doesn't break anybody, r=me.
Comment 12 Eric Seidel (no email) 2009-06-08 18:09:16 PDT
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.
Comment 13 Ben Murdoch 2009-06-09 06:08:00 PDT
(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.
Comment 14 Ben Murdoch 2009-06-09 06:11:44 PDT
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.
Comment 15 David Kilzer (:ddkilzer) 2009-06-09 06:44:23 PDT
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.
Comment 16 Ben Murdoch 2009-06-09 07:08:55 PDT
(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.


Comment 17 Ben Murdoch 2009-06-09 07:10:06 PDT
Created attachment 31092 [details]
Lazily create LocalStorage object in PageGroup

Update patch based on ddkilzer's suggestions.
Comment 18 Ben Murdoch 2009-06-09 07:12:06 PDT
Created attachment 31093 [details]
Lazily create LocalStorage object in PageGroup.

Fixes small typo in changelog.
Comment 19 David Kilzer (:ddkilzer) 2009-06-09 09:14:45 PDT
Comment on attachment 31093 [details]
Lazily create LocalStorage object in PageGroup.

r=me!
Comment 20 Adam Barth 2009-06-13 20:35:35 PDT
Will land.
Comment 21 Adam Barth 2009-06-13 20:45:21 PDT
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.
Comment 22 Adam Barth 2009-06-13 20:48:10 PDT
Sending        WebCore/ChangeLog
Sending        WebCore/page/PageGroup.cpp
Sending        WebCore/page/PageGroup.h
Transmitting file data ...
Committed revision 44663.