Closed Bug 1135954 Opened 5 years ago Closed 5 years ago

Avoid computing padding and border for rbc/rtc

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Attached patch patchSplinter Review
Attachment #8568272 - Flags: review?(dholbert)
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+
Attachment #8568272 - Flags: review?(cam)
(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 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+
(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.
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.
(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.
Should be good.
Attachment #8569542 - Flags: review?(cam)
Attachment #8569542 - Flags: review?(cam) → review+
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?
Attachment #8568272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.