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
Ben Murdoch
2009-06-04 10:12:12 PDT
Created attachment 30949 [details]
Proposed patch
Created attachment 30950 [details]
Proposed patch
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. (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. (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. (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 Created attachment 31053 [details]
ENABLE(DOM_STORAGE) guards.
Adds macro guards to dom storage source files.
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.
Unrelated patches should have their own bugs to keep down review confusion. :( (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 on attachment 31053 [details]
ENABLE(DOM_STORAGE) guards.
Assuming you watch the bots to make sure this doesn't break anybody, r=me.
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.
(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. 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 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. (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. Created attachment 31092 [details]
Lazily create LocalStorage object in PageGroup
Update patch based on ddkilzer's suggestions.
Created attachment 31093 [details]
Lazily create LocalStorage object in PageGroup.
Fixes small typo in changelog.
Comment on attachment 31093 [details]
Lazily create LocalStorage object in PageGroup.
r=me!
Will land. 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. Sending WebCore/ChangeLog Sending WebCore/page/PageGroup.cpp Sending WebCore/page/PageGroup.h Transmitting file data ... Committed revision 44663. |