Bug 26189 - DOM Storage: Changes for Android and macro guards
: DOM Storage: Changes for Android and macro guards
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All Other
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-06-04 10:12 PST by
Modified: 2009-06-13 20:48 PST (History)


Attachments
Proposed patch (17.20 KB, patch)
2009-06-04 10:12 PST, Ben Murdoch
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (17.22 KB, patch)
2009-06-04 10:16 PST, Ben Murdoch
no flags Review Patch | Details | Formatted Diff | Diff
ENABLE(DOM_STORAGE) guards. (12.26 KB, patch)
2009-06-08 11:07 PST, Ben Murdoch
eric: review+
Review Patch | Details | Formatted Diff | Diff
Lazily create localstorage objects for PageGroups. (1.89 KB, patch)
2009-06-08 11:16 PST, Ben Murdoch
no flags Review Patch | Details | Formatted Diff | Diff
Lazily create LocalStorage object in PageGroup. (3.38 KB, patch)
2009-06-09 06:11 PST, Ben Murdoch
ddkilzer: review-
Review Patch | Details | Formatted Diff | Diff
Lazily create LocalStorage object in PageGroup (3.04 KB, patch)
2009-06-09 07:10 PST, Ben Murdoch
no flags Review Patch | Details | Formatted Diff | Diff
Lazily create LocalStorage object in PageGroup. (3.03 KB, patch)
2009-06-09 07:12 PST, Ben Murdoch
ddkilzer: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-04 10:12:12 PST
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 From 2009-06-04 10:12:43 PST -------
Created an attachment (id=30949) [details]
Proposed patch
------- Comment #2 From 2009-06-04 10:16:28 PST -------
Created an attachment (id=30950) [details]
Proposed patch
------- Comment #3 From 2009-06-05 08:02:44 PST -------
(From update of attachment 30950 [details])
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 From 2009-06-05 08:16:32 PST -------
(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 From 2009-06-05 08:48:54 PST -------
(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 From 2009-06-08 11:03:54 PST -------
(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 From 2009-06-08 11:07:52 PST -------
Created an attachment (id=31053) [details]
ENABLE(DOM_STORAGE) guards.

Adds macro guards to dom storage source files.
------- Comment #8 From 2009-06-08 11:16:56 PST -------
Created an attachment (id=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 From 2009-06-08 13:09:29 PST -------
Unrelated patches should have their own bugs to keep down review confusion. :(
------- Comment #10 From 2009-06-08 14:41:20 PST -------
(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 From 2009-06-08 18:06:35 PST -------
(From update of attachment 31053 [details])
Assuming you watch the bots to make sure this doesn't break anybody, r=me.
------- Comment #12 From 2009-06-08 18:09:16 PST -------
(From update of attachment 31054 [details])
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 From 2009-06-09 06:08:00 PST -------
(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 From 2009-06-09 06:11:44 PST -------
Created an attachment (id=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 From 2009-06-09 06:44:23 PST -------
(From update of attachment 31089 [details])
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 From 2009-06-09 07:08:55 PST -------
(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 From 2009-06-09 07:10:06 PST -------
Created an attachment (id=31092) [details]
Lazily create LocalStorage object in PageGroup

Update patch based on ddkilzer's suggestions.
------- Comment #18 From 2009-06-09 07:12:06 PST -------
Created an attachment (id=31093) [details]
Lazily create LocalStorage object in PageGroup.

Fixes small typo in changelog.
------- Comment #19 From 2009-06-09 09:14:45 PST -------
(From update of attachment 31093 [details])
r=me!
------- Comment #20 From 2009-06-13 20:35:35 PST -------
Will land.
------- Comment #21 From 2009-06-13 20:45:21 PST -------
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 From 2009-06-13 20:48:10 PST -------
Sending        WebCore/ChangeLog
Sending        WebCore/page/PageGroup.cpp
Sending        WebCore/page/PageGroup.h
Transmitting file data ...
Committed revision 44663.