83060 2012-04-03 13:11:43 -0700 [chromium] When setting animation started events, should check the root layer 2012-04-04 13:31:44 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 vollick vollick cc-bugs danakj enne jamesr nduca webkit.review.bot oldest_to_newest 594789 0 vollick 2012-04-03 13:11:43 -0700 Should ensure that we have a valid root layer before setting animation events. 595091 1 135475 vollick 2012-04-03 18:12:21 -0700 Created attachment 135475 Patch 595131 2 danakj 2012-04-03 19:00:13 -0700 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? 595724 3 135634 vollick 2012-04-04 11:10:43 -0700 Created attachment 135634 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). 595741 4 135634 enne 2012-04-04 11:24:26 -0700 Comment on attachment 135634 Patch Looks good to me. 595748 5 danakj 2012-04-04 11:32:30 -0700 (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 :) 595864 6 135634 webkit.review.bot 2012-04-04 13:31:40 -0700 Comment on attachment 135634 Patch Clearing flags on attachment: 135634 Committed r113232: <http://trac.webkit.org/changeset/113232> 595866 7 webkit.review.bot 2012-04-04 13:31:44 -0700 All reviewed patches have been landed. Closing bug. 135475 2012-04-03 18:12:21 -0700 2012-04-04 11:10:40 -0700 Patch bug-83060-20120403211220.patch text/plain 1545 vollick U3VidmVyc2lvbiBSZXZpc2lvbjogMTEzMTEyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZGQwNzUwMDk3ZjgwMmYz MDExZjVkNTlmNWVhNmZjYjI0ZGZhZjEyMC4uMWE0OTMzNDUwZTQ4YTdkYjVhNDhkZGE5MmQxMDc0 NTQ2YjdmNTEyZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEyLTA0LTAzICBJYW4g Vm9sbGljayAgPHZvbGxpY2tAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtjaHJvbWl1bV0gV2hl biBzZXR0aW5nIGFuaW1hdGlvbiBzdGFydGVkIGV2ZW50cywgc2hvdWxkIGNoZWNrIHRoZSByb290 IGxheWVyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04 MzA2MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5v IG5ldyB0ZXN0cy4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0ND TGF5ZXJUcmVlSG9zdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDQ0xheWVyVHJlZUhvc3Q6OnNl dEFuaW1hdGlvbkV2ZW50cyk6CisKIDIwMTItMDQtMDMgIFJhZmFlbCBXZWluc3RlaW4gIDxyYWZh ZWx3QGNocm9taXVtLm9yZz4KIAogICAgICAgICBVc2UgVjggY29tcGxldGlvbiBjYWxsYmFjayBB UEkgdG8gYXNzZXJ0IHRoYXQgVjhSZWN1cnNpb25TY29wZSBpcyBvbiB0aGUgc3RhY2sgd2hlbmV2 ZXIgaW52b2tpbmcgc2NyaXB0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9jaHJvbWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuY3BwIGIvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vY2MvQ0NMYXllclRyZWVIb3N0LmNwcAppbmRleCBi YmMyNzg2NWNjM2NhN2JkODZjOWRhYTQxZTU4M2M1NTExYzkyN2E2Li4xMTk4NzY1ZTQzMTI0NzQy YzljYTdiNDc4NmE5M2Y0ZGFiYTc5YWQzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuY3BwCisrKyBiL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0NDTGF5ZXJUcmVlSG9zdC5j cHAKQEAgLTMxNCw2ICszMTQsOSBAQCBib29sIENDTGF5ZXJUcmVlSG9zdDo6Y29tbWl0UmVxdWVz dGVkKCkgY29uc3QKIAogdm9pZCBDQ0xheWVyVHJlZUhvc3Q6OnNldEFuaW1hdGlvbkV2ZW50cyhQ YXNzT3duUHRyPENDQW5pbWF0aW9uRXZlbnRzVmVjdG9yPiBldmVudHMsIGRvdWJsZSB3YWxsQ2xv Y2tUaW1lKQogeworICAgIGlmICghbV9yb290TGF5ZXIpCisgICAgICAgIHJldHVybjsKKwogICAg IEFTU0VSVChDQ1RocmVhZFByb3h5Ojppc01haW5UaHJlYWQoKSk7CiAgICAgc2V0QW5pbWF0aW9u RXZlbnRzUmVjdXJzaXZlKCpldmVudHMsIG1fcm9vdExheWVyLmdldCgpLCB3YWxsQ2xvY2tUaW1l KTsKIH0K 135634 2012-04-04 11:10:43 -0700 2012-04-04 13:31:40 -0700 Patch bug-83060-20120404141042.patch text/plain 2594 vollick U3VidmVyc2lvbiBSZXZpc2lvbjogMTEzMTgyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZDVkYmExNmYxZjIyOWI1 YmM1ODEyNzNkYmQwZGU0ZjZhM2I1ZWZiYi4uMjU2OGMxYjQyNjI4ZWZhMWEzMDI3NTdjMDJjOGI1 OGU0ZGNhZjgxNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEyLTA0LTA0ICBJYW4g Vm9sbGljayAgPHZvbGxpY2tAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtjaHJvbWl1bV0gV2hl biBzZXR0aW5nIGFuaW1hdGlvbiBzdGFydGVkIGV2ZW50cywgc2hvdWxkIGNoZWNrIHRoZSByb290 IGxheWVyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04 MzA2MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5v IG5ldyB0ZXN0cy4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0ND TGF5ZXJUcmVlSG9zdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDQ0xheWVyVHJlZUhvc3Q6OmFu aW1hdGVMYXllcnMpOgorICAgICAgICAoV2ViQ29yZTo6Q0NMYXllclRyZWVIb3N0OjphbmltYXRl TGF5ZXJzUmVjdXJzaXZlKToKKyAgICAgICAgKFdlYkNvcmU6OkNDTGF5ZXJUcmVlSG9zdDo6c2V0 QW5pbWF0aW9uRXZlbnRzUmVjdXJzaXZlKToKKwogMjAxMi0wNC0wNCAgQWxsYW4gU2FuZGZlbGQg SmVuc2VuICA8YWxsYW4uamVuc2VuQG5va2lhLmNvbT4KIAogICAgICAgICBCZXN0IHpvb21hYmxl IGFyZWEgZG9lcyBub3QgYmFsYW5jZSBpbnRlcnNlY3Rpb24gd2l0aCB0YXJnZXQgYXJlYS4KZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0ND TGF5ZXJUcmVlSG9zdC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJv bWl1bS9jYy9DQ0xheWVyVHJlZUhvc3QuY3BwCmluZGV4IGJiYzI3ODY1Y2MzY2E3YmQ4NmM5ZGFh NDFlNTgzYzU1MTFjOTI3YTYuLmQzZmM5NzBiZDM2NzNkNzFjODIwYmEzMzhhNmJkMDZkNDYyMzU2 ZWUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVt L2NjL0NDTGF5ZXJUcmVlSG9zdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvY2hyb21pdW0vY2MvQ0NMYXllclRyZWVIb3N0LmNwcApAQCAtNjU4LDcgKzY1OCw3IEBA IHZvaWQgQ0NMYXllclRyZWVIb3N0OjpkZWxldGVUZXh0dXJlQWZ0ZXJDb21taXQoUGFzc093blB0 cjxNYW5hZ2VkVGV4dHVyZT4gdGV4dHVyCiAKIHZvaWQgQ0NMYXllclRyZWVIb3N0OjphbmltYXRl TGF5ZXJzKGRvdWJsZSBtb25vdG9uaWNUaW1lKQogewotICAgIGlmICghbV9zZXR0aW5ncy50aHJl YWRlZEFuaW1hdGlvbkVuYWJsZWQgfHwgIW1fbmVlZHNBbmltYXRlTGF5ZXJzIHx8ICFtX3Jvb3RM YXllcikKKyAgICBpZiAoIW1fc2V0dGluZ3MudGhyZWFkZWRBbmltYXRpb25FbmFibGVkIHx8ICFt X25lZWRzQW5pbWF0ZUxheWVycykKICAgICAgICAgcmV0dXJuOwogCiAgICAgVFJBQ0VfRVZFTlQo IkNDTGF5ZXJUcmVlSG9zdEltcGw6OmFuaW1hdGVMYXllcnMiLCB0aGlzLCAwKTsKQEAgLTY2Nyw2 ICs2NjcsOSBAQCB2b2lkIENDTGF5ZXJUcmVlSG9zdDo6YW5pbWF0ZUxheWVycyhkb3VibGUgbW9u b3RvbmljVGltZSkKIAogYm9vbCBDQ0xheWVyVHJlZUhvc3Q6OmFuaW1hdGVMYXllcnNSZWN1cnNp dmUoTGF5ZXJDaHJvbWl1bSogY3VycmVudCwgZG91YmxlIG1vbm90b25pY1RpbWUpCiB7CisgICAg aWYgKCFjdXJyZW50KQorICAgICAgICByZXR1cm4gZmFsc2U7CisKICAgICBib29sIHN1YnRyZWVO ZWVkc0FuaW1hdGVMYXllcnMgPSBmYWxzZTsKICAgICBDQ0xheWVyQW5pbWF0aW9uQ29udHJvbGxl ciogY3VycmVudENvbnRyb2xsZXIgPSBjdXJyZW50LT5sYXllckFuaW1hdGlvbkNvbnRyb2xsZXIo KTsKICAgICBjdXJyZW50Q29udHJvbGxlci0+YW5pbWF0ZShtb25vdG9uaWNUaW1lLCAwKTsKQEAg LTY4NSw2ICs2ODgsOSBAQCBib29sIENDTGF5ZXJUcmVlSG9zdDo6YW5pbWF0ZUxheWVyc1JlY3Vy c2l2ZShMYXllckNocm9taXVtKiBjdXJyZW50LCBkb3VibGUgbW9ubwogCiB2b2lkIENDTGF5ZXJU cmVlSG9zdDo6c2V0QW5pbWF0aW9uRXZlbnRzUmVjdXJzaXZlKGNvbnN0IENDQW5pbWF0aW9uRXZl bnRzVmVjdG9yJiBldmVudHMsIExheWVyQ2hyb21pdW0qIGxheWVyLCBkb3VibGUgd2FsbENsb2Nr VGltZSkKIHsKKyAgICBpZiAoIWxheWVyKQorICAgICAgICByZXR1cm47CisKICAgICBmb3IgKHNp emVfdCBldmVudEluZGV4ID0gMDsgZXZlbnRJbmRleCA8IGV2ZW50cy5zaXplKCk7ICsrZXZlbnRJ bmRleCkgewogICAgICAgICBpZiAobGF5ZXItPmlkKCkgPT0gZXZlbnRzW2V2ZW50SW5kZXhdLmxh eWVySWQpCiAgICAgICAgICAgICBsYXllci0+bm90aWZ5QW5pbWF0aW9uU3RhcnRlZChldmVudHNb ZXZlbnRJbmRleF0sIHdhbGxDbG9ja1RpbWUpOwo=