Bug 70766 - LayerRendererChromium::setVisible called needlessly without a visibility change
Summary: LayerRendererChromium::setVisible called needlessly without a visibility change
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: 2011-10-24 14:47 PDT by Michal Mocny
Modified: 2011-10-25 13:58 PDT (History)
4 users (show)

See Also:

Attachments
Patch (2.43 KB, patch)
2011-10-24 14:50 PDT, Michal Mocny 		no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2011-10-25 08:27 PDT, Michal Mocny 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Mocny 2011-10-24 14:47:48 PDT 
LayerRendererChromium::setVisible called needlessly without a visibility change
Comment 1 Michal Mocny 2011-10-24 14:50:00 PDT 
Created attachment 112253 [details]
Patch
Comment 2 Michal Mocny 2011-10-24 14:54:58 PDT 
This fixes the issues with setVisibilityCHROMIUM(true) being spammed after the recently plumbing changes (https://bugs.webkit.org/show_bug.cgi?id=70635).

There are other options to prevent this, but talking with Nat it seems that there is no need for LayerRendererChromium::setVisible to be called unless visibility actually changed.
Comment 3 Nat Duca 2011-10-24 14:58:03 PDT 
Comment on attachment 112253 [details]
Patch

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

Seems ok modulo the comment above.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:51
> +    , m_visible(false)

True or false here? I was expecting true personally. Do GL contexts start up "invisible"?
Comment 4 Kenneth Russell 2011-10-24 15:19:40 PDT 
Comment on attachment 112253 [details]
Patch

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

Nat's question is a good one. Small issue in the ChangeLog needs to be fixed as well.

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

This second OOPS will have to be removed in order for the commit queue to process this patch.
Comment 5 Michal Mocny 2011-10-25 08:27:14 PDT 
Created attachment 112339 [details]
Patch
Comment 6 Kenneth Russell 2011-10-25 10:45:47 PDT 
Comment on attachment 112339 [details]
Patch

Looks fine. r=me
Comment 7 WebKit Review Bot 2011-10-25 13:58:23 PDT 
Comment on attachment 112339 [details]
Patch

Clearing flags on attachment: 112339

Committed r98392: <http://trac.webkit.org/changeset/98392>
Comment 8 WebKit Review Bot 2011-10-25 13:58:28 PDT 
All reviewed patches have been landed.  Closing bug.