[Webkit-unassigned] [Bug 26629] Refactor localStorage code for use in multi-process browsers (part 2)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26629


beidson at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31688|review?                     |review-
               Flag|                            |




------- Comment #2 from beidson at apple.com  2009-06-22 16:16 PDT -------
(From update of attachment 31688)
In general, a big task like this being split up into multiple patches is a
great thing.  It helps follow the progression of the task and more easily
digest each step.  I appreciate that you've done that.  

Admittedly I haven't looked ahead to patches 3/4/5 yet but that said...

> +// FIXME: In the future, we should allow use of localStorage while it's importing (when safe to do so).
> +// Blocking everything until the import is complete is by far the simplest and safest thing to do, but
> +// there is certainly room for safe optimization: Key/length will never be able to make use of such an
> +// optimization (since the order of iteration can change as items are being added). Get can return any
> +// item currently in the map. Get/remove can work whether or not it's in the map, but we'll need a list
> +// of items the import should not overwrite. Clear can also work, but it'll need to kill the import
> +// job first.

What this FIXME isn't saying is that most of the optimizations it refers to
currently exist, but this patch is removing them.  I don't think it's okay to
have that regression, even temporarily.

This patch seems fine on the surface, except for this regression.  Does 3/4/5
restore the optimization?


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.