Summary: | InlineIterator position (unsigned int) variable can wrap around | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, dino, esprehn+autocc, glenn, hyatt, kondapallykalyan, rniwa, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2014-03-20 15:16:54 PDT
Created attachment 227347 [details]
Patch
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 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
Comment on attachment 227347 [details]
Patch
r=me
Unable to reproduce test failures. Created attachment 227497 [details]
Resubmit to EWS
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? Created attachment 227718 [details]
Patch
Created attachment 227719 [details]
More asserts
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. Can you explain what you mean by "wraparound amount?" Created attachment 227728 [details]
InlineIterator has new boolean value
(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. 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. *** Bug 130552 has been marked as a duplicate of this bug. *** 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. This caused one test to assert: http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r166328%20(3608)/results.html Failing test |