Bug 74435 - inline setting m_regionForStyling since region is rarely set
Summary: inline setting m_regionForStyling since region is rarely set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 74141
  Show dependency treegraph
 
Reported: 2011-12-13 12:05 PST by Tony Chang
Modified: 2011-12-13 15:53 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.36 KB, patch)
2011-12-13 12:06 PST, Tony Chang
kling: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.