Bug 65723 – A few purely stylistic modifications to visible_units.cpp

• Component: HTML Editing
• Status: RESOLVED FIXED

[Van Lam]

See title

[Van Lam]

Created attachment 102988 [details]
Proposed fix

Renamed greatestValueUnder to greatestOffsetUnder, positionIsInsideBox to positionIsInBoxButNotOnBoundary (to avoid confusion with positionIsInBox, which is just a getInlineBoxAndOffset check).
Removed use of invalidOffset as an error value in greatestOffsetUnder and smallestOffsetAbove since semantically it should only be used to check if it makes sense to compare offsets in a single box.

[Ryosuke Niwa]

Comment on attachment 102988 [details]
Proposed fix

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

> Source/WebCore/editing/visible_units.cpp:1644
> +    if (index != -1)

Probably better to check index < 0.

> Source/WebCore/editing/visible_units.cpp:1681
> +    if (index != -1)

Ditto.

[Van Lam]

Created attachment 102991 [details]
Revised fix

Changed comparison index != -1 to index >= 0.

[Ryosuke Niwa]

Comment on attachment 102991 [details]
Revised fix

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

> Source/WebCore/editing/visible_units.cpp:1643
> +    : smallestOffsetAbove(offset, blockDirection == RTL, orderedWordBoundaries);

Oops, cq- because you forgot to indent here.

[Van Lam]

Created attachment 102992 [details]
Revised fix

Indented. Nice catch :)

[WebKit Review Bot]

Comment on attachment 102992 [details]
Revised fix

Clearing flags on attachment: 102992

Committed r92431: <http://trac.webkit.org/changeset/92431>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 102992 [details]
Revised fix

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

> Source/WebCore/editing/visible_units.cpp:1450
> -        return invalidOffset;
> +        return -1;

Replacing a name constant with a magic value of -1 does not seem like a style improvement to me.

[Ryosuke Niwa]

Comment on attachment 102992 [details]
Revised fix

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

>> Source/WebCore/editing/visible_units.cpp:1450
>> +        return -1;
> 
> Replacing a name constant with a magic value of -1 does not seem like a style improvement to me.

invalidOffset was supposed to be used only for offsets in inline boxes.  So this was misused here.  We can introduce new const like NotFound instead.