Bug 69545 – Implement testRunner.dumpSelectionRect() in WebKitTestRunner

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

layoutTestController.dumpSelectionRect() paints a red rectangle in the pixel result. This is not supported in WTR.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 168695 [details]
Patch

[Build Bot]

Comment on attachment 168695 [details]
Patch

Attachment 168695 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14289635

[Raphael Kubo da Costa (:rakuco)]

Created attachment 168700 [details]
Hopefully fix the mac build

[Build Bot]

Comment on attachment 168700 [details]
Hopefully fix the mac build

Attachment 168700 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14291675

[Raphael Kubo da Costa (:rakuco)]

Created attachment 168706 [details]
Patch

[Simon Fraser (smfr)]

I think it would be better to do something more similar to what I did for repaint rects. You could add a method on Internals:

ClientRect internals.clientSelectionRect()

so that non-pixel tests can test the selection rect. Then you can fix WTR/DRT to paint this as an overlay, like repaints rects, if the testRunner flag is set.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #6)
> I think it would be better to do something more similar to what I did for repaint rects. You could add a method on Internals:
>
> ClientRect internals.clientSelectionRect()
>
> so that non-pixel tests can test the selection rect. Then you can fix WTR/DRT to paint this as an overlay, like repaints rects, if the testRunner flag is set.

We talked a bit on IRC and I am still unsure this is the best approach. For one, we have agreed that getting rid of dumpSelectionRect() in its current form so that non-pixel tests can use it is a reasonable goal for the longer term.

As for the current patch, there are a few possibilities, but all of them involve either writing more code or writing code that is port-specific:

 o Painting the overlay in the way the repaint rects overlay is painted. This means not using WebPageOverlay, but instead just writing port-specific code to do the painting in WTR.

 o Making paintRepaintRectOverlay more generic and manipulating the GraphicsContext from WTR. This would at least require exposing more API in WKGraphicsContext.

 o Use actual WebPageOverlays. If done from WTR, it would require exposing more overlay API so that we turn off fading animations and set things such as border color and thickness. If done from WebProcess, it would involve passing the selection and repaint rectangles and making the code differentiate between one and the other so that they are painted in different ways.

Is there a preference for one of these methods, or a better idea that wouldn't require a lot more code to achieve the same results?

[Simon Fraser (smfr)]

Comment on attachment 168706 [details]
Patch

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

> Source/WebCore/WebCore.exp.in:853
> +__ZN7WebCore7makeRGBEiii

I don't see this being used; do you need it?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #8)
> (From update of attachment 168706 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168706&action=review
> 
> > Source/WebCore/WebCore.exp.in:853
> > +__ZN7WebCore7makeRGBEiii
> 
> I don't see this being used; do you need it?

According to the logs in comment 2 and comment 4, yes.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 168706 [details]
Patch

Clearing flags on attachment: 168706

Committed r131476: <http://trac.webkit.org/changeset/131476>

[Raphael Kubo da Costa (:rakuco)]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> We talked a bit on IRC and I am still unsure this is the best approach. For one, we have agreed that getting rid of dumpSelectionRect() in its current form so that non-pixel tests can use it is a reasonable goal for the longer term.

File <https://bugs.webkit.org/show_bug.cgi?id=99480>.