Bug 82501

Summary: [chromium] Add tracing events around CCLayerTreeHostImpl visibility.
Product: WebKit Reporter: Michal Mocny <mmocny>
Component: New BugsAssignee: Michal Mocny <mmocny>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, jamesr, nduca, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Michal Mocny 2012-03-28 12:08:59 PDT
[chromium] Add tracing events around CCLayerTreeHostImpl visibility.
Comment 1 Michal Mocny 2012-03-28 12:09:16 PDT
Created attachment 134363 [details]
Patch
Comment 2 Michal Mocny 2012-03-28 12:12:46 PDT
Comment on attachment 134363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134363&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:109
> +    TRACE_EVENT_START1("webkit", "CCLayerTreeHostImpl visible", this, "CCLayerTreeHostImpl", this);

This is a repeated line -- perhaps the two TRACE events should be moved into an anonymous namespace helper function that gets called instead.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:479
> +    if (m_visible) {

The braces here are against style, but are NOT optional given how the following macro is written.  Gives compile errors without them.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:480
> +      TRACE_EVENT_START1("webkit", "CCLayerTreeHostImpl visible", this, "CCLayerTreeHostImpl", this);

I added an argument to identify the tree by address.  This is because of the way tracing information is laid out, it is hard to tell when which tree is visible.
Comment 3 WebKit Review Bot 2012-03-28 12:12:54 PDT
Attachment 134363 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:481:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:483:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Michal Mocny 2012-03-28 12:19:05 PDT
Created attachment 134365 [details]
Patch
Comment 5 Michal Mocny 2012-03-28 12:19:57 PDT
Comment on attachment 134365 [details]
Patch

Using helper functions solves both the code reuse issue and style violation.
Comment 6 Nat Duca 2012-03-28 12:23:23 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=134363&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:109
>> +    TRACE_EVENT_START1("webkit", "CCLayerTreeHostImpl visible", this, "CCLayerTreeHostImpl", this);
> 
> This is a repeated line -- perhaps the two TRACE events should be moved into an anonymous namespace helper function that gets called instead.

Yeah, no clean way to do this...

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:479
>> +    if (m_visible) {
> 
> The braces here are against style, but are NOT optional given how the following macro is written.  Gives compile errors without them.

I just pinged jbates, /me hopes he can fix this. Lets workaround this by making a didVisibilityChange helper function for now...

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:480
>> +      TRACE_EVENT_START1("webkit", "CCLayerTreeHostImpl visible", this, "CCLayerTreeHostImpl", this);
> 
> I added an argument to identify the tree by address.  This is because of the way tracing information is laid out, it is hard to tell when which tree is visible.

Thats fine. Lets also say "CCLayerTreeHostImpl::Visible"
Comment 7 Michal Mocny 2012-03-28 12:30:30 PDT
Comment on attachment 134365 [details]
Patch

minor patch incoming.
Comment 8 Michal Mocny 2012-03-28 12:32:48 PDT
Created attachment 134371 [details]
Patch
Comment 9 Michal Mocny 2012-03-28 12:50:45 PDT
Created attachment 134375 [details]
Patch
Comment 10 Michal Mocny 2012-03-28 12:51:41 PDT
Comment on attachment 134375 [details]
Patch

Changed didVisibilityChange function to meet webkit style of replacing if {} else {} with if { ... return; } ...
Comment 11 WebKit Review Bot 2012-03-28 14:18:37 PDT
Comment on attachment 134375 [details]
Patch

Clearing flags on attachment: 134375

Committed r112439: <http://trac.webkit.org/changeset/112439>
Comment 12 WebKit Review Bot 2012-03-28 14:18:41 PDT
All reviewed patches have been landed.  Closing bug.