Bug 76927 - [Chromium] Inconsistent initialization of CCSettings in WebLayerTreeView.
Summary: [Chromium] Inconsistent initialization of CCSettings in WebLayerTreeView.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks: 76937
  Show dependency treegraph
 
Reported: 2012-01-24 11:03 PST by David Reveman
Modified: 2012-02-14 02:36 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.67 KB, patch)
2012-01-24 11:08 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (1.91 KB, patch)
2012-02-13 16:06 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2012-02-13 21:28 PST, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2012-01-24 11:03:39 PST
perTilePainting and partialTextureUpdates are currently not initialized properly in WebLayerTreeView::Settings::operator CCSettings().
Comment 1 David Reveman 2012-01-24 11:08:16 PST
Created attachment 123774 [details]
Patch
Comment 2 James Robinson 2012-01-24 11:24:36 PST
Comment on attachment 123774 [details]
Patch

Test?
Comment 3 David Reveman 2012-01-24 13:10:45 PST
This is not as bad as I thought. All settings are initialized properly, there's just some inconsistency about which ones gets set in WebLayerTreeView.cpp. I'd like to fix the inconsistency with this patch prior to landing: https://bugs.webkit.org/show_bug.cgi?id=76937
Comment 4 David Reveman 2012-02-13 16:06:31 PST
Created attachment 126854 [details]
Patch

Rebase
Comment 5 James Robinson 2012-02-13 20:39:05 PST
Comment on attachment 126854 [details]
Patch

Wait, CCSettings already has a constructor that sets all of these values to their defaults.  Why do we bother manually resetting these to their defaults again in WLTV?
Comment 6 David Reveman 2012-02-13 21:13:12 PST
(In reply to comment #5)
> (From update of attachment 126854 [details])
> Wait, CCSettings already has a constructor that sets all of these values to their defaults.  Why do we bother manually resetting these to their defaults again in WLTV?

I don't know. Maybe to be explicit about what settings are not yet exposed here? This patch just makes sure we're consistent. I'm happy to have it just remove all this unnecessary initialization instead. Would you prefer that?
Comment 7 David Reveman 2012-02-13 21:28:46 PST
Created attachment 126903 [details]
Patch

Instead remove unnecessary initialization
Comment 8 James Robinson 2012-02-13 21:37:57 PST
Yeah, I think so - it makes more sense to me if WLTV only has to know about the values that it cares about and can leave everything else alone.
Comment 9 James Robinson 2012-02-13 21:38:45 PST
Comment on attachment 126903 [details]
Patch

I prefer this version.  I'm somewhat curious if Antoine has any specific opinion.  I definitely don't want to have to update two files every time we add a new setting to CCSettings unless we want to also expose it via the WLTV API.
Comment 10 Antoine Labour 2012-02-13 22:09:01 PST
Just FYI, I think showFPSCounter & showPlatformLayerTree have been accidentally disabled with a merge gone wrong, they used to work (although triggering a harmless assert in debug). I just haven't gone back and check that they actually work...
In general, we don't need to expose anything in WLTV::Settings unless we want to use it, and we can rely on the CCSettings constructor to properly initialize things.
Comment 11 WebKit Review Bot 2012-02-14 02:36:14 PST
Comment on attachment 126903 [details]
Patch

Clearing flags on attachment: 126903

Committed r107695: <http://trac.webkit.org/changeset/107695>
Comment 12 WebKit Review Bot 2012-02-14 02:36:21 PST
All reviewed patches have been landed.  Closing bug.