Bug 118032 - Don't force layout when querying a fixed or non-box margin/padding property
Summary: Don't force layout when querying a fixed or non-box margin/padding property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: BlinkMergeCandidate
Depends on: 118930 118936
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-25 23:07 PDT by Ryosuke Niwa
Modified: 2013-07-25 14:39 PDT (History)
13 users (show)

See Also:


Attachments
Merges the patch (10.78 KB, patch)
2013-07-24 22:36 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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.
Comment 1 Simon Fraser (smfr) 2013-06-25 23:49:39 PDT
Don't merge verbatim. isLayoutDependent should not take a PassRefPtr<RenderStyle> style.
Comment 2 Antti Koivisto 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());
Comment 4 Ryosuke Niwa 2013-07-24 22:36:57 PDT
Created attachment 207433 [details]
Merges the patch
Comment 5 Tony Gentilcore 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/
Comment 6 Ryosuke Niwa 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?
Comment 7 Dave Hyatt 2013-07-25 13:49:12 PDT
Comment on attachment 207433 [details]
Merges the patch

r=me
Comment 8 Tony Gentilcore 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.
Comment 9 Ryosuke Niwa 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>
Comment 10 Ryosuke Niwa 2013-07-25 14:39:39 PDT
All reviewed patches have been landed.  Closing bug.