Bug 27072 - Split StorageNamespace and StorageArea into an interface and implementation
Summary: Split StorageNamespace and StorageArea into an interface and implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-08 00:32 PDT by Jeremy Orlow
Modified: 2009-07-08 16:55 PDT (History)
2 users (show)

See Also:


Attachments
v1 (61.86 KB, patch)
2009-07-08 00:52 PDT, Jeremy Orlow
fishd: review+
Details | Formatted Diff | Diff
v2 (62.47 KB, patch)
2009-07-08 13:25 PDT, Jeremy Orlow
fishd: review+
Details | Formatted Diff | Diff
Fix build in release mode (60.27 KB, patch)
2009-07-08 16:27 PDT, Jeremy Orlow
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2009-07-08 00:32:55 PDT
I need to split StorageNamespace and StorageArea into an interface and implementation.  In a later patch, I'll implement a proxy interface that'll run inside the Chromium renderer process.

This is a continuation of other refactoring work: https://bugs.webkit.org/show_bug.cgi?id=25376
Comment 1 Jeremy Orlow 2009-07-08 00:52:24 PDT
Created attachment 32435 [details]
v1

I need to split StorageNamespace and StorageArea into an interface and implementation.  In a later patch, I'll implement a proxy interface that'll run inside the Chromium renderer process.

This is a continuation of other refactoring work: https://bugs.webkit.org/show_bug.cgi?id=25376
Comment 2 Darin Fisher (:fishd, Google) 2009-07-08 10:21:00 PDT
Comment on attachment 32435 [details]
v1

> Index: WebCore/storage/StorageArea.cpp

> +#if PLATFORM(CHROMIUM)
> +#else
> +    return StorageAreaImpl::create(storageType, origin, syncManager);
>  #endif

I think this should be #if !PLATFORM(CHROMIUM) to avoid the #else, or
perhaps you should add a similar FIXME comment there as you do below:


> Index: WebCore/storage/StorageNamespace.cpp

> +#if PLATFORM(CHROMIUM)
> +    // FIXME: Implement.
> +#else
> +    return StorageNamespaceImpl::localStorageNamespace(path);
> +#endif

Otherwise, looks good to me.  R=me
Comment 3 Jeremy Orlow 2009-07-08 13:25:48 PDT
Created attachment 32470 [details]
v2

The main difference is that I took out the Chromium ifdef's and replaced them with an #error for Chromium.
Comment 4 Darin Fisher (:fishd, Google) 2009-07-08 15:27:46 PDT
Landed as:  http://trac.webkit.org/changeset/45643
Comment 5 Darin Fisher (:fishd, Google) 2009-07-08 15:37:17 PDT
I had to revert that change due to build errors.  r45644 was the rollback.
Comment 6 Jeremy Orlow 2009-07-08 16:27:59 PDT
Created attachment 32486 [details]
Fix build in release mode

I'm not sure why this showed up in release mode and not debug (probably optimizations?), but there was a build error in the last patch.  To fix it, I defined 2 virtual functions in StorageArea and made StorageAreaSync use a StorageArea* rather than a StroageAreaImpl* which removed some complex dependencies.
Comment 7 Darin Fisher (:fishd, Google) 2009-07-08 16:49:02 PDT
> I'm not sure why this showed up in release mode and not debug (probably
> optimizations?), but there was a build error in the last patch.  To fix it, I
> defined 2 virtual functions in StorageArea and made StorageAreaSync use a
> StorageArea* rather than a StroageAreaImpl* which removed some complex
> dependencies.

I think it would be better to make StorageAreaSync talk directly to the implementation of StorageArea since StorageAreaSync is in effect part of the implementation of StorageArea.  This can be done as a follow-on patch as far as I am concerned.
Comment 8 Darin Fisher (:fishd, Google) 2009-07-08 16:55:49 PDT
Landed (take 2) as:  http://trac.webkit.org/changeset/45651