[webkit-reviews] review denied: [Bug 26629] Refactor localStorage code for use in multi-process browsers (part 2) : [Attachment 31688] patch v1 (corresponds to patch 2 - v2 in the other thread)

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 26629: Refactor localStorage code for use in multi-process browsers (part
2)
https://bugs.webkit.org/show_bug.cgi?id=26629

Attachment 31688: patch v1 (corresponds to patch 2 - v2 in the other thread)
https://bugs.webkit.org/attachment.cgi?id=31688&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
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?