Bug 11257 - Remove mac platform dependance from Loader::ReceivedResponse() and CachedImage::createImage()
Summary: Remove mac platform dependance from Loader::ReceivedResponse() and CachedImag...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: David Carson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-10-11 12:50 PDT by David Carson
Modified: 2006-10-14 03:27 PDT (History)
2 users (show)

See Also:


Attachments
Patch (6.09 KB, patch)
2006-10-11 13:00 PDT, David Carson
no flags Details | Formatted Diff | Diff
Updated patch (6.72 KB, patch)
2006-10-11 13:36 PDT, David Carson
mjs: review-
Details | Formatted Diff | Diff
Included fixes from mjs's comment (11.02 KB, patch)
2006-10-12 10:02 PDT, David Carson
no flags Details | Formatted Diff | Diff
missed some non-mac build items (12.14 KB, patch)
2006-10-12 10:51 PDT, David Carson
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Carson 2006-10-11 12:50:18 PDT
The logic in receivedResponse can be used by more than just the mac platform if the functions CacheObjectExpiresTime() and ResponseIsMultipart() were made platform independant. The functions are defined in a non-platform header file, LoaderFunctions.h
Comment 1 David Carson 2006-10-11 13:00:56 PDT
Created attachment 11036 [details]
Patch

Modified link stubs for Windows and Qt as there are two functions that should to be implemented to enable use of the Loader::receivedResponse() function. These functions update the memory cache status of the response.
Comment 2 David Carson 2006-10-11 13:33:22 PDT
CachedImage also does not need to be special cased. By passing PlatformRequest to ResponseMIMEType(), platforms can make their own implementation.
Comment 3 David Carson 2006-10-11 13:36:12 PDT
Created attachment 11038 [details]
Updated patch

Created new patch to include changes for ResponseMIMEType()
Comment 4 Maciej Stachowiak 2006-10-12 02:41:38 PDT
Comment on attachment 11038 [details]
Updated patch

Looks generally ok, but here's some things I'm sure about:

-#if __APPLE__
         m_image = new Image(this, ResponseMIMEType(m_response) == "application/pdf");
-#else
-        m_image = new Image(this, false);
-#endif

I don't think any platform but Mac supports PDF images yet, so I'm not sure this is right. but maybe Image will fail gracefully.


As a matter of style, we don't usually use this kind of initialization for non-class types, just = would be better.

+    NSURLResponse* response(platResponse);

r- for now to consider these comments.
Comment 5 Dave Hyatt 2006-10-12 02:44:36 PDT
The PDF #ifdef needs to be kept.  It should be changed to PLATFORM(CG) though.
Comment 6 David Carson 2006-10-12 08:42:49 PDT
(In reply to comment #4)
> (From update of attachment 11038 [details] [edit])
> Looks generally ok, but here's some things I'm sure about:
> 
> -#if __APPLE__
>          m_image = new Image(this, ResponseMIMEType(m_response) ==
> "application/pdf");
> -#else
> -        m_image = new Image(this, false);
> -#endif
> 
> I don't think any platform but Mac supports PDF images yet, so I'm not sure
> this is right. but maybe Image will fail gracefully.

it will fail gracefully in Image.cpp

> 
> As a matter of style, we don't usually use this kind of initialization for
> non-class types, just = would be better.
> 
> +    NSURLResponse* response(platResponse);

Because:
typedef NSURLResponse* PlatformResponse;
the line is not needed at all. I have removed these.

> r- for now to consider these comments.
> 

Comment 7 David Carson 2006-10-12 09:17:13 PDT
(In reply to comment #5)
> The PDF #ifdef needs to be kept.  It should be changed to PLATFORM(CG) though.
> 

I disagree. Image class will handle this nicely. If PDF Image support should be 'hidden' to Apple only, then the second arguement for the Image object should not exist for other platforms and the constructor be within an #ifdef
I can see no harm in removing this #ifdef as Image object ignores the parameter anyway for any other platform but CG
Comment 8 David Carson 2006-10-12 10:02:28 PDT
Created attachment 11046 [details]
Included fixes from mjs's comment

I made a more complete patch, which corrects all instances of NSURLResponse in LoaderFunctions.h 
This also includes a correction to CachedResource to store PlatformResponse and PlatformData rather than the Apple specific NSURLResponse and NSData.
The definition for PlatformData and PlatformResponse is in ResourceLoaderClient.h
Comment 9 David Carson 2006-10-12 10:51:38 PDT
Created attachment 11047 [details]
missed some non-mac build items

Missed adding these functions to the TemporaryLinkStubs.cpp
Check mac implementation to determine what needs to be done for these functions.
CachedResource::setResponse(PlatformResponse) { notImplemented(); }
CachedResource::setAllData(PlatformData) { notImplemented(); }
Comment 10 Maciej Stachowiak 2006-10-13 13:46:45 PDT
Comment on attachment 11047 [details]
missed some non-mac build items

r=me
Comment 11 Mark Rowe (bdash) 2006-10-14 02:47:12 PDT
Landed in r17056.
Comment 12 Dave Hyatt 2006-10-14 03:26:30 PDT
This is fine for now.  The PDF thing is basically all wrong right now anyway, since eventually we want to create a PDFImage subclass instead of passing a boolean in to the constructor like this.
Comment 13 Dave Hyatt 2006-10-14 03:27:12 PDT
(Which is why I think the #ifdef has to come back eventually, since everyone won't be able to make the derived class.)