Bug 90039 – Avoid calling GraphicsContext drawing primitives for 0px borders

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

There is no guarantee in our code that we wouldn't call GraphicsContext for a 0px border. Apart from being an unneeded overhead, it looks like we can end up painting the border on Chromium: the only repro I have involves painting to a PDF where Skia re-interprets the 0px border as a thin line.

[Julien Chaffraix]

Created attachment 149667 [details]
WIP patch. Needs more thoughts.

[Julien Chaffraix]

Created attachment 152126 [details]
Better fix 1: Added more checks and ASSERTs.

[WebKit Review Bot]

Comment on attachment 152126 [details]
Better fix 1: Added more checks and ASSERTs.

Attachment 152126 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13207907

New failing tests:
http/tests/w3c/webperf/approved/navigation-timing/html/test_performance_attributes_exist_in_object.html

[WebKit Review Bot]

Created attachment 152146 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Eric Seidel (no email)]

Comment on attachment 152126 [details]
Better fix 1: Added more checks and ASSERTs.

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

Seems reasonable.  But I worry we're just papering over issues here.

> Source/WebCore/ChangeLog:12
> +        0px borders, regardless of the border-style, will not be painted. Thus this is a waste
> +        of time on all platforms. On Chromium, this trigger some issues with pdf rendering as
> +        Skia interprets 0px lines as very thin lines.

I'm not saying we shouldn't do this change... but it seems like this differnece in skia is a more general problem, which needs to be fixed.  GraphicsContextSkia should just discard any 0px lines if that matches other platforms?

[Julien Chaffraix]

Comment on attachment 152126 [details]
Better fix 1: Added more checks and ASSERTs.

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

>> Source/WebCore/ChangeLog:12
>> +        Skia interprets 0px lines as very thin lines.
> 
> I'm not saying we shouldn't do this change... but it seems like this differnece in skia is a more general problem, which needs to be fixed.  GraphicsContextSkia should just discard any 0px lines if that matches other platforms?

I had a similar position on that (even thinking we should fix Skia but that's not something we can change AFAICT). This change is good outside Skia as it doesn't make sense to call the platform with a 0px line but as you pointed out it doesn't solve the underlying issue. I don't have evidence that it's very widespread (not to discard that it may be) so if we start whack-a-molling, I agree with you that we should filter at the GraphicsContextSkia level.

[Julien Chaffraix]

Comment on attachment 152126 [details]
Better fix 1: Added more checks and ASSERTs.

Thanks for the review Eric!

[WebKit Review Bot]

Comment on attachment 152126 [details]
Better fix 1: Added more checks and ASSERTs.

Clearing flags on attachment: 152126

Committed r123061: <http://trac.webkit.org/changeset/123061>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.