Bug 66854

Summary:

[Qt] Path::boundingRect() is unnecessarily slow.

Product:

WebKit

Reporter:

Andreas Kling <kling>

Component:

Layout and Rendering

Assignee:

Andreas Kling <kling>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, ossy, tonikitoo, webkit.review.bot

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

66945

Bug Blocks:

Attachments:

Description	Flags
Proposed patch	none
Proposed patch	none

Andreas Kling

Reported 2011-08-24 06:43:13 PDT

This will allow us to remove the Qt-specific optimization in CanvasRenderingContext2D::stroke().

Attachments
Proposed patch (3.59 KB, patch) 2011-08-24 06:43 PDT, Andreas Kling	no flags	Details Formatted Diff Diff
Proposed patch (2.64 KB, patch) 2011-08-25 03:47 PDT, Andreas Kling	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Andreas Kling

Comment 1 2011-08-24 06:43:51 PDT

Created attachment 104989 [details] Proposed patch

Andreas Kling

Comment 2 2011-08-24 07:28:52 PDT

Comment on attachment 104989 [details] Proposed patch Clearing r? while discussing this with some random Belgian person.

Andreas Kling

Comment 3 2011-08-25 03:43:09 PDT

Looking at other ports, and clients of Path::boundingRect(), it turns out that it's not necessary to return a 100% accurate bounding rectangle, since it's only used for dirty region computation and short-circuiting the SVG path-based elements' hit testing. Changing the scope of the bug to always use QPainterPath::controlPointRect() in Qt's Path::boundingRect() since this is significantly faster and matches the current behavior of (at least) Skia's Path.

Andreas Kling

Comment 4 2011-08-25 03:47:40 PDT

Created attachment 105159 [details] Proposed patch

Benjamin Poulain

Comment 5 2011-08-25 03:49:23 PDT

Comment on attachment 105159 [details] Proposed patch Great

WebKit Review Bot

Comment 6 2011-08-25 04:48:17 PDT

Comment on attachment 105159 [details] Proposed patch Clearing flags on attachment: 105159 Committed r93774: <http://trac.webkit.org/changeset/93774>

WebKit Review Bot

Comment 7 2011-08-25 04:48:23 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 8 2011-08-25 09:02:30 PDT

Reopen, because it was rolled out by https://trac.webkit.org/changeset/93790 This patch broke zillion tests on the Qt bot. You guys should have run layout tests before landing. Please fix the bug before re-landing.

Antonio Gomes

Comment 9 2011-08-25 09:40:46 PDT

(In reply to comment #8) > Reopen, because it was rolled out by https://trac.webkit.org/changeset/93790 > > This patch broke zillion tests on the Qt bot. You guys should have run > layout tests before landing. Please fix the bug before re-landing. zillion != 26 :)

Csaba Osztrogonác

Comment 10 2011-08-25 09:42:13 PDT

(In reply to comment #9) > zillion != 26 :) You're right, but 26 !=0, and all test must pass.

Andreas Kling

Comment 11 2011-08-25 10:39:34 PDT

(In reply to comment #10) > (In reply to comment #9) > > zillion != 26 :) > > You're right, but 26 !=0, and all test must pass. Like I said on IRC, those failures are actually progressions and should just be updated. I couldn't do it at the time because I didn't have a checkout. I'll roll it back in along with a rebaseline. Thanks for playing another round of the mindless rollout game :)

Csaba Osztrogonác

Comment 12 2011-08-25 10:44:20 PDT

I don't know anything about your patch... I tried to ping you on #qtwebkit, but you didn't answer. I had to do it, because red bot can't catch new regressions ... That's why we need always green bots. That's why developers have to run layout tests before landing and not leave the tree red for hours ...

Andreas Kling

Comment 13 2011-08-26 06:36:14 PDT

Committed r93873: <http://trac.webkit.org/changeset/93873>

Note You need to log in before you can comment on or make changes to this bug.