RESOLVED FIXED 66854
[Qt] Path::boundingRect() is unnecessarily slow. 
https://bugs.webkit.org/show_bug.cgi?id=66854
Summary [Qt] Path::boundingRect() is unnecessarily slow.
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
DetailsFormatted DiffDiff
Proposed patch (2.64 KB, patch)
2011-08-25 03:47 PDT, Andreas Kling 		no flags
DetailsFormatted DiffDiff
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.