Bug 45798 – Unify FILE_SYSTEM and FILE_WRITER enables under the name FILE_SYSTEM.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric U.]

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.

[Eric U.]

Created attachment 67687 [details]
Patch

[Jian Li]

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.

[Eric U.]

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.

[Eric U.]

> 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.

[Eric U.]

Created attachment 67693 [details]
Patch

[Jian Li]

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.

[Eric U.]

Created attachment 67700 [details]
Patch

[Eric U.]

(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.

[Kinuko Yasuda]

(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.)

[WebKit Commit Bot]

Comment on attachment 67700 [details]
Patch

Clearing flags on attachment: 67700

Committed r67614: <http://trac.webkit.org/changeset/67614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Guillory]

My chromium debug build was complaining that AsyncFileWriterChromium::~AsyncFileWriterChromium has no implementation. Also I believe the destructor should be virtual.

[Eric U.]

(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.