[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
----------------------------------------------------------------------------
         AssignedTo|webkit-unassigned at lists.web |yong.li at torchmobile.com
                   |kit.org                     |




--- Comment #20 from Jeremy Orlow <jorlow at chromium.org>  2009-08-18 15:32:49 PDT ---
Looking better!  Here's some comments:

Ignoring all DB code.  (Not sure, but it might be best if you broke it into its
own patch/bug.)

> diff --git a/WebCore/storage/LocalStorageThread.h b/WebCore/storage/LocalStorageThread.h
> index 3d58427..176e279 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.

My understanding is that copyright changes should only be made for substantial
contributions to a file.  (This applies to many of the other files below as
well.

>   *
>   * 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 ENABLE(SINGLE_THREADED)
> +
> +#include "single-threaded/LocalStorageThread.h"
> +
> +#else

Instead of this, please do what you did for StorageAreaSync.h.  It's much
cleaner, IMHO.  Sharing .cpp code is nice, but I think sharing .h code is
essential.

> diff --git a/WebCore/storage/single-threaded/LocalStorageThread.cpp b/WebCore/storage/single-threaded/LocalStorageThread.cpp
> new file mode 100644
> index 0000000..8dcb902
> --- /dev/null
> +++ b/WebCore/storage/single-threaded/LocalStorageThread.cpp
> @@ -0,0 +1,83 @@
> +/*
> + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved.

There are still chunks of existing code in here, right?  If so, you should
probably add Apple's name back to the copyright.

> diff --git a/WebCore/storage/single-threaded/StorageAreaSync.cpp b/WebCore/storage/single-threaded/StorageAreaSync.cpp
> new file mode 100644
> index 0000000..ac21e7c
> --- /dev/null
> +++ b/WebCore/storage/single-threaded/StorageAreaSync.cpp

There sure is a lot of copied code in here.  Why not just put the functions
that are completely different in here, #ifdef out the ones you define here in
the main file...but do it in _one_ chunk (i.e. move them all to the end so you
can do it with one ifdef), and factor out any little differences into their own
functions?

Also, if there are any files that you copy whole-sale, you probably need to svn
cp them.  I know you use git and git can't do this, but I think it's worth it
to be able to see what's copied and what's not.

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