Bug 79856 - <msubsup> setNeedsLayout() correction
Summary: <msubsup> setNeedsLayout() correction
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Julien Chaffraix
Depends on:
Reported: 2012-02-28 18:54 PST by Dave Barton
Modified: 2012-02-29 17:17 PST (History)
2 users (show)

See Also:

Patch (1.98 KB, patch)
2012-02-28 19:01 PST, Dave Barton
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2012-02-29 10:05 PST, Dave Barton
no flags Details | Formatted Diff | Diff
Patch for landing (1.87 KB, patch)
2012-02-29 12:06 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Barton 2012-02-28 18:54:07 PST
<msubsup> setNeedsLayout() correction
Comment 1 Dave Barton 2012-02-28 19:01:13 PST
Created attachment 129376 [details]
Comment 2 Julien Chaffraix 2012-02-28 19:11:29 PST
Comment on attachment 129376 [details]

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.
Comment 3 Dave Barton 2012-02-29 10:05:49 PST
Created attachment 129467 [details]
Comment 4 Dave Barton 2012-02-29 10:10:12 PST
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.
Comment 5 WebKit Review Bot 2012-02-29 11:21:26 PST
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.
Comment 6 Julien Chaffraix 2012-02-29 12:04:32 PST
Comment on attachment 129467 [details]

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.
Comment 7 Julien Chaffraix 2012-02-29 12:06:40 PST
Created attachment 129488 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-02-29 17:17:50 PST
Comment on attachment 129488 [details]
Patch for landing

Clearing flags on attachment: 129488

Committed r109286: <http://trac.webkit.org/changeset/109286>
Comment 9 WebKit Review Bot 2012-02-29 17:17:54 PST
All reviewed patches have been landed.  Closing bug.