Summary: | Unify FILE_SYSTEM and FILE_WRITER enables under the name FILE_SYSTEM. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric U. <ericu> | ||||||||
Component: | DOM | Assignee: | Eric U. <ericu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ctguil, jianli, kinuko | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Eric U.
2010-09-14 18:11:50 PDT
Created attachment 67687 [details]
Patch
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 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. > 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.
Created attachment 67693 [details]
Patch
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. Created attachment 67700 [details]
Patch
(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. (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 on attachment 67700 [details] Patch Clearing flags on attachment: 67700 Committed r67614: <http://trac.webkit.org/changeset/67614> All reviewed patches have been landed. Closing bug. My chromium debug build was complaining that AsyncFileWriterChromium::~AsyncFileWriterChromium has no implementation. Also I believe the destructor should be virtual. (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. |