| Summary: | [chromium] When setting animation started events, should check the root layer | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | vollick | ||||||
| Component: | WebKit Misc. | Assignee: | vollick | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cc-bugs, danakj, enne, jamesr, nduca, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
vollick
2012-04-03 13:11:43 PDT
Created attachment 135475 [details]
Patch
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? 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). Comment on attachment 135634 [details]
Patch
Looks good to me.
(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 :) Comment on attachment 135634 [details] Patch Clearing flags on attachment: 135634 Committed r113232: <http://trac.webkit.org/changeset/113232> All reviewed patches have been landed. Closing bug. |