Bug 40606 - Fix compilation errors in BlobBuilder with FILE_WRITER enabled
Summary: Fix compilation errors in BlobBuilder with FILE_WRITER enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-14 19:55 PDT by Kinuko Yasuda
Modified: 2010-06-15 16:36 PDT (History)
1 user (show)

See Also:


Attachments
Patch (1.54 KB, patch)
2010-06-14 20:10 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (1.61 KB, patch)
2010-06-15 00:12 PDT, Kinuko Yasuda
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2010-06-14 19:55:06 PDT
There're some compilation erros in BlobBuilder when it's built with FILE_WRITER_ENABLED flag.
Comment 1 Kinuko Yasuda 2010-06-14 20:10:42 PDT
Created attachment 58743 [details]
Patch
Comment 2 David Levin 2010-06-14 23:21:28 PDT
Comment on attachment 58743 [details]
Patch

WebCore/ChangeLog:1
 +  2010-06-14  Kinuko Yasuda  <kinuko@kinuko2-macpro.mtv.corp.google.com>
The email address needs to be fixed.


WebCore/ChangeLog:8
 +          No new tests.
This should explain why no new tests were added. For example:
"No functionality change so no new tests."


WebCore/html/BlobBuilder.h:38
 +  #include "PlatformString.h"
Actually why is PlatformString.h included? (Can't a forward declaration for String suffice?)


WebCore/html/BlobBuilder.h:41
 +  #include <wtf/text/CString.h>
I believe you're including this due to the destructor needing it. If you explicitly define the destructor for BlobBuilder or even ~StringBlobItem and put it in a cpp file, then this include shouldn't be needed.

But this header already "#include BlobItem.h" which "#include <wtf/text/CString.h>", so it is unclear why this is needed to fix the build.


WebCore/html/BlobBuilder.h: 43
 +  class ExceptionCode;

Typically Exception code is defined like this:
  typedef int ExceptionCode;
and ExceptionCode.h isn't included. (Also if you add this line, it should come after the class Blob; forward declaration.)
Comment 3 Kinuko Yasuda 2010-06-15 00:12:52 PDT
Created attachment 58761 [details]
Patch
Comment 4 Kinuko Yasuda 2010-06-15 00:20:23 PDT
Thanks for reviewing; the patch was a bit rough.

(In reply to comment #2)
> (From update of attachment 58743 [details])
> WebCore/ChangeLog:1
>  +  2010-06-14  Kinuko Yasuda  <kinuko@kinuko2-macpro.mtv.corp.google.com>
> The email address needs to be fixed.

Fixed.

> WebCore/ChangeLog:8
>  +          No new tests.
> This should explain why no new tests were added. For example:
> "No functionality change so no new tests."

Fixed.

> WebCore/html/BlobBuilder.h:38
>  +  #include "PlatformString.h"
> WebCore/html/BlobBuilder.h:41
>  +  #include <wtf/text/CString.h>

Both lines weren't necessary.  Removed them and added a forward declaration for String.
(I had needed to include wtf/text/CString.h because I had included PlatformString.h that wasn't really necessary.)

> WebCore/html/BlobBuilder.h: 43
>  +  class ExceptionCode;
> 
> Typically Exception code is defined like this:
>   typedef int ExceptionCode;
> and ExceptionCode.h isn't included. (Also if you add this line, it should come after the class Blob; forward declaration.)

Right, the forward declaration actually didn't work (so I had removed the line and added '#include "BlobBuilder.h"' in the previous patch).

I replaced the line with a typedef.
Comment 5 David Levin 2010-06-15 05:51:50 PDT
Comment on attachment 58761 [details]
Patch

\
Comment 6 Kinuko Yasuda 2010-06-15 16:36:24 PDT
Committed r61225: <http://trac.webkit.org/changeset/61225>