Bug 125614 - Set m_pos as private in InlineIterator, and use getter and setter functions
Summary: Set m_pos as private in InlineIterator, and use getter and setter functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-11 19:25 PST by Gyuyoung Kim
Modified: 2013-12-25 23:28 PST (History)
7 users (show)

See Also:


Attachments
Patch (34.82 KB, patch)
2013-12-11 19:26 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (179.75 KB, application/zip)
2013-12-11 20:47 PST, Build Bot
no flags Details
Patch for ews (34.82 KB, patch)
2013-12-11 22:33 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (34.81 KB, patch)
2013-12-25 15:42 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (34.30 KB, patch)
2013-12-25 17:58 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (34.29 KB, patch)
2013-12-25 20:16 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2013-12-11 19:25:03 PST
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.
Comment 1 Gyuyoung Kim 2013-12-11 19:26:57 PST
Created attachment 219032 [details]
Patch
Comment 2 Build Bot 2013-12-11 20:47:53 PST
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
Comment 3 Build Bot 2013-12-11 20:47:56 PST
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
Comment 4 Gyuyoung Kim 2013-12-11 22:33:25 PST
Created attachment 219043 [details]
Patch for ews
Comment 5 Gyuyoung Kim 2013-12-12 16:16:35 PST
CC'ing Kling
Comment 6 Gyuyoung Kim 2013-12-25 15:42:49 PST
Created attachment 220007 [details]
Patch
Comment 7 Alexey Proskuryakov 2013-12-25 16:43:18 PST
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.
Comment 8 Gyuyoung Kim 2013-12-25 17:58:48 PST
Created attachment 220015 [details]
Patch
Comment 9 Gyuyoung Kim 2013-12-25 18:00:51 PST
(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.
Comment 10 WebKit Commit Bot 2013-12-25 19:43:49 PST
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
Comment 11 Ryosuke Niwa 2013-12-25 19:50:03 PST
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.
Comment 12 Gyuyoung Kim 2013-12-25 20:16:52 PST
Created attachment 220020 [details]
Patch
Comment 13 Gyuyoung Kim 2013-12-25 20:18:32 PST
(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.
Comment 14 WebKit Commit Bot 2013-12-25 23:28:33 PST
Comment on attachment 220020 [details]
Patch

Clearing flags on attachment: 220020

Committed r161085: <http://trac.webkit.org/changeset/161085>
Comment 15 WebKit Commit Bot 2013-12-25 23:28:37 PST
All reviewed patches have been landed.  Closing bug.