Bug 180552 - [WK2] Expose image via WKBundleHitTestResult API
Summary: [WK2] Expose image via WKBundleHitTestResult API
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-07 16:02 PST by Zach Li
Modified: 2017-12-12 13:32 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.02 KB, patch)
2017-12-07 16:36 PST, Zach Li
no flags Details | Formatted Diff | Diff
Patch v2 (7.99 KB, patch)
2017-12-08 14:37 PST, Zach Li
simon.fraser: review+
a.tion.surf: commit-queue?
Details | Formatted Diff | Diff
Patch v3 (7.96 KB, patch)
2017-12-12 11:52 PST, Zach Li
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch v3 (8.00 KB, patch)
2017-12-12 12:02 PST, Zach Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zach Li 2017-12-07 16:02:17 PST
Expose image via WKBundleHitTestResult API.
Comment 1 Zach Li 2017-12-07 16:36:19 PST
Created attachment 328753 [details]
Patch
Comment 2 Wenson Hsieh 2017-12-07 17:03:21 PST
Comment on attachment 328753 [details]
Patch

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

I think you should be able to write an API test for this. I would look for other tests in Tools/TestWebKitAPI for an example.

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:180
> +    if (!is<BitmapImage>(*image))

You can remove the nullptr check above if you make this conditional !is<BitmapImage>(image) instead, since is() will return false for nullptr.

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:184
> +    IntSize intSize = bitmapImage.sizeRespectingOrientation();

I think sizeRespectingOrientation would be a clearer name for this variable.

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:189
> +    graphicsContext->drawImage(bitmapImage, floatRect);

I don't think the temp variable floatRect adds much. Can we inline this rect here as {{ }, intSize}?
Comment 3 Anders Carlsson 2017-12-08 12:40:23 PST
Comment on attachment 328753 [details]
Patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.cpp:114
> +WKImageRef WKBundleHitTestResultGetImage(WKBundleHitTestResultRef hitTestResultRef)
> +{
> +    RefPtr<WebImage> webImage = toImpl(hitTestResultRef)->image();
> +    return toAPI(webImage.leakRef());
> +}

This should be called WKBundleHitTestResultCopyImage.
Comment 4 Zach Li 2017-12-08 13:51:30 PST
Comment on attachment 328753 [details]
Patch

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

>> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.cpp:114
>> +}
> 
> This should be called WKBundleHitTestResultCopyImage.

Good point, will change.

>> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:180
>> +    if (!is<BitmapImage>(*image))
> 
> You can remove the nullptr check above if you make this conditional !is<BitmapImage>(image) instead, since is() will return false for nullptr.

I will change the conditional to !is<BitmapImage>(image).

>> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:184
>> +    IntSize intSize = bitmapImage.sizeRespectingOrientation();
> 
> I think sizeRespectingOrientation would be a clearer name for this variable.

Sure, will change.

>> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:189
>> +    graphicsContext->drawImage(bitmapImage, floatRect);
> 
> I don't think the temp variable floatRect adds much. Can we inline this rect here as {{ }, intSize}?

Sure.
Comment 5 Zach Li 2017-12-08 14:05:16 PST
(In reply to Wenson Hsieh from comment #2)
> Comment on attachment 328753 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328753&action=review
> 
> I think you should be able to write an API test for this. I would look for
> other tests in Tools/TestWebKitAPI for an example.

Per discussion with Wenson, I will add tests as a follow-up patch, which is tracked by
https://bugs.webkit.org/show_bug.cgi?id=180605.

> 
> > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:180
> > +    if (!is<BitmapImage>(*image))
> 
> You can remove the nullptr check above if you make this conditional
> !is<BitmapImage>(image) instead, since is() will return false for nullptr.
> 
> > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:184
> > +    IntSize intSize = bitmapImage.sizeRespectingOrientation();
> 
> I think sizeRespectingOrientation would be a clearer name for this variable.
> 
> > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:189
> > +    graphicsContext->drawImage(bitmapImage, floatRect);
> 
> I don't think the temp variable floatRect adds much. Can we inline this rect
> here as {{ }, intSize}?
Comment 6 Zach Li 2017-12-08 14:37:35 PST
Created attachment 328872 [details]
Patch v2
Comment 7 Simon Fraser (smfr) 2017-12-08 15:53:23 PST
Comment on attachment 328872 [details]
Patch v2

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

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:185
> +    graphicsContext->drawImage(bitmapImage, {{ }, sizeRespectingOrientation});

Does this do the correct thing for images with EXIF rotations?
Comment 8 Zach Li 2017-12-08 16:29:18 PST
(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 328872 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328872&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:185
> > +    graphicsContext->drawImage(bitmapImage, {{ }, sizeRespectingOrientation});
> 
> Does this do the correct thing for images with EXIF rotations?

I took a look at -drawImage() function and by default, DoNotRespectImageOrientation is passed to ImagePaintingOptions, so this means it would not auto-rotate images with EXIF orientation tags? (Sorry, I am not so familiar with this area of code)

But at least for my use case, image orientation does not matter.
Comment 9 Simon Fraser (smfr) 2017-12-08 16:41:22 PST
(In reply to Zach Li from comment #8)
> (In reply to Simon Fraser (smfr) from comment #7)
> > Comment on attachment 328872 [details]
> > Patch v2
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=328872&action=review
> > 
> > > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:185
> > > +    graphicsContext->drawImage(bitmapImage, {{ }, sizeRespectingOrientation});
> > 
> > Does this do the correct thing for images with EXIF rotations?
> 
> I took a look at -drawImage() function and by default,
> DoNotRespectImageOrientation is passed to ImagePaintingOptions, so this
> means it would not auto-rotate images with EXIF orientation tags? (Sorry, I
> am not so familiar with this area of code)
> 
> But at least for my use case, image orientation does not matter.

But if the image is rotated, you'll get an image with the wrong dimensions, and squished. So I think you have to care. Did you test it?
Comment 10 Zach Li 2017-12-08 16:55:25 PST
(In reply to Simon Fraser (smfr) from comment #9)
> (In reply to Zach Li from comment #8)
> > (In reply to Simon Fraser (smfr) from comment #7)
> > > Comment on attachment 328872 [details]
> > > Patch v2
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=328872&action=review
> > > 
> > > > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:185
> > > > +    graphicsContext->drawImage(bitmapImage, {{ }, sizeRespectingOrientation});
> > > 
> > > Does this do the correct thing for images with EXIF rotations?
> > 
> > I took a look at -drawImage() function and by default,
> > DoNotRespectImageOrientation is passed to ImagePaintingOptions, so this
> > means it would not auto-rotate images with EXIF orientation tags? (Sorry, I
> > am not so familiar with this area of code)
> > 
> > But at least for my use case, image orientation does not matter.
> 
> But if the image is rotated, you'll get an image with the wrong dimensions,
> and squished. So I think you have to care. Did you test it?

If the image is rotated, doesn't sizeRespectingOrientation give me the correct dimension, hence not causing the image to be squished?

I just learned images with EXIF rotations from you, so haven't tested this case yet.
Comment 11 Zach Li 2017-12-12 11:45:02 PST
(In reply to Zach Li from comment #10)
> (In reply to Simon Fraser (smfr) from comment #9)
> > (In reply to Zach Li from comment #8)
> > > (In reply to Simon Fraser (smfr) from comment #7)
> > > > Comment on attachment 328872 [details]
> > > > Patch v2
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=328872&action=review
> > > > 
> > > > > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:185
> > > > > +    graphicsContext->drawImage(bitmapImage, {{ }, sizeRespectingOrientation});
> > > > 
> > > > Does this do the correct thing for images with EXIF rotations?
> > > 
> > > I took a look at -drawImage() function and by default,
> > > DoNotRespectImageOrientation is passed to ImagePaintingOptions, so this
> > > means it would not auto-rotate images with EXIF orientation tags? (Sorry, I
> > > am not so familiar with this area of code)
> > > 
> > > But at least for my use case, image orientation does not matter.
> > 
> > But if the image is rotated, you'll get an image with the wrong dimensions,
> > and squished. So I think you have to care. Did you test it?
> 
> If the image is rotated, doesn't sizeRespectingOrientation give me the
> correct dimension, hence not causing the image to be squished?
> 
> I just learned images with EXIF rotations from you, so haven't tested this
> case yet.

Simon, you are right. We should not use -sizeRespectingOrientation since it will cause the image to be squished. We should just use -size.
Comment 12 Zach Li 2017-12-12 11:52:52 PST
Created attachment 329133 [details]
Patch v3
Comment 13 Simon Fraser (smfr) 2017-12-12 11:55:24 PST
Comment on attachment 329133 [details]
Patch v3

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

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:185
> +    BitmapImage& bitmapImage = downcast<BitmapImage>(*image);
> +    IntSize size(bitmapImage.size());
> +    auto webImage = WebImage::create(size, static_cast<ImageOptions>(0));
> +
> +    auto graphicsContext = webImage->bitmap().createGraphicsContext();
> +    graphicsContext->drawImage(bitmapImage, {{ }, size});

This needs a FIXME comment saying something like "FIXME: need to handle EXIF rotation".
Comment 14 Zach Li 2017-12-12 12:02:51 PST
Created attachment 329135 [details]
Patch v3
Comment 15 Zach Li 2017-12-12 12:03:37 PST
(In reply to Simon Fraser (smfr) from comment #13)
> Comment on attachment 329133 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329133&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:185
> > +    BitmapImage& bitmapImage = downcast<BitmapImage>(*image);
> > +    IntSize size(bitmapImage.size());
> > +    auto webImage = WebImage::create(size, static_cast<ImageOptions>(0));
> > +
> > +    auto graphicsContext = webImage->bitmap().createGraphicsContext();
> > +    graphicsContext->drawImage(bitmapImage, {{ }, size});
> 
> This needs a FIXME comment saying something like "FIXME: need to handle EXIF
> rotation".

Done in the patch.
Comment 16 WebKit Commit Bot 2017-12-12 13:01:30 PST
Comment on attachment 329135 [details]
Patch v3

Clearing flags on attachment: 329135

Committed r225798: <https://trac.webkit.org/changeset/225798>