Bug 125614 – Set m_pos as private in InlineIterator, and use getter and setter functions

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Gyuyoung Kim]

InlineIterator has been exported m_pos as public directly though it is member variable. This patch set it as private, and add getter/setter functions for it.

[Gyuyoung Kim]

Created attachment 219032 [details]
Patch

[Build Bot]

Comment on attachment 219032 [details]
Patch

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

New failing tests:
accessibility/anchor-linked-anonymous-block-crash.html
canvas/philip/tests/2d.composite.canvas.destination-in.html
canvas/philip/tests/2d.composite.canvas.source-out.html
accessibility/aria-hidden-negates-no-visibility.html
compositing/direct-image-compositing.html
accessibility/aria-checkbox-checked.html
canvas/philip/tests/2d.composite.canvas.copy.html
accessibility/alt-tag-on-image-with-nonimage-role.html
canvas/philip/tests/2d.composite.canvas.source-in.html
accessibility/accessibility-node-reparent.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
accessibility/aria-labeled-with-hidden-node.html
compositing/layers-inside-overflow-scroll.html
accessibility/accessibility-object-detached.html
canvas/philip/tests/2d.composite.canvas.source-atop.html
accessibility/aria-activedescendant-crash.html
accessibility/aria-disabled-propagated-to-children.html
accessibility/aria-hidden-updates-alldescendants.html
accessibility/aria-describedby-on-input.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.canvas.destination-over.html
accessibility/aria-invalid.html
accessibility/aria-fallback-roles.html
canvas/philip/tests/2d.composite.canvas.lighter.html
canvas/philip/tests/2d.composite.canvas.xor.html
canvas/philip/tests/2d.composite.canvas.source-over.html
accessibility/adjacent-continuations-cause-assertion-failure.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

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

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

[Gyuyoung Kim]

Created attachment 219043 [details]
Patch for ews

[Gyuyoung Kim]

CC'ing Kling

[Gyuyoung Kim]

Created attachment 220007 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 220007 [details]
Patch

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

This is a good iterative improvement.

But manipulating offsets using +/- 1 looks suspicious. Won't this break on Unicode characters that don't fit in 16 bits?

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:924
> +                unsigned currentPosition = m_startOfIgnoredSpaces.offset();
> +                m_startOfIgnoredSpaces.setOffset(--currentPosition);

I don't think that the name currentPosition is helpful or even correct.

Can we write it like this?

    m_startOfIgnoredSpaces.setOffset(m_startOfIgnoredSpaces.offset() - 1);

But then again, this is an iterator, it should know how to iterate. Maybe the right answer is to add a fastDecrementInTextNode() function (there is already fastIncrementInTextNode()), and use that?

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1085
> +                unsigned currentPosition = endpoint.offset();
> +                endpoint.setOffset(--currentPosition);

Same comment about bad name and iteration that should be encapsulated.

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1135
> +        unsigned currentPosition = m_lineBreak.offset();
> +        m_lineBreak.setOffset(--currentPosition);

Same comment about bad name and iteration that should be encapsulated.

> Source/WebCore/rendering/line/TrailingObjects.cpp:46
> +            unsigned currentPosition = lineMidpointState.midpoints[trailingSpaceMidpoint].offset();
> +            lineMidpointState.midpoints[trailingSpaceMidpoint].setOffset(--currentPosition);

Same comment about bad name and iteration that should be encapsulated.

[Gyuyoung Kim]

Created attachment 220015 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #7)

> But then again, this is an iterator, it should know how to iterate. Maybe the right answer is to add a fastDecrementInTextNode() function (there is already fastIncrementInTextNode()), and use that?

Let me think about this on new bug. Thanks.

[WebKit Commit Bot]

Comment on attachment 220015 [details]
Patch

Rejecting attachment 220015 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 220015, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/6126968416239616

[Ryosuke Niwa]

Comment on attachment 220015 [details]
Patch

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

Nice cleanup!

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2059
> +    bool appliedStartWidth = resolver.position().offset() > 0;

Since offset() is unsigned, we don't need to compare it against 0.
i.e. "bool appliedStartWidth = resolver.position().offset();"
to follow our style guide https://www.webkit.org/coding/coding-style.html#zero-comparison

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:855
> +                if (!stoppedIgnoringSpaces && m_current.offset() > 0)

Ditto about comparing against 0.  Perhaps it's better to add a helper function like m_current.isAtStart()?

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:909
> +        if (isSVGText && m_current.offset() > 0) {

Ditto.

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1130
> +    if (m_lineBreak.offset() > 0) {

Ditto.

[Gyuyoung Kim]

Created attachment 220020 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #11)
> (From update of attachment 220015 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220015&action=review
> 
> Nice cleanup!
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2059
> > +    bool appliedStartWidth = resolver.position().offset() > 0;
> 
> Since offset() is unsigned, we don't need to compare it against 0.
> i.e. "bool appliedStartWidth = resolver.position().offset();"
> to follow our style guide https://www.webkit.org/coding/coding-style.html#zero-comparison
> 
> > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:855
> > +                if (!stoppedIgnoringSpaces && m_current.offset() > 0)
> 
> Ditto about comparing against 0.  Perhaps it's better to add a helper function like m_current.isAtStart()?
> 
> > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:909
> > +        if (isSVGText && m_current.offset() > 0) {
> 
> Ditto.
> 
> > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1130
> > +    if (m_lineBreak.offset() > 0) {
> 
> Ditto.

Nice detection ! Fixed.

[WebKit Commit Bot]

Comment on attachment 220020 [details]
Patch

Clearing flags on attachment: 220020

Committed r161085: <http://trac.webkit.org/changeset/161085>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.