Bug 82501 - [chromium] Add tracing events around CCLayerTreeHostImpl visibility.
Summary: [chromium] Add tracing events around CCLayerTreeHostImpl visibility.
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: Michal Mocny
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-28 12:08 PDT by Michal Mocny
Modified: 2012-03-28 14:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.01 KB, patch)
2012-03-28 12:09 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (2.35 KB, patch)
2012-03-28 12:19 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (2.31 KB, patch)
2012-03-28 12:32 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (2.23 KB, patch)
2012-03-28 12:50 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff

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