Bug 136876 – Should have an editing behavior specific for IOS

• Status: NEW
• Component: WebCore Misc.

[Enrica Casucci]

In order to avoid using PLATFORM guards to have different editing behaviors on iOS we should have an editing behavior that is specific to iOS.

[Enrica Casucci]

Created attachment 238228 [details]
Patch

[WebKit Commit Bot]

Attachment 238228 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/DeleteSelectionCommand.cpp:868:  Consider using toText helper function in WebCore/dom/Text.h instead of static_cast<Text*>  [readability/check] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 238228 [details]
Patch

Looks good! Now that we have this as a behavior, can we add a test for it.

[Ryosuke Niwa]

Comment on attachment 238228 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:619
> -    if (m_frame && m_frame->settings().editingBehaviorType() == EditingMacBehavior)
> +    if (m_frame && (m_frame->settings().editingBehaviorType() == EditingMacBehavior || m_frame->settings().editingBehaviorType() == EditingIOSBehavior))

We should extract this into a helper function in EditingBehavior.h, not that you should do it in this patch.

> Source/WebCore/testing/InternalSettings.cpp:353
> +    else if (equalIgnoringCase(editingBehavior, "ios"))
> +        settings()->setEditingBehaviorType(EditingIOSBehavior);

Are you gonna update existing tests in a follow up patch?

[Enrica Casucci]

> We should extract this into a helper function in EditingBehavior.h, not that you should do it in this patch.
> 
Agreed. This is the only place where we check the EditingBehaviorType directly.

> > Source/WebCore/testing/InternalSettings.cpp:353
> > +    else if (equalIgnoringCase(editingBehavior, "ios"))
> > +        settings()->setEditingBehaviorType(EditingIOSBehavior);
> 
> Are you gonna update existing tests in a follow up patch?
All the tests currently set the editing behavior as Mac and they should all continue to work.
We don't have any test that currently test the iOS behavior, but this change will allow us to have some.

[Sam Weinig]

(In reply to comment #5)
> > We should extract this into a helper function in EditingBehavior.h, not that you should do it in this patch.
> > 
> Agreed. This is the only place where we check the EditingBehaviorType directly.
> 
> > > Source/WebCore/testing/InternalSettings.cpp:353
> > > +    else if (equalIgnoringCase(editingBehavior, "ios"))
> > > +        settings()->setEditingBehaviorType(EditingIOSBehavior);
> > 
> > Are you gonna update existing tests in a follow up patch?
> All the tests currently set the editing behavior as Mac and they should all continue to work.
> We don't have any test that currently test the iOS behavior, but this change will allow us to have some.

Hm, I probably should have had you break this up into two patches then, one to add the ability to test the iOS behavior, and one that actually changed the code and added a test.  Please do add a test though.