[Webkit-unassigned] [Bug 28019] WINCE PORT: modified files in WebCore/storage
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 18 13:32:41 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=28019
--- Comment #16 from Jeremy Orlow <jorlow at chromium.org> 2009-08-18 13:32:39 PDT ---
First of all, good point on being sync and single threaded being different.
Got mixed up there.
(In reply to comment #14)
> >
> > Let me know if you need any more specific advice, but please don't commit this
> > patch as is.
>
> We could implement and maintain any file by our own. but the point is trying to
> share code with other ports without adding pains. Not saying we're going to
> commit it as it is. but I cannot see why a single-threaded solution shouldn't
> go in.
I think a single-threaded solution should go in, but not as a pile of #ifdefs.
I'd be OK with having _one_ section of StorageAreaSync and and
StorageSyncManager within #ifdef's, but I'm not a reviewer so I'm not sure if
even that is typically acceptable.
Here are some specific comments:
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index e597233..53be1d0 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,25 @@
> +2009-08-05 Yong Li <yong.li at torchmobile.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Need a short description and bug URL (OOPS!)
> + https://bugs.webkit.org/show_bug.cgi?id=28019
> +
> + * storage/DatabaseThread.h:
> + * storage/LocalStorageThread.h:
> + * storage/StorageAreaSync.cpp:
> + (WebCore::StorageAreaSync::StorageAreaSync):
> + (WebCore::StorageAreaSync::performImport):
> + (WebCore::StorageAreaSync::sync):
> + (WebCore::StorageAreaSync::performSync):
> + * storage/StorageAreaSync.h:
> + (WebCore::StorageAreaSync::blockUntilImportComplete):
> + (WebCore::StorageAreaSync::markImported):
> + * storage/StorageSyncManager.cpp:
> + (WebCore::StorageSyncManager::close):
> + (WebCore::StorageSyncManager::scheduleImport):
> + (WebCore::StorageSyncManager::scheduleSync):
> +
> 2009-07-21 Yong Li <yong.li at torchmobile.com>
>
> Reviewed by NOBODY (OOPS!).
> diff --git a/WebCore/storage/DatabaseThread.h b/WebCore/storage/DatabaseThread.h
> index 5aab5fd..c744b86 100644
> --- a/WebCore/storage/DatabaseThread.h
> +++ b/WebCore/storage/DatabaseThread.h
> @@ -1,5 +1,6 @@
> /*
> * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.
> + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions
> @@ -29,6 +30,13 @@
> #define DatabaseThread_h
>
> #if ENABLE(DATABASE)
> +
> +#if PLATFORM(WINCE)
> +
> +#include "wince/DatabaseThreadWince.h"
> +
> +#else
> +
> #include <wtf/Deque.h>
> #include <wtf/HashMap.h>
> #include <wtf/HashSet.h>
> @@ -79,5 +87,7 @@ private:
>
> } // namespace WebCore
>
> +#endif // PLATFORM(WINCE)
> +
> #endif // ENABLE(DATABASE)
> #endif // DatabaseThread_h
> diff --git a/WebCore/storage/LocalStorageThread.h b/WebCore/storage/LocalStorageThread.h
> index 3d58427..45178e8 100644
> --- a/WebCore/storage/LocalStorageThread.h
> +++ b/WebCore/storage/LocalStorageThread.h
> @@ -1,5 +1,6 @@
> /*
> * Copyright (C) 2008 Apple Inc. All Rights Reserved.
> + * Copyright (C) 2009 Torch Mobile, Inc. All Rights Reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions
> @@ -28,6 +29,12 @@
>
> #if ENABLE(DOM_STORAGE)
>
> +#if PLATFORM(WINCE)
> +
> +#include "wince/LocalStorageThreadWince.h"
Is there anything in this file? Maybe the above #if should say
ENABLE(DOM_STORAGE) && !PLATFORM(WINCE) ?
Or maybe you can (with few or no ifdefs) eliminate any .h file dependencies on
this?
> +
> +#else
> +
> #include <wtf/HashSet.h>
> #include <wtf/MessageQueue.h>
> #include <wtf/PassRefPtr.h>
> @@ -71,6 +78,8 @@ namespace WebCore {
>
> } // namespace WebCore
>
> +#endif // PLATFORM(WINCE)
> +
> #endif // ENABLE(DOM_STORAGE)
>
> #endif // LocalStorageThread_h
> diff --git a/WebCore/storage/StorageAreaSync.cpp b/WebCore/storage/StorageAreaSync.cpp
> index 01d2a65..5182fd6 100644
> --- a/WebCore/storage/StorageAreaSync.cpp
> +++ b/WebCore/storage/StorageAreaSync.cpp
> @@ -55,8 +55,13 @@ StorageAreaSync::StorageAreaSync(PassRefPtr<StorageSyncManager> storageSyncManag
> , m_syncManager(storageSyncManager)
> , m_clearItemsWhileSyncing(false)
> , m_syncScheduled(false)
> +#if !PLATFORM(WINCE)
> , m_importComplete(false)
> +#endif
> {
> +#if PLATFORM(WINCE)
> + performImport();
> +#else
> ASSERT(m_storageArea);
> ASSERT(m_syncManager);
>
> @@ -64,6 +69,7 @@ StorageAreaSync::StorageAreaSync(PassRefPtr<StorageSyncManager> storageSyncManag
> // not silently ignoring it. https://bugs.webkit.org/show_bug.cgi?id=25894
> if (!m_syncManager->scheduleImport(this))
> m_importComplete = true;
> +#endif
Why not just implement your own syncManager and have it work as you'd like it
to?
> }
>
> StorageAreaSync::~StorageAreaSync()
> @@ -160,7 +166,9 @@ void StorageAreaSync::syncTimerFired(Timer<StorageAreaSync>*)
>
> void StorageAreaSync::performImport()
> {
> +#if !PLATFORM(WINCE)
> ASSERT(!isMainThread());
> +#endif
> ASSERT(!m_database.isOpen());
>
> String databaseFilename = m_syncManager->fullDatabaseFilename(m_storageArea->securityOrigin());
> @@ -204,7 +212,9 @@ void StorageAreaSync::performImport()
> return;
> }
>
> +#if !PLATFORM(WINCE)
> MutexLocker locker(m_importLock);
> +#endif
>
> HashMap<String, String>::iterator it = itemMap.begin();
> HashMap<String, String>::iterator end = itemMap.end();
> @@ -212,12 +222,15 @@ void StorageAreaSync::performImport()
> for (; it != end; ++it)
> m_storageArea->importItem(it->first, it->second);
>
> +#if !PLATFORM(WINCE)
> // Break the (ref count) cycle.
> m_storageArea = 0;
> m_importComplete = true;
> m_importCondition.signal();
> +#endif
> }
>
> +#if !PLATFORM(WINCE)
> void StorageAreaSync::markImported()
> {
> ASSERT(!isMainThread());
> @@ -250,10 +263,13 @@ void StorageAreaSync::blockUntilImportComplete() const
> ASSERT(m_importComplete);
> ASSERT(!m_storageArea);
> }
> +#endif
>
> void StorageAreaSync::sync(bool clearItems, const HashMap<String, String>& items)
> {
> +#if !PLATFORM(WINCE)
> ASSERT(!isMainThread());
> +#endif
>
> if (!m_database.isOpen())
> return;
> @@ -309,7 +325,9 @@ void StorageAreaSync::sync(bool clearItems, const HashMap<String, String>& items
>
> void StorageAreaSync::performSync()
> {
> +#if !PLATFORM(WINCE)
> ASSERT(!isMainThread());
> +#endif
How about making this a function somewhere. isBackgroundThread. This could be
implemented either here or the StorageSyncManager.
The implementation for single threaded could just return true. The normal
implementation could be !isMainThread();
>
> bool clearItems;
> HashMap<String, String> items;
> diff --git a/WebCore/storage/StorageAreaSync.h b/WebCore/storage/StorageAreaSync.h
> index e436bef..fa64984 100644
> --- a/WebCore/storage/StorageAreaSync.h
> +++ b/WebCore/storage/StorageAreaSync.h
> @@ -46,7 +46,11 @@ namespace WebCore {
> ~StorageAreaSync();
>
> void scheduleFinalSync();
> +#if PLATFORM(WINCE)
> + void blockUntilImportComplete() const {}
> +#else
> void blockUntilImportComplete() const;
> +#endif
There's no need to make this inline.
>
> void scheduleItemForSync(const String& key, const String& value);
> void scheduleClear();
> @@ -83,10 +87,14 @@ namespace WebCore {
> bool m_clearItemsWhileSyncing;
> bool m_syncScheduled;
>
> +#if PLATFORM(WINCE)
> + void markImported() {}
> +#else
> mutable Mutex m_importLock;
> mutable ThreadCondition m_importCondition;
> mutable bool m_importComplete;
> void markImported();
> +#endif
It's a nit, but I don't see why markImported should be inlined here. The mutex
and condition could just be ifdef'ed off with no #else.
> diff --git a/WebCore/storage/StorageSyncManager.cpp b/WebCore/storage/StorageSyncManager.cpp
> index a935242..b90d180 100644
> --- a/WebCore/storage/StorageSyncManager.cpp
> +++ b/WebCore/storage/StorageSyncManager.cpp
> @@ -72,7 +72,9 @@ String StorageSyncManager::fullDatabaseFilename(SecurityOrigin* origin)
>
> void StorageSyncManager::close()
> {
> +#if !PLATFORM(WINCE)
> ASSERT(isMainThread());
> +#endif
Why are these necessary? Since there's only one thread, wouldn't it always be
on the main thread?
If it's absolutely necessary, it could be factored into a function like I
mentioned above.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list