It would make more sense for it to return an unsigned.
Created attachment 233210 [details] Patch
<rdar://problem/17337617>
Created attachment 233240 [details] Patch
Attachment 233240 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106: The parameter name "const" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
*** Bug 133594 has been marked as a duplicate of this bug. ***
Comment on attachment 233240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233240&action=review > Source/WebCore/platform/graphics/Font.cpp:340 > + unsigned to = (initialTo == -1 ? run.length() : initialTo); Can initialTo ever be -2 (or lower)? Should this be "initialTo < 0 ?", as it is in 'drawEmphasisMarks'? > Source/WebCore/platform/graphics/Font.cpp:511 > + unsigned to = (initialTo == -1 ? run.length() : initialTo); Ditto above question. > Source/WebCore/platform/graphics/Font.cpp:1068 > + unsigned offsetInString = static_cast<unsigned>(initialOffsetInString); What if initialOffsetInString is negative?
Comment on attachment 233240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233240&action=review >> Source/WebCore/platform/graphics/Font.cpp:340 >> + unsigned to = (initialTo == -1 ? run.length() : initialTo); > > Can initialTo ever be -2 (or lower)? Should this be "initialTo < 0 ?", as it is in 'drawEmphasisMarks'? It shouldn't be, but it's simple enough to change the conditional. >> Source/WebCore/platform/graphics/Font.cpp:511 >> + unsigned to = (initialTo == -1 ? run.length() : initialTo); > > Ditto above question. Same answer. >> Source/WebCore/platform/graphics/Font.cpp:1068 >> + unsigned offsetInString = static_cast<unsigned>(initialOffsetInString); > > What if initialOffsetInString is negative? It should never be - the only negative value that it should ever have is -1 (kNoOffset). I'll add an assert for this.
Created attachment 233253 [details] Patch
Attachment 233253 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106: The parameter name "const" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
The style red bubble is a false negative, and it looks like the EFL bot was killed during compilation
Comment on attachment 233253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233253&action=review > Source/WebCore/rendering/InlineTextBox.cpp:202 > + if (endPos < m_start) if (endPos <= m_start) > Source/WebCore/rendering/InlineTextBox.cpp:204 > + unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len)); Could you change m_len's type to unsigned too? I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int. Its setter looks wrong too and can result in unsigned overflow: void setLen(unsigned len) { m_len = len; } So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short. It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change. > Source/WebCore/rendering/InlineTextBox.cpp:278 > + if (endPos < m_start) same here. though now that I am seeing it the second time, it might be better to keep the old structure, though I don't feel too strongly about it. (shaving off a std::min() call vs. better code structuring (better code reading, less error prone))
Comment on attachment 233253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233253&action=review >>> Source/WebCore/rendering/InlineTextBox.cpp:204 >>> + unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len)); >> >> Could you change m_len's type to unsigned too? >> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int. >> >> Its setter looks wrong too and can result in unsigned overflow: >> void setLen(unsigned len) { m_len = len; } >> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short. >> >> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change. > > Could you change m_len's type to unsigned too? > I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int. > > Its setter looks wrong too and can result in unsigned overflow: > void setLen(unsigned len) { m_len = len; } > So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short. > > It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change. That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.
(In reply to comment #13) > (From update of attachment 233253 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233253&action=review > > >>> Source/WebCore/rendering/InlineTextBox.cpp:204 > >>> + unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len)); > >> > >> Could you change m_len's type to unsigned too? > >> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int. > >> > >> Its setter looks wrong too and can result in unsigned overflow: > >> void setLen(unsigned len) { m_len = len; } > >> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short. > >> > >> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change. > > > > Could you change m_len's type to unsigned too? > > I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int. > > > > Its setter looks wrong too and can result in unsigned overflow: > > void setLen(unsigned len) { m_len = len; } > > So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short. > > > > It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change. > > That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics. On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch.
Created attachment 233476 [details] Patch
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 233253 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=233253&action=review > > > > >>> Source/WebCore/rendering/InlineTextBox.cpp:204 > > >>> + unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len)); > > >> > > >> Could you change m_len's type to unsigned too? > > >> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int. > > >> > > >> Its setter looks wrong too and can result in unsigned overflow: > > >> void setLen(unsigned len) { m_len = len; } > > >> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short. > > >> > > >> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change. > > > > > > Could you change m_len's type to unsigned too? > > > I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int. > > > > > > Its setter looks wrong too and can result in unsigned overflow: > > > void setLen(unsigned len) { m_len = len; } > > > So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short. > > > > > > It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change. > > > > That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics. > On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch. Really? Signed math isn't always the same speed as unsigned math on all hardware? I had no idea.
Attachment 233476 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106: The parameter name "const" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #16) > > > That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics. > > On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch. > > Really? Signed math isn't always the same speed as unsigned math on all hardware? I had no idea. ARMv7 doesn't have an instruction for signed modulo, for instance.
Comment on attachment 233476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233476&action=review > Source/WebCore/rendering/InlineTextBox.cpp:278 > + if (endPos < m_start) endPos <= m_start
http://trac.webkit.org/changeset/170265
Re-opened since this is blocked by bug 134174
The problem was a "<" vs a "<=". Re-landed in http://trac.webkit.org/changeset/170337.
(In reply to comment #22) > The problem was a "<" vs a "<=". Re-landed in http://trac.webkit.org/changeset/170337. It made 3 tests crash on Apple Mac debug bots.
Re-opened since this is blocked by bug 134250
Having trouble reproducing locally :(
Was able to reproduce these, and the fix is trivial. I'll land again shortly. Hopefully this time it sticks.
http://trac.webkit.org/changeset/170390