94889 2012-08-23 18:53:39 -0700 Remove the now-unneeded invalidations in RenderTable::removeCaption 2012-08-30 07:48:02 -0700 1 1 1 Unclassified WebKit Tables 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 jchaffraix jchaffraix eric hyatt inferno robert webkit.review.bot oldest_to_newest 703649 0 jchaffraix 2012-08-23 18:53:39 -0700 Following bug 94842, we moved the post-removal invalidation to RenderTable's children. While doing so, the invalidation code in RenderTable::removeCaption appeared to be some old code that was introduced when we didn't support several captions in the same table (see for example bug 58249). It should be safe to remove it now. 703653 1 160308 jchaffraix 2012-08-23 19:04:41 -0700 Created attachment 160308 Proposed code removal. 703715 2 160308 inferno 2012-08-23 21:19:47 -0700 Comment on attachment 160308 Proposed code removal. View in context: https://bugs.webkit.org/attachment.cgi?id=160308&action=review > Source/WebCore/rendering/RenderTable.cpp:-209 > - node()->setNeedsStyleRecalc(); Can you please explain in a little more detail why this change is not necessary now ? > Source/WebCore/rendering/RenderTable.cpp:-211 > - setNeedsSectionRecalc(); Good change to remove setNeedsSectionRecalc() here, but this is incomplete without removing the m_captions manipulation in recalcsections. recalcsections shouldn't be worrying about captions. 703734 3 inferno 2012-08-23 21:56:37 -0700 actually we need RenderTableCaption::insertedIntoTree and RenderTableCaption::removedFromTree. insertedIntoTree is pretty important since currently m_captions is only set in ::addChild 705259 4 160308 jchaffraix 2012-08-27 07:34:28 -0700 Comment on attachment 160308 Proposed code removal. View in context: https://bugs.webkit.org/attachment.cgi?id=160308&action=review >> Source/WebCore/rendering/RenderTable.cpp:-209 >> - node()->setNeedsStyleRecalc(); > > Can you please explain in a little more detail why this change is not necessary now ? Sure. First having rendering force a style recalc is a violation of how style recalc flows (it flows from CSS / JavaScript updates and forces the render tree to update) and should be considered carefully. Here I see no indication that our style may change due to the caption removal. Mostly, this code was added to solve bug 58249. The idea was to re-attach most of the table child following a caption removal. This call was never justified and was done to ensure that table children were properly re-attached on table caption removal. As we don't re-attach the table children on table removal, it is now dead code. >> Source/WebCore/rendering/RenderTable.cpp:-211 >> - >> - setNeedsSectionRecalc(); > > Good change to remove setNeedsSectionRecalc() here, but this is incomplete without removing the m_captions manipulation in recalcsections. recalcsections shouldn't be worrying about captions. Fair enough, I would have qualified as orthogonal but it's likely better to just sanitize this logic while at it. 705368 5 inferno 2012-08-27 10:17:32 -0700 Ok, can you please rebaseline the patch after https://bugs.webkit.org/show_bug.cgi?id=95090 is landed. ah! i now remember the style recalc hack. i remember adding it when we only supported one table caption and when it is getting removed, we trigger style recalc so that other captions can be set to m_caption. Robert, u ok with removing this ? Seems like you didn't remove it when you added the multiple captions support. Any reason not to remove it ? I hope there is no usecase we are missing here. 705439 6 robert 2012-08-27 11:31:17 -0700 (In reply to comment #5) > Ok, can you please rebaseline the patch after https://bugs.webkit.org/show_bug.cgi?id=95090 is landed. > > ah! i now remember the style recalc hack. i remember adding it when we only supported one table caption and when it is getting removed, we trigger style recalc so that other captions can be set to m_caption. > > Robert, u ok with removing this ? Seems like you didn't remove it when you added the multiple captions support. Any reason not to remove it ? I hope there is no usecase we are missing here. I can't think of any good reason for keeping it! 705993 7 160882 jchaffraix 2012-08-27 19:11:01 -0700 Created attachment 160882 Proposed change: removed setNeedsStylRecalc per discussion. 707003 8 160882 webkit.review.bot 2012-08-28 19:24:12 -0700 Comment on attachment 160882 Proposed change: removed setNeedsStylRecalc per discussion. Rejecting attachment 160882 from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: _by_email return self._reviewer_only(self.account_by_email(email)) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/config/committers.py", line 687, in account_by_email return self._email_to_account_map().get(email.lower()) if email else None File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/config/committers.py", line 580, in _email_to_account_map assert(email not in self._accounts_by_email) # We should never have duplicate emails. AssertionError Full output: http://queues.webkit.org/results/13682017 708613 9 jchaffraix 2012-08-30 07:48:02 -0700 Committed r127139: <http://trac.webkit.org/changeset/127139> 160308 2012-08-23 19:04:41 -0700 2012-08-27 19:10:58 -0700 Proposed code removal. bug-94889-20120823190438.patch text/plain 2038 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogMTI2NDk2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMWVlZjQyYmMyZWQ3OGVi MTQzZmRmMDc0ZmUzODFiNjIzZDIwYjU2MC4uODE2MWE5MGU5MDI2NDY5MzFjMjNkOGJhYWJjNzM4 MDQ5YzZmNDZhMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDEyLTA4LTIzICBKdWxp ZW4gQ2hhZmZyYWl4ICA8amNoYWZmcmFpeEB3ZWJraXQub3JnPgorCisgICAgICAgIFJlbW92ZSB0 aGUgbm93LXVubmVlZGVkIGludmFsaWRhdGlvbnMgaW4gUmVuZGVyVGFibGU6OnJlbW92ZUNhcHRp b24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk0ODg5 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRm9sbG93 aW5nIGJ1ZyA5NDg0MiwgdGhlIGludmFsaWRhdGlvbiBjb2RlIHdhcyBwdXNoZWQgZG93biB0byBS ZW5kZXJUYWJsZQorICAgICAgICBjaGlsZHJlbi4gVGhpcyBtYWRlIGFwcGFyZW50IHRoYXQgdGhl IGNhcHRpb24gaW52YWxpZGF0aW9uIGNvZGUgd2FzIG9sZCBhbmQKKyAgICAgICAgYmVsb25nZWQg dG8gYSB0aW1lIHdoZXJlIHdlIGRpZG4ndCBzdXBwb3J0IG11bHRpcGxlIGNhcHRpb24uCisKKyAg ICAgICAgUmVmYWN0b3JpbmcgY292ZXJlZCBieSB0aGUgZXhpc3RpbmcgdGFibGUgdGVzdHMuCisK KyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyVGFibGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 UmVuZGVyVGFibGU6OnJlbW92ZUNhcHRpb24pOgorICAgICAgICBSZW1vdmVkIHRoZSBjYWxsIHRv IHJlY2FsY1N0eWxlKCkgYW5kIHNldE5lZWRzU2VjdGlvbnNSZWNhbGMoKSBhcyB3ZSBkb24ndCBu ZWVkCisgICAgICAgIHRvIHNob3cgYW5kIGNvbGxlY3Qgb3VyIG9ubHkgY2FwdGlvbiB3aGVuIGl0 IGlzIHJlbW92ZWQuIFRoZSBtdWx0aXBsZSBjYXB0aW9ucyBjb2RlCisgICAgICAgIGFscmVhZHkg a2VlcCB0cmFja3Mgb2YgdGhlIGNhcHRpb25zIGFuZCBpbnZhbGlkYXRlIGFzIG5lZWRlZC4KKwog MjAxMi0wOC0yMyAgRG9taW5pYyBNYXp6b25pICA8ZG1henpvbmlAZ29vZ2xlLmNvbT4KIAogICAg ICAgICBBWDogRm9jdXNhYmxlIGVsZW1lbnRzIHdpdGhvdXQgYSByb2xlIHNob3VsZCBub3QgYmUg aWdub3JlZApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRhYmxl LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUYWJsZS5jcHAKaW5kZXggYjVj MzA4MzczZGYzNmQ2YjI4NzZjOWFjNTM5ZTdlZDAwZDk2ZTRjNy4uZGE3MDM5YWZlZTk1ZDlmYWEz Yjg0NTA3ZmE1ZGRkYjdhZDc0YjFhMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVy aW5nL1JlbmRlclRhYmxlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy VGFibGUuY3BwCkBAIC0yMDIsMTMgKzIwMiw2IEBAIHZvaWQgUmVuZGVyVGFibGU6OnJlbW92ZUNh cHRpb24oY29uc3QgUmVuZGVyVGFibGVDYXB0aW9uKiBvbGRDYXB0aW9uKQogICAgIHNpemVfdCBp bmRleCA9IG1fY2FwdGlvbnMuZmluZChvbGRDYXB0aW9uKTsKICAgICBBU1NFUlQoaW5kZXggIT0g bm90Rm91bmQpOwogICAgIG1fY2FwdGlvbnMucmVtb3ZlKGluZGV4KTsKLQotICAgIC8vIEZJWE1F OiBUaGUgcmVzdCBvZiB0aGlzIGZ1bmN0aW9uIGlzIHByb2JhYmx5IG5vdCBuZWVkZWQgc2luY2Ug d2UgaGF2ZSAKLSAgICAvLyBpbXBsZW1lbnRlZCBwcm9wZXIgbXVsdGlwbGUgY2FwdGlvbnMgc3Vw cG9ydCAoc2VlIGJ1ZyA1ODI0OSkuCi0gICAgaWYgKG5vZGUoKSkKLSAgICAgICAgbm9kZSgpLT5z ZXROZWVkc1N0eWxlUmVjYWxjKCk7Ci0KLSAgICBzZXROZWVkc1NlY3Rpb25SZWNhbGMoKTsKIH0K IAogdm9pZCBSZW5kZXJUYWJsZTo6Y29tcHV0ZUxvZ2ljYWxXaWR0aCgpCg== 160882 2012-08-27 19:11:01 -0700 2012-08-28 19:24:12 -0700 Proposed change: removed setNeedsStylRecalc per discussion. bug-94889-20120827191053.patch text/plain 1868 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogMTI2ODM0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDk2M2U2OGE0MzNjZTYw YjcwNWEzY2YyMGY3NWYxMTFhMjA4ZGQ1NS4uZTIxNzZhZDQ3ZGQyYzY5YWRjNTVlOTNkODZlYTBk NTIyZjcyZGU2YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDEyLTA4LTI3ICBKdWxp ZW4gQ2hhZmZyYWl4ICA8amNoYWZmcmFpeEB3ZWJraXQub3JnPgorCisgICAgICAgIFJlbW92ZSB0 aGUgbm93LXVubmVlZGVkIGludmFsaWRhdGlvbnMgaW4gUmVuZGVyVGFibGU6OnJlbW92ZUNhcHRp b24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk0ODg5 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRm9sbG93 aW5nIGJ1ZyA5NDg0MiBhbmQgOTUwOTAsIHRoZSBpbnZhbGlkYXRpb24gY29kZSB3YXMgcHVzaGVk IGRvd24gdG8KKyAgICAgICAgUmVuZGVyVGFibGVDYXB0aW9uLiBUaGlzIG1hZGUgYXBwYXJlbnQg dGhhdCB3ZSBkaWQgc29tZSBpbnZhbGlkYXRpb25zIHRoYXQKKyAgICAgICAgd2VyZSBub3QgbmVl ZGVkLgorCisgICAgICAgIFJlZmFjdG9yaW5nIGNvdmVyZWQgYnkgZXhpc3RpbmcgdGVzdHMuCisK KyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyVGFibGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 UmVuZGVyVGFibGU6OnJlbW92ZUNhcHRpb24pOgorICAgICAgICBSZW1vdmVkIHNldE5lZWRzUmVj YWxjU3R5bGUoKSBhcyBpdCdzIHVubmVlZGVkIG5vdy4gSXQgd2FzIHByb2JhYmx5IG5lZWRlZCBi YWNrIHdoZW4KKyAgICAgICAgd2UgZGlkbid0IHN1cHBvcnQgbXVsdGlwbGUgY2FwdGlvbnMgKHNl ZSBidWcgNTgyNDkpIGJ1dCB0aGUgbmVlZCB3YXMgbmV2ZXIgZG9jdW1lbnRlZAorICAgICAgICBz byB5b3UgY291bGQgd29uZGVyIGlmIGl0IHdhcyByZWFsbHkgbmVlZGVkIGluIHRoZSBmaXJzdCBw bGFjZS4KKwogMjAxMi0wOC0yNyAgRGVhbiBKYWNrc29uICA8ZGlub0BhcHBsZS5jb20+CiAKICAg ICAgICAgVW5yZXZpZXdlZCBhdHRlbXB0ZWQgYnVpbGQgZml4IGZvciBNYWMuCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGFibGUuY3BwIGIvU291cmNlL1dlYkNv cmUvcmVuZGVyaW5nL1JlbmRlclRhYmxlLmNwcAppbmRleCA3Mzg1OTM2ZmNkMWE5NzY0ZmJmZWQ1 ODVkNTBlNzFiNjQxOGFkYjhiLi45MzQxOGJkNDI1NTExNWU5Y2FlODZlZGRiNWQyYzIwMzRhNGY0 YzY1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGFibGUuY3Bw CisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUYWJsZS5jcHAKQEAgLTIxMCwx MSArMjEwLDYgQEAgdm9pZCBSZW5kZXJUYWJsZTo6cmVtb3ZlQ2FwdGlvbihjb25zdCBSZW5kZXJU YWJsZUNhcHRpb24qIG9sZENhcHRpb24pCiAgICAgICAgIHJldHVybjsKIAogICAgIG1fY2FwdGlv bnMucmVtb3ZlKGluZGV4KTsKLQotICAgIC8vIEZJWE1FOiBUaGUgcmVzdCBvZiB0aGlzIGZ1bmN0 aW9uIGlzIHByb2JhYmx5IG5vdCBuZWVkZWQgc2luY2Ugd2UgaGF2ZSAKLSAgICAvLyBpbXBsZW1l bnRlZCBwcm9wZXIgbXVsdGlwbGUgY2FwdGlvbnMgc3VwcG9ydCAoc2VlIGJ1ZyA1ODI0OSkuCi0g ICAgaWYgKG5vZGUoKSkKLSAgICAgICAgbm9kZSgpLT5zZXROZWVkc1N0eWxlUmVjYWxjKCk7CiB9 CiAKIHZvaWQgUmVuZGVyVGFibGU6OmNvbXB1dGVMb2dpY2FsV2lkdGgoKQo=