Bug 118032

Summary:

Don't force layout when querying a fixed or non-box margin/padding property

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

CSS

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, commit-queue, eric, esprehn+autocc, glenn, hyatt, kling, koivisto, macpherson, menard, mjs, simon.fraser, tonyg

Priority:

Keywords:

BlinkMergeCandidate

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

118930, 118936

Bug Blocks:

Attachments:

Description	Flags
Merges the patch	none

Ryosuke Niwa

Reported 2013-06-25 23:07:48 PDT

Merge https://chromium.googlesource.com/chromium/blink/+/66427d0825fcc2975bd50220cdcaa2504d6f36e5 The case which calculates the margin only depends on the RenderStyle when the margin is fixed. It is non-fixed margins that depend on the RenderBox's margin. The case that calculates the padding only depends on the RenderStyle when the element is not a box. This improves the page load time of the economist by 27% (4.7s -> 3.4s) on my z620 workstation.

Attachments
Merges the patch (10.78 KB, patch) 2013-07-24 22:36 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2013-06-25 23:49:39 PDT

Don't merge verbatim. isLayoutDependent should not take a PassRefPtr<RenderStyle> style.

Antti Koivisto

Comment 2 2013-06-26 13:47:22 PDT

Also there should be an inline helper function for all these renderer && renderer->isBox() && (!style || !style->paddingBottom().isFixed());

Ryosuke Niwa

Comment 3 2013-07-15 14:53:30 PDT

Also see https://chromium.googlesource.com/chromium/blink/+/ff234b1593b2b493d47f38f687d09a87bc42c9eb. Also see the bug 118618.

Ryosuke Niwa

Comment 4 2013-07-24 22:36:57 PDT

Created attachment 207433 [details] Merges the patch

Tony Gentilcore

Comment 5 2013-07-25 08:03:58 PDT

There were two follow-ups you might be interested in: https://codereview.chromium.org/18298016/ https://codereview.chromium.org/19272007/

Ryosuke Niwa

Comment 6 2013-07-25 11:53:15 PDT

(In reply to comment #5) > There were two follow-ups you might be interested in: > https://codereview.chromium.org/18298016/ > https://codereview.chromium.org/19272007/ Thanks but those are follow ups for the patch to be merged in the bug 118618, right?

Dave Hyatt

Comment 7 2013-07-25 13:49:12 PDT

Comment on attachment 207433 [details] Merges the patch r=me

Tony Gentilcore

Comment 8 2013-07-25 13:54:26 PDT

(In reply to comment #6) > (In reply to comment #5) > > There were two follow-ups you might be interested in: > > https://codereview.chromium.org/18298016/ > > https://codereview.chromium.org/19272007/ > > Thanks but those are follow ups for the patch to be merged in the bug 118618, right? You are right. Sorry, I got the bugs confused.

Ryosuke Niwa

Comment 9 2013-07-25 14:39:34 PDT

Comment on attachment 207433 [details] Merges the patch Clearing flags on attachment: 207433 Committed r153347: <http://trac.webkit.org/changeset/153347>

Ryosuke Niwa

Comment 10 2013-07-25 14:39:39 PDT

All reviewed patches have been landed. Closing bug.

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