On Fri, 27 Aug 2021 09:32:24 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> modules/javafx.web/src/main/java/com/sun/javafx/webkit/prism/WCGraphicsPrismContext.java
>> line 459:
>>
>>> 457: public void setClip(WCRectangle c) {
>>> 458: if (!isOpaque) {
>>> 459: clearRect((int)c.getX(), (int)c.getY(),
>>
>> Are you sure that there are no ill effects from clearing the rectangle every
>> time a clip is set on a non-opaque context? This seems like a surprising
>> side effect.
>
> Initially, this was needed when there was some level of transparency: when
> scrolling the old content was not cleared and you could see it at its old
> position.
>
> For the full transparency case, this is still the case, but for translucent
> colors (0 < opacity < 1) I can't reproduce it anymore, so I'll modify this to
> apply only `if (isTransparent) { clearRect(); }`.
>
> I don't see a performance drop because of this.
I'm more worried about correctness than performance. Setting a clip does not
necessarily imply that the clipped region should be cleared. So this feels a
bit like a workaround for a missing `clearRect` elsewhere. I wonder if there is
code somewhere that assumes that if the fill color is fully transparent it can
skip the call to `clearRect`?
>> modules/javafx.web/src/main/java/javafx/scene/web/WebView.java line 733:
>>
>>> 731: Color color = get();
>>> 732: page.setBackgroundColor(color != null ?
>>> color.hashCode() :
>>> 733: DEFAULT_PAGE_FILL.hashCode());
>>
>> This is relying on an undocumented, implementation detail. The current
>> implementation of `Color::hashCode` happens to do what you want, but it is
>> not specified. It seems safer to create a utility method to do this.
>
> Yes, that makes total sense.
>
> In fact, there was also the need to add a new
> [method](https://github.com/openjdk/jfx/pull/563/files#diff-b80bc720bf639cde38c5197a7619561221abcd34fb9ff7a933f4b932a1f36735R2579)
> in `WebPage` to read back the color from the int value, so I was thinking
> that it would be better to add a new method to `WebPage` like:
>
>
> public void setBackgroundColor(Color backgroundColor) {
> int int32Color = WebPage.getBackgroundInt32Color(backgroundColor);
> setBackgroundColor(int32Color);
> }
>
> private static int getBackgroundInt32Color(Color color) {
> // implementation similar to Color::hashCode
> }
>
> and from webView we could simply do:
>
> page.setBackgroundColor(color);
>
>
> Thoughts?
Yes, this seems a good solution.
-------------
PR: https://git.openjdk.java.net/jfx/pull/563
