Bug 74435 – inline setting m_regionForStyling since region is rarely set

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

inline setting m_regionForStyling since region is rarely set

[Tony Chang]

Created attachment 119059 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119059 [details]
Patch

Attachment 119059 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10844442

New failing tests:
media/event-attributes.html

[Tony Chang]

Committed r102712: <http://trac.webkit.org/changeset/102712>

[Darin Adler]

What does “since region is rarely set” mean? This change is opaque to me.

[Tony Chang]

(In reply to comment #4)
> What does “since region is rarely set” mean? This change is opaque to me.

Region is NULL unless there are CSS Regions in the page.  Do you want me to rewrite the ChangeLog entry?

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > What does “since region is rarely set” mean? This change is opaque to me.
> 
> Region is NULL unless there are CSS Regions in the page.  Do you want me to rewrite the ChangeLog entry?

Probably not worth it now, but there’s just too much left unsaid in that note. A good comment would be more like:

    Inline all of initForRegionStyling except for the rarely used part for non-null regions.

[Andreas Kling]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > What does “since region is rarely set” mean? This change is opaque to me.
> > 
> > Region is NULL unless there are CSS Regions in the page.  Do you want me to rewrite the ChangeLog entry?
> 
> Probably not worth it now, but there’s just too much left unsaid in that note. A good comment would be more like:
> 
>     Inline all of initForRegionStyling except for the rarely used part for non-null regions.

/me makes a mental note to increase his standards for ChangeLog entries. Given the code change, I thought this was fairly straightforward, but that comment is indeed more clear.

[Tony Chang]

ChangeLog updated in http://trac.webkit.org/changeset/102714.

[Darin Adler]

A comment to an even higher standard would say why the inlining is good. Such as “sped things up measurably” or “I am sure this is faster because the function is called in only one place” or whatever. Anyway, no need to try to polish this more.