Bug 135671 – [iOS] Make document marker assets not specific to particular scale factors

• Component: Text
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[iOS] Make document marker assets not specific to particular scale factors

[Radar WebKit Bug Importer]

<rdar://problem/17936546>

[Radar WebKit Bug Importer]

<rdar://problem/17936564>

[Jon Lee]

<rdar://problem/17855327>

[Myles C. Maxfield]

Created attachment 236138 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 236138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236138&action=review

> Source/WebCore/platform/ios/wak/WKGraphics.mm:96
> +    NSString *fileName = scaleFactor == 1 ? [NSString stringWithUTF8String:imageFile] : [NSString stringWithFormat:@"%s@%dx", imageFile, scaleFactor];

You've removed the fallback that used to be here. I think it would be better to reinstate the fallback, but assert when the correct image is not found.

[Myles C. Maxfield]

Created attachment 236139 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 236139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236139&action=review

> Source/WebCore/platform/ios/wak/WKGraphics.mm:116
> +    NSData *imageData = nullptr;
> +    for (unsigned scaleFactor = wkGetScreenScaleFactor(); scaleFactor > 0; --scaleFactor) {
> +        imageData = [NSData dataWithContentsOfFile:imageResourcePath(image_file, scaleFactor)];
> +        ASSERT(scaleFactor != wkGetScreenScaleFactor() || imageData);

Now you're just fetching the 1x image.

[Myles C. Maxfield]

Created attachment 236143 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 236143 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236143&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests, because on iOS I can only test whether or not document
> +        markers should be rendered, rather than whether or not they were rendered

I don't think you need to justify the lack of tests.

> Source/WebCore/platform/ios/wak/WKGraphics.mm:116
> +        ASSERT(scaleFactor != wkGetScreenScaleFactor() || imageData);

I think an if (imageData) break; would be a little easier to read.

[Myles C. Maxfield]

Comment on attachment 236143 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236143&action=review

>> Source/WebCore/ChangeLog:9
>> +        markers should be rendered, rather than whether or not they were rendered
> 
> I don't think you need to justify the lack of tests.

Done.

>> Source/WebCore/platform/ios/wak/WKGraphics.mm:116
>> +        ASSERT(scaleFactor != wkGetScreenScaleFactor() || imageData);
> 
> I think an if (imageData) break; would be a little easier to read.

Done.

[Myles C. Maxfield]

https://trac.webkit.org/r172197