InlineIterator position (unsigned int) variable can wrap around

[Myles C. Maxfield]

InlineIterator position can wrap around

[Myles C. Maxfield]

Created attachment 227347 [details]
Patch

[Build Bot]

Comment on attachment 227347 [details]
Patch

Attachment 227347 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5871879202013184

New failing tests:
media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html

[Build Bot]

Created attachment 227358 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Dave Hyatt]

Comment on attachment 227347 [details]
Patch

r=me

[Myles C. Maxfield]

Unable to reproduce test failures.

[Myles C. Maxfield]

Created attachment 227497 [details]
Resubmit to EWS

[Darin Adler]

Comment on attachment 227497 [details]
Resubmit to EWS

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

> LayoutTests/ChangeLog:9
> +        * fast/text/whitespace-only-text-in-rtl-expected.txt: Added.
> +        * fast/text/whitespace-only-text-in-rtl.html: Added.

It’s non-obvious what this is testing, and what passing or failing is like. Could you change the test so it’s self documenting?

[Myles C. Maxfield]

Created attachment 227718 [details]
Patch

[Myles C. Maxfield]

Created attachment 227719 [details]
More asserts

[Simon Fraser (smfr)]

Comment on attachment 227719 [details]
More asserts

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

> Source/WebCore/rendering/InlineIterator.h:102
> +    // Leave space after this value so we have room to detect wraparound
> +    static const unsigned kSkipEntireElementOffset = UINT_MAX - 15;

I still think this approach is a bit too magical. Why not give this class another bit of state: a bool that's true if we're in the wraparound state, or give it a separate "wraparound amount" if it can be > 1.

[Myles C. Maxfield]

Can you explain what you mean by "wraparound amount?"

[Myles C. Maxfield]

Created attachment 227728 [details]
InlineIterator has new boolean value

[Simon Fraser (smfr)]

(In reply to comment #11)
> Can you explain what you mean by "wraparound amount?"

I think it was always just 1, right? Not sure why you did the -15 in the previous patch though.

[Simon Fraser (smfr)]

Comment on attachment 227728 [details]
InlineIterator has new boolean value

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

Much more understandable!

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1072
> +                unsigned endpointOffset = endpoint.offset();
> +                if (endpointOffset)
> +                    endpoint.setOffset(endpointOffset - 1);
> +                else
> +                    endpoint.setRefersToEndOfPreviousNode();

Seems like this could be a method on InlineIterator.

[Myles C. Maxfield]

*** Bug 130552 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Comment on attachment 227728 [details]
InlineIterator has new boolean value

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

>> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1072
>> +                    endpoint.setRefersToEndOfPreviousNode();
> 
> Seems like this could be a method on InlineIterator.

Done.

[Myles C. Maxfield]

https://trac.webkit.org/changeset/166245

[Simon Fraser (smfr)]

This caused one test to assert:

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r166328%20(3608)/results.html

[Myles C. Maxfield]

Failing test

[Myles C. Maxfield]

<rdar://problem/16432098>
<rdar://problem/15215021>

[Myles C. Maxfield]

Fixed in https://bugs.webkit.org/show_bug.cgi?id=130874