|Summary:||[Chromium] Inconsistent initialization of CCSettings in WebLayerTreeView.|
|Product:||WebKit||Reporter:||David Reveman <reveman>|
|Component:||WebKit Misc.||Assignee:||David Reveman <reveman>|
|Severity:||Normal||CC:||jamesr, piman, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description David Reveman 2012-01-24 11:03:39 PST
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.