Bug 180552 – [WK2] Expose image via WKBundleHitTestResult API

• Status: NEW
• Component: WebKit2

[Zach Li]

Expose image via WKBundleHitTestResult API.

[Zach Li]

Created attachment 328753 [details]
Patch

[Wenson Hsieh]

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}?

[Anders Carlsson]

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.

[Zach Li]

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.

[Zach Li]

(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}?

[Zach Li]

Created attachment 328872 [details]
Patch v2

[Simon Fraser (smfr)]

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?

[Zach Li]

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

[Simon Fraser (smfr)]

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

[Zach Li]

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

[Zach Li]

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

[Zach Li]

Created attachment 329133 [details]
Patch v3

[Simon Fraser (smfr)]

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

[Zach Li]

Created attachment 329135 [details]
Patch v3

[Zach Li]

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

[WebKit Commit Bot]

Comment on attachment 329135 [details]
Patch v3

Clearing flags on attachment: 329135

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