[webkit-reviews] review denied: [Bug 26189] DOM Storage: Changes for Android and macro guards : [Attachment 31089] Lazily create LocalStorage object in PageGroup.

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Ben Murdoch
<benm at google.com>'s request for review:
Bug 26189: DOM Storage: Changes for Android and macro guards
https://bugs.webkit.org/show_bug.cgi?id=26189

Attachment 31089: Lazily create LocalStorage object in PageGroup.
https://bugs.webkit.org/attachment.cgi?id=31089&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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.