[Webkit-unassigned] [Bug 28019] WINCE PORT: modified files in WebCore/storage

[bugzilla-daemon at webkit.org]

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


Jeremy Orlow <jorlow at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jorlow at chromium.org




--- Comment #12 from Jeremy Orlow <jorlow at chromium.org>  2009-08-18 13:01:09 PDT ---
This is going to make things quite a bit uglier.

Maybe (In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 34135 [details] [details])
> > Well, I don't know what this change is doing from the ChangeLog.  I assume
> > you're trying to make it possible to run Database operations on the main
> > thread?
> > 
> > Redirecting to a platform-specific header is generally never the right
> > approach:
> > #if PLATFORM(WINCE)
> >  35 
> >  36 #include "wince/DatabaseThreadWince.h"
> >  37 
> >  38 #else
> > 
> > The abstraction (whatever you're trying to do there) likely belongs at a
> > different/higher level.  It would be bad, for example if 5 ports all had
> > redirects there.
> 
> Your assumption is correct.  We are of the understanding that few if any will
> actually want this.  If it's desired by others, I agree it should be
> refactored.  In KDE we had a policy that something doesn't become an API until
> at least 2 separate users of it exist, otherwise it's a special case.  I think
> that's prudent for WebKit too.  I don't see the reason to burden the main code
> until we have evidence that someone else wants to use it too.  Quite honestly,
> it's a bit ugly to have a multithreaded and a singlethreaded version in the
> same place so I'd rather bury this ugly one away.

Adding ifdefs like this really uglies up the code and is not necessary.  The
abstraction layer is already there: it's StorageAreaSync.  The class's API is
as follows:

void scheduleFinalSync();
void blockUntilImportComplete() const;
void scheduleItemForSync(const String& key, const String& value);
void scheduleClear();

blockUntilImportComplete could synchronously read in the data. 
scheduleItemForSync could immediately write an item out.  scheduleClear could
immediately clear the data.  scheduleFinalSync could be a no-op.

I believe you can easily/cleanly change things so the StorageSyncManager
needn't be instantiated for your port.  If you needed ideas on how, I could
take a closer look.

For WINCE, you could just not compile LocalStorageThread.cpp,
LocalStorageTask.cpp, StorageAreaSync.cpp, and StorageAreaManager.cpp and
instead provide your own versions (if any) of these files (which would be in a
singleThread or wince directory).  We use this trick all the time in Chromium
(for example, with StorageNamespace.cpp).

Let me know if you need any more specific advice, but please don't commit this
patch as is.

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