Bug 205706 – Enable the offset assertion in HTMLTextFormControlElement::indexForPosition

• Status: REOPENED
• Component: DOM

[Mark Lam]

The assertion was guarded by a #ifndef ASSERT_DISABLED.  ASSERT_DISABLED is always defined, either as 0 or 1.  Hence, this assertion code is never ever built.  Changing the #ifndef ASSERT_DISABLED to #if !ASSERT_DISABLED results in a build error.  We should either fix this assertion, or remove it.

[Mark Lam]

Created attachment 386653 [details]
proposed patch.

[Darin Adler]

Comment on attachment 386653 [details]
proposed patch.

I think this was valuable assertion code; too bad it was left turned off. I understand the code didn’t compile when enabled, but it shouldn’t be too hard to fix. I think it’s likely worth fixing rather than deleting.

[Mark Lam]

Comment on attachment 386653 [details]
proposed patch.

Let's fix the assertion instead of deleting it as Darin requested.

[Darin Adler]

By the way, there's a chance that when we turn on the assertion we'll see failures, which will mean we'll have to turn it back off again until someone can investigate!

[Ryosuke Niwa]

Created attachment 387618 [details]
Enables the assertion

[Ryosuke Niwa]

Created attachment 387621 [details]
Fixed GTK+ builds

[Mark Lam]

Comment on attachment 387621 [details]
Fixed GTK+ builds

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

> Source/WebCore/html/HTMLTextFormControlElement.cpp:647
>      // https://bugs.webkit.org/show_bug.cgi?id=205706.

Please remove the above FIXME and bug url comment now that you’re fixing it.

[Ryosuke Niwa]

(In reply to Mark Lam from comment #7)
> Comment on attachment 387621 [details]
> Fixed GTK+ builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387621&action=review
> 
> > Source/WebCore/html/HTMLTextFormControlElement.cpp:647
> >      // https://bugs.webkit.org/show_bug.cgi?id=205706.
> 
> Please remove the above FIXME and bug url comment now that you’re fixing it.

Oh oops, removed that.

[Ryosuke Niwa]

Created attachment 387625 [details]
Updated

[Darin Adler]

Comment on attachment 387625 [details]
Updated

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2035
> +    return WebCore::indexForVisiblePosition(*node, position, { TextIteratorLegnthOption::GenerateSpacesForReplacedElements });

Typo here: "legnth".

> Source/WebCore/editing/TextIteratorBehavior.h:64
>  typedef unsigned short TextIteratorBehavior;

Should move this one to OptionSet too later, right?

[Ryosuke Niwa]

Created attachment 387747 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 387747 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #10)
> Comment on attachment 387625 [details]
> Updated
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387625&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2035
> > +    return WebCore::indexForVisiblePosition(*node, position, { TextIteratorLegnthOption::GenerateSpacesForReplacedElements });
> 
> Typo here: "legnth".

Fixed.

> > Source/WebCore/editing/TextIteratorBehavior.h:64
> >  typedef unsigned short TextIteratorBehavior;
> 
> Should move this one to OptionSet too later, right?

Yes, but in a separate patch.

[Ryosuke Niwa]

Created attachment 387751 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 387751 [details]
Patch for landing

Wait for EWS again.

[Ryosuke Niwa]

Committed r254557: <https://trac.webkit.org/changeset/254557>

[Radar WebKit Bug Importer]

<rdar://problem/58595369>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 207725