Closed
Bug 1135954
Opened 9 years ago
Closed 9 years ago
Avoid computing padding and border for rbc/rtc
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: xidorn, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
4.47 KB,
patch
|
heycam
:
review+
dholbert
:
feedback+
abillings
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.59 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
Although we decided to suppress border and padding on rbc/rtc in bug 1055667, but we currently still compute and check their current value, which is not necessary and wastes computation. It could sometimes even trigger unnecessary network request (e.g. for border-image). We should be able to simply give every rbc/rtc an empty struct.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8568272 -
Flags: review?(dholbert)
Comment 2•9 years ago
|
||
Comment on attachment 8568272 [details] [diff] [review] patch I should defer to a style-system peer on this, I think (heycam or bz or dbaron). (Of the three, heycam may be most-available for reviews right now, I think; though dbaron has reviewed more ruby stuff IIRC, so you could also wait for him to be back from travel, if this can wait a week.) Two nits though: >@@ -433,34 +433,64 @@ nsStyleContext::GetUniqueStyleData(const > #define UNIQUE_CASE(c_) \ > case eStyleStruct_##c_: \ > result = new (presContext) nsStyle##c_( \ > * static_cast<const nsStyle##c_ *>(current)); \ > break; > > UNIQUE_CASE(Display) > UNIQUE_CASE(Background) >- UNIQUE_CASE(Border) >- UNIQUE_CASE(Padding) > UNIQUE_CASE(Text) This chunk (removing these 2 lines from GetUniqueStyleData) seems like it would affect stuff beyond rbc/rtc... >+// This is an evil function, but less evil than GetUniqueStyleData. It >+// creates an empty style struct and sets it to the style context. The second line here needs some wording-tweaks. It sounds like it's saying: myNewEmptyStyleStruct = theStyleContext; which isn't what you mean here. Maybe replace with "creates an empty style struct for this nsStyleContext" (?)
Attachment #8568272 -
Flags: review?(dholbert) → feedback+
Assignee | ||
Updated•9 years ago
|
Attachment #8568272 -
Flags: review?(cam)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #2) > Comment on attachment 8568272 [details] [diff] [review] > patch > > I should defer to a style-system peer on this, I think (heycam or bz or > dbaron). (Of the three, heycam may be most-available for reviews right now, > I think; though dbaron has reviewed more ruby stuff IIRC, so you could also > wait for him to be back from travel, if this can wait a week.) > > Two nits though: > >@@ -433,34 +433,64 @@ nsStyleContext::GetUniqueStyleData(const > > #define UNIQUE_CASE(c_) \ > > case eStyleStruct_##c_: \ > > result = new (presContext) nsStyle##c_( \ > > * static_cast<const nsStyle##c_ *>(current)); \ > > break; > > > > UNIQUE_CASE(Display) > > UNIQUE_CASE(Background) > >- UNIQUE_CASE(Border) > >- UNIQUE_CASE(Padding) > > UNIQUE_CASE(Text) > > This chunk (removing these 2 lines from GetUniqueStyleData) seems like it > would affect stuff beyond rbc/rtc... No, they won't. The two lines were added for rbc/rtc. As shown from DXR result, they are not used in other places. In addition, it seems that only Display and Text are still in use now, which means we probably can remove other two as well.
Comment 4•9 years ago
|
||
Comment on attachment 8568272 [details] [diff] [review] patch Review of attachment 8568272 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleContext.cpp @@ +482,5 @@ > + return nullptr; > + } > + > + SetStyle(aSID, result); > + return result; Can you add a comment in here saying that the new style struct is owned by this style context, but that we don't need to clear the bit in mBits because we've asserted that at the top of the function?
Attachment #8568272 -
Flags: review?(cam) → review+
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #4) > Comment on attachment 8568272 [details] [diff] [review] > patch > > Review of attachment 8568272 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/style/nsStyleContext.cpp > @@ +482,5 @@ > > + return nullptr; > > + } > > + > > + SetStyle(aSID, result); > > + return result; > > Can you add a comment in here saying that the new style struct is owned by > this style context, but that we don't need to clear the bit in mBits because > we've asserted that at the top of the function? OK. BTW, I wonder can we make them share a same rule node so that we don't need to create one for each style context of rbc/rtc.
Comment 6•9 years ago
|
||
Can you also make this new function private? No need to expose it. Can you make GetUniqueStyleData private as well, in a follow-up? (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #3) > No, they won't. The two lines were added for rbc/rtc. As shown from DXR > result, they are not used in other places. In addition, it seems that only > Display and Text are still in use now, which means we probably can remove > other two as well. Please file a follow-up for this too.
Comment 7•9 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #5) > OK. BTW, I wonder can we make them share a same rule node so that we don't > need to create one for each style context of rbc/rtc. I don't think that's possible, since in general you can style different rbc/rtc elements differently.
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2233dae7bd58
Updated•9 years ago
|
Attachment #8569542 -
Flags: review?(cam) → review+
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fb7a83119e https://hg.mozilla.org/integration/mozilla-inbound/rev/67e70c0c2aaf
https://hg.mozilla.org/mozilla-central/rev/b7fb7a83119e https://hg.mozilla.org/mozilla-central/rev/67e70c0c2aaf
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8568272 [details] [diff] [review] patch Approval Request Comment [Feature/regressing bug #]: bug 1055667 [User impact if declined]: Useful optimization for Ruby, which we've enabled by default as far back as Aurora; hence, it'd be nice to uplift this to Aurora, particularly this early in the Aurora time-table. [Describe test coverage new/current, TreeHerder]: No change to rendering tests for bug 1055667 [Risks and why]: No risk given css ruby is just enabled several days ago, we won't break things we supported before by this patch. [String/UUID change made/needed]:n/a
Attachment #8568272 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8568272 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
status-firefox38:
--- → affected
tracking-firefox38:
--- → +
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/838f4982e927 https://hg.mozilla.org/releases/mozilla-aurora/rev/2bae960bfe47
You need to log in
before you can comment on or make changes to this bug.
Description
•