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
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.
CachedImage also does not need to be special cased. By passing PlatformRequest to ResponseMIMEType(), platforms can make their own implementation.
Created attachment 11038 [details] Updated patch Created new patch to include changes for ResponseMIMEType()
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.
The PDF #ifdef needs to be kept. It should be changed to PLATFORM(CG) though.
(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. >
(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
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
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 on attachment 11047 [details] missed some non-mac build items r=me
Landed in r17056.
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.
(Which is why I think the #ifdef has to come back eventually, since everyone won't be able to make the derived class.)