Bug 74435

Summary: inline setting m_regionForStyling since region is rarely set
Product: WebKit Reporter: Tony Chang <tony>
Component: New BugsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, kling, macpherson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 74141    
Attachments:
Description Flags
Patch kling: review+, webkit.review.bot: commit-queue-

Description Tony Chang 2011-12-13 12:05:32 PST 
inline setting m_regionForStyling since region is rarely set
Comment 1 Tony Chang 2011-12-13 12:06:08 PST 
Created attachment 119059 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-13 13:51:54 PST 
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
Comment 3 Tony Chang 2011-12-13 15:38:24 PST 
Committed r102712: <http://trac.webkit.org/changeset/102712>
Comment 4 Darin Adler 2011-12-13 15:40:46 PST 
What does “since region is rarely set” mean? This change is opaque to me.
Comment 5 Tony Chang 2011-12-13 15:42:22 PST 
(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?
Comment 6 Darin Adler 2011-12-13 15:46:17 PST 
(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.
Comment 7 Andreas Kling 2011-12-13 15:50:11 PST 
(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.
Comment 8 Tony Chang 2011-12-13 15:51:31 PST 
ChangeLog updated in http://trac.webkit.org/changeset/102714.
Comment 9 Darin Adler 2011-12-13 15:53:25 PST 
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.