Bug 83060 - [chromium] When setting animation started events, should check the root layer
Summary: [chromium] When setting animation started events, should check the root layer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-03 13:11 PDT by vollick
Modified: 2012-04-04 13:31 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.51 KB, patch)
2012-04-03 18:12 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (2.53 KB, patch)
2012-04-04 11:10 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-04-03 13:11:43 PDT
Should ensure that we have a valid root layer before setting animation events.
Comment 1 vollick 2012-04-03 18:12:21 PDT
Created attachment 135475 [details]
Patch
Comment 2 Dana Jansens 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?
Comment 3 vollick 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).
Comment 4 Adrienne Walker 2012-04-04 11:24:26 PDT
Comment on attachment 135634 [details]
Patch

Looks good to me.
Comment 5 Dana Jansens 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 :)
Comment 6 WebKit Review Bot 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>
Comment 7 WebKit Review Bot 2012-04-04 13:31:44 PDT
All reviewed patches have been landed.  Closing bug.