Bug 79856 – <msubsup> setNeedsLayout() correction

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Dave Barton]

<msubsup> setNeedsLayout() correction

[Dave Barton]

Created attachment 129376 [details]
Patch

[Julien Chaffraix]

Comment on attachment 129376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129376&action=review

> Source/WebCore/ChangeLog:10
> +        superscriptWrapper->setNeedsLayout(true, false); in the last patch actually needs to be
> +        superscriptWrapper->setNeedsLayout(true); in order to mark its parent, m_scripts, as
> +        needing layout also.

I would rather see a patch with your parent marked in this case. As I told you calling setNeedsLayout(true, true) during layout is really something you don't want to do. That is unless you want to open a security can of worms.

> Source/WebCore/ChangeLog:12
> +        No new tests. It's actually difficult to test the need for this with the current code,

I would really love to see a test for 2 reasons:
* this would help your future refactorings.
* this will give some confidence that the code is properly tested.

[Dave Barton]

Created attachment 129467 [details]
Patch

[Dave Barton]

I changed the code to mark the m_scripts parent.

As discussed in private e-mail, the existing tests should cover this fix, except that the current code has a lot of extra setNeedsLayout() calls, so the tests pass anyway. It's hard to come up with a test that wouldn't pass now, but there may be one. We could wait to land this change until we're sure it's needed (the existing tests will start to fail), but that will complicate a future patch, and a potential bug will remain in the meantime.

[WebKit Review Bot]

Attachment 129467 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Comment on attachment 129467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129467&action=review

>> Source/WebCore/ChangeLog:8
>> +    	m_scripts->setNeedsLayout(true, false); needs to be added to the fix for bug 79274.
> 
> Line contains tab character.  [whitespace/tab] [5]

You will not be able to land this. I will fix and submit the patch for landing.

[Julien Chaffraix]

Created attachment 129488 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 129488 [details]
Patch for landing

Clearing flags on attachment: 129488

Committed r109286: <http://trac.webkit.org/changeset/109286>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.