Bug 45798 - Unify FILE_SYSTEM and FILE_WRITER enables under the name FILE_SYSTEM.
Summary: Unify FILE_SYSTEM and FILE_WRITER enables under the name FILE_SYSTEM.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric U.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-14 18:11 PDT by Eric U.
Modified: 2010-09-17 11:34 PDT (History)
4 users (show)

See Also:


Attachments
Patch (38.70 KB, patch)
2010-09-15 10:21 PDT, Eric U.
no flags Details | Formatted Diff | Diff
Patch (43.43 KB, patch)
2010-09-15 11:37 PDT, Eric U.
no flags Details | Formatted Diff | Diff
Patch (43.57 KB, patch)
2010-09-15 12:38 PDT, Eric U.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2010-09-14 18:11:50 PDT
It doesn't make sense to enable FILE_SYSTEM without FILE_WRITER, and our in-progress implementation of FileWriter doesn't do anything without FileSystem.  So to simplify code and build files, we should just use the FILE_SYSTEM guard for now.
Comment 1 Eric U. 2010-09-15 10:21:27 PDT
Created attachment 67687 [details]
Patch
Comment 2 Jian Li 2010-09-15 10:41:26 PDT
Comment on attachment 67687 [details]
Patch

Do you also need to change WebKit/chromium/AsyncFileWriterChromium.*?

I also notice that ENABLE_FILE_SYSTEM is not defined in WebKit/chromium/features.gypi.

View in context: https://bugs.webkit.org/attachment.cgi?id=67687&action=prettypatch

> configure.ac:-913
> -AM_CONDITIONAL([ENABLE_FILE_WRITER],[test "$enable_file_writer" = "yes"])
I think you also need to remove another occurrence of file_writer usage in this file.

> WebCore/ChangeLog:5
> +        Unify FILE_SYSTEM and FILE_WRITER enables
Please mention that you're moving towards FILE_SYSTEM guard.

> WebCore/ChangeLog:8
> +        No new tests.
This line can be removed. No need to mention it if we do not have specific thing to say here.
Comment 3 Eric U. 2010-09-15 11:00:35 PDT
Comment on attachment 67687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67687&action=prettypatch

Re: AsyncFileWriterChromium.*: Yes, my client was out of sync.  I'm merging out and will repost shortly.  Thanks for the super-speedy review!

> configure.ac:-913
> -AM_CONDITIONAL([ENABLE_FILE_WRITER],[test "$enable_file_writer" = "yes"])
Fixed.

> WebCore/ChangeLog:5
> +        Unify FILE_SYSTEM and FILE_WRITER enables
Fixed.

> WebCore/ChangeLog:8
> +        No new tests.
Done.
Comment 4 Eric U. 2010-09-15 11:33:30 PDT
> I also notice that ENABLE_FILE_SYSTEM is not defined in WebKit/chromium/features.gypi.

I expect Kinuko will decide to turn that on quite soon; I don't want to do it without her advice.
Comment 5 Eric U. 2010-09-15 11:37:07 PDT
Created attachment 67693 [details]
Patch
Comment 6 Jian Li 2010-09-15 11:58:24 PDT
Comment on attachment 67693 [details]
Patch

One last thing.

View in context: https://bugs.webkit.org/attachment.cgi?id=67693&action=prettypatch

> WebCore/ChangeLog:5
> +        Unify FILE_SYSTEM and FILE_WRITER enables under the name FILE_SYSTEM.
Please also update the bug title and all other ChangeLog to use the same description for consistency.
Comment 7 Eric U. 2010-09-15 12:38:48 PDT
Created attachment 67700 [details]
Patch
Comment 8 Eric U. 2010-09-15 12:39:30 PDT
(In reply to comment #6)
> (From update of attachment 67693 [details])
> One last thing.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=67693&action=prettypatch
> 
> > WebCore/ChangeLog:5
> > +        Unify FILE_SYSTEM and FILE_WRITER enables under the name FILE_SYSTEM.
> Please also update the bug title and all other ChangeLog to use the same description for consistency.

Sorry about that--I got half of them, but should have checked more carefully.
All fixed.
Comment 9 Kinuko Yasuda 2010-09-15 12:50:18 PDT
(In reply to comment #4)
> > I also notice that ENABLE_FILE_SYSTEM is not defined in WebKit/chromium/features.gypi.
> 
> I expect Kinuko will decide to turn that on quite soon; I don't want to do it without her advice.

It's turned on in chromium side (build/features_override.gypi) mainly because the last big change was in chromium.  If you're going to update the patch one more time (not likely?) please feel free to add it.  Otherwise I'll add the flag in my next patch so no need to worry...  Thanks,
(Btw I think we can remove the FILE_READER in WebKit/chromium/features.gypi now.)
Comment 10 WebKit Commit Bot 2010-09-16 06:45:12 PDT
Comment on attachment 67700 [details]
Patch

Clearing flags on attachment: 67700

Committed r67614: <http://trac.webkit.org/changeset/67614>
Comment 11 WebKit Commit Bot 2010-09-16 06:45:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Chris Guillory 2010-09-17 10:27:32 PDT
My chromium debug build was complaining that AsyncFileWriterChromium::~AsyncFileWriterChromium has no implementation. Also I believe the destructor should be virtual.
Comment 13 Eric U. 2010-09-17 11:34:23 PDT
(In reply to comment #12)
> My chromium debug build was complaining that AsyncFileWriterChromium::~AsyncFileWriterChromium has no implementation. Also I believe the destructor should be virtual.

Chris, you're quite right--there's a bug there.
As a workaround, just delete the declaration of the destructor in AsyncFileWriterChromium; it's already there [virtually] from its parent interface.
I'll put a fix up shortly--sorry about the inconvenience.