RESOLVED FIXED 83060
[chromium] When setting animation started events, should check the root layer
https://bugs.webkit.org/show_bug.cgi?id=83060
Summary [chromium] When setting animation started events, should check the root layer
vollick
Reported 2012-04-03 13:11:43 PDT
Should ensure that we have a valid root layer before setting animation events.
Attachments
Patch (1.51 KB, patch)
2012-04-03 18:12 PDT, vollick
no flags
Patch (2.53 KB, patch)
2012-04-04 11:10 PDT, vollick
no flags
vollick
Comment 1 2012-04-03 18:12:21 PDT
Dana Jansens
Comment 2 2012-04-03 19:00:13 PDT
I feel like we're starting to have if (rootLayer) checks all over the LayerTreeHosts. Is there one central place in the proxy or somewhere that we can do this check?
vollick
Comment 3 2012-04-04 11:10:43 PDT
Created attachment 135634 [details] Patch (In reply to comment #2) > I feel like we're starting to have if (rootLayer) checks all over the LayerTreeHosts. Is there one central place in the proxy or somewhere that we can do this check? There aren't _that_ many checks ;) With this change, I think the count would be up to four (two of which are in setRootLayer). I've moved the two extra checks into the recursive functions, which is perhaps a better place for them. I really think that the layer tree hosts are the right folks to be doing these nullity checks, though. I think that bumping the checks up to the proxies is the wrong move. As I understand things, the proxies operate on layer tree hosts and the layer tree hosts operate on layers. IMO, if the proxy has logic to detect when the root layer is null and avoid calling certain functions in that case, then the separation of concerns of the proxies and hosts has been muddied. I think a proxy should be able to happily tell the hosts to do things and it should be up to the hosts to determine whether or not it can actually do them (possibly returning error codes, if appropriate).
Adrienne Walker
Comment 4 2012-04-04 11:24:26 PDT
Comment on attachment 135634 [details] Patch Looks good to me.
Dana Jansens
Comment 5 2012-04-04 11:32:30 PDT
(In reply to comment #3) > Created an attachment (id=135634) [details] > Patch > > (In reply to comment #2) > > I feel like we're starting to have if (rootLayer) checks all over the LayerTreeHosts. Is there one central place in the proxy or somewhere that we can do this check? > > There aren't _that_ many checks ;) With this change, I think the count would be up to four (two of which are in setRootLayer). I've moved the two extra checks into the recursive functions, which is perhaps a better place for them. > > I really think that the layer tree hosts are the right folks to be doing these nullity checks, though. I think that bumping the checks up to the proxies is the wrong move. As I understand things, the proxies operate on layer tree hosts and the layer tree hosts operate on layers. IMO, if the proxy has logic to detect when the root layer is null and avoid calling certain functions in that case, then the separation of concerns of the proxies and hosts has been muddied. I think a proxy should be able to happily tell the hosts to do things and it should be up to the hosts to determine whether or not it can actually do them (possibly returning error codes, if appropriate). Makes sense, yeh :)
WebKit Review Bot
Comment 6 2012-04-04 13:31:40 PDT
Comment on attachment 135634 [details] Patch Clearing flags on attachment: 135634 Committed r113232: <http://trac.webkit.org/changeset/113232>
WebKit Review Bot
Comment 7 2012-04-04 13:31:44 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.