Bug 94889 – Remove the now-unneeded invalidations in RenderTable::removeCaption

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 160308 [details]
Proposed code removal.

[Abhishek Arya]

Comment on attachment 160308 [details]
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.

[Abhishek Arya]

actually we need RenderTableCaption::insertedIntoTree and RenderTableCaption::removedFromTree. insertedIntoTree is pretty important since currently m_captions is only set in ::addChild

[Julien Chaffraix]

Comment on attachment 160308 [details]
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.

[Abhishek Arya]

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.

[Robert Hogan]

(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!

[Julien Chaffraix]

Created attachment 160882 [details]
Proposed change: removed setNeedsStylRecalc per discussion.

[WebKit Review Bot]

Comment on attachment 160882 [details]
Proposed change: removed setNeedsStylRecalc per discussion.

Rejecting attachment 160882 [details] 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

[Julien Chaffraix]

Committed r127139: <http://trac.webkit.org/changeset/127139>