Bug 11257 – Remove mac platform dependance from Loader::ReceivedResponse() and CachedImage::createImage()

• Component: Platform
• Status: RESOLVED FIXED

[David Carson]

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

[David Carson]

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.

[David Carson]

CachedImage also does not need to be special cased. By passing PlatformRequest to ResponseMIMEType(), platforms can make their own implementation.

[David Carson]

Created attachment 11038 [details]
Updated patch

Created new patch to include changes for ResponseMIMEType()

[Maciej Stachowiak]

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.

[Dave Hyatt]

The PDF #ifdef needs to be kept.  It should be changed to PLATFORM(CG) though.

[David Carson]

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

[David Carson]

(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

[David Carson]

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

[David Carson]

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(); }

[Maciej Stachowiak]

Comment on attachment 11047 [details]
missed some non-mac build items

r=me

[Mark Rowe (bdash)]

Landed in r17056.

[Dave Hyatt]

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.

[Dave Hyatt]

(Which is why I think the #ifdef has to come back eventually, since everyone won't be able to make the derived class.)