Bug 76927 – [Chromium] Inconsistent initialization of CCSettings in WebLayerTreeView.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[David Reveman]

perTilePainting and partialTextureUpdates are currently not initialized properly in WebLayerTreeView::Settings::operator CCSettings().

[David Reveman]

Created attachment 123774 [details]
Patch

[James Robinson]

Comment on attachment 123774 [details]
Patch

Test?

[David Reveman]

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

[David Reveman]

Created attachment 126854 [details]
Patch

Rebase

[James Robinson]

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?

[David Reveman]

(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?

[David Reveman]

Created attachment 126903 [details]
Patch

Instead remove unnecessary initialization

[James Robinson]

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.

[James Robinson]

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.

[Antoine Labour]

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.

[WebKit Review Bot]

Comment on attachment 126903 [details]
Patch

Clearing flags on attachment: 126903

Committed r107695: <http://trac.webkit.org/changeset/107695>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.