Bug 76927

Summary:

[Chromium] Inconsistent initialization of CCSettings in WebLayerTreeView.

Product:

WebKit

Reporter:

David Reveman <reveman>

Component:

WebKit Misc.

Assignee:

David Reveman <reveman>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

jamesr, piman, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

76937

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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.