Bug 90039 - Avoid calling GraphicsContext drawing primitives for 0px borders
Summary: Avoid calling GraphicsContext drawing primitives for 0px borders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-26 19:25 PDT by Julien Chaffraix
Modified: 2012-07-18 18:53 PDT (History)
4 users (show)

See Also:


Attachments
WIP patch. Needs more thoughts. (5.75 KB, patch)
2012-06-26 19:26 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Better fix 1: Added more checks and ASSERTs. (13.06 KB, patch)
2012-07-12 18:41 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (476.28 KB, application/zip)
2012-07-12 21:00 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-06-26 19:25:00 PDT
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.
Comment 1 Julien Chaffraix 2012-06-26 19:26:24 PDT
Created attachment 149667 [details]
WIP patch. Needs more thoughts.
Comment 2 Julien Chaffraix 2012-07-12 18:41:30 PDT
Created attachment 152126 [details]
Better fix 1: Added more checks and ASSERTs.
Comment 3 WebKit Review Bot 2012-07-12 21:00:50 PDT
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
Comment 4 WebKit Review Bot 2012-07-12 21:00:53 PDT
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
Comment 5 Eric Seidel (no email) 2012-07-18 16:05:52 PDT
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?
Comment 6 Julien Chaffraix 2012-07-18 16:30:06 PDT
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.
Comment 7 Julien Chaffraix 2012-07-18 17:23:52 PDT
Comment on attachment 152126 [details]
Better fix 1: Added more checks and ASSERTs.

Thanks for the review Eric!
Comment 8 WebKit Review Bot 2012-07-18 18:52:57 PDT
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>
Comment 9 WebKit Review Bot 2012-07-18 18:53:02 PDT
All reviewed patches have been landed.  Closing bug.