Bug 107171 – shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom should return false on Android

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Chris Hopman]

Add Android-specific EditingBehavior

[Chris Hopman]

Created attachment 183266 [details]
Patch

[Chris Hopman]

Created attachment 183268 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Levi Weintraub]

Comment on attachment 183268 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:13
> +        No new tests (OOPS!).

You'll have to change this. You can't lande with "OOPS!" in the ChangeLog. Ideally you should add a test that verifies this behavior, but I'd be content with an explanation of why you're not adding one.

[Eric Seidel (no email)]

Android was adding some custom code to move the cursor.  Is that no longer needed after thsi? or is this just tweaking other editing behaviors in webkit to be more android-like?

[Ryosuke Niwa]

Comment on attachment 183268 [details]
Patch

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

I'm sorry but I have to r- this.

> Source/WebCore/ChangeLog:11
> +        On Android, EditingBehavior::shouldMoveCaret[...] controls the
> +        behavior of insertion handles. This change adds a new Android specific
> +        editing behavior type so that we can change these settings independent
> +        of behavior for other platforms.

I don't think adding a new editing behavior makes sense here. r-.
The only reason we decided to have these editing behaviors is so that we can test these behaviors in layout tests.
Given that we're not doing it, ifdef will suffice.

> Source/WebCore/editing/EditingBehavior.h:71
> -        return m_type == EditingUnixBehavior;
> +        return m_type == EditingUnixBehavior || m_type == EditingAndroidBehavior;

This code is never used in Android port.

[Levi Weintraub]

Comment on attachment 183268 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        of behavior for other platforms.
> 
> I don't think adding a new editing behavior makes sense here. r-.
> The only reason we decided to have these editing behaviors is so that we can test these behaviors in layout tests.
> Given that we're not doing it, ifdef will suffice.

I don't agree that encapsulating these classes of behavior in an enum makes less sense than adding ifdefs for them when the enum exists. I'd be happier covering this in layout tests than needing to disable/add specific expectations for Android, which would occur if just #ifdefing EditingUnixBehavior.

[Ryosuke Niwa]

Comment on attachment 183268 [details]
Patch

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

> Source/WebCore/editing/EditingBehavior.h:44
> +        return m_type != EditingWindowsBehavior && m_type != EditingAndroidBehavior;

This patch doesn't just add an editing behavior but changes the behavior of WebCore on Android port.
As such, it needs a test.

[Ryosuke Niwa]

(In reply to comment #7)
> (From update of attachment 183268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183268&action=review
> 
> >> Source/WebCore/ChangeLog:11
> >> +        of behavior for other platforms.
> > 
> > I don't think adding a new editing behavior makes sense here. r-.
> > The only reason we decided to have these editing behaviors is so that we can test these behaviors in layout tests.
> > Given that we're not doing it, ifdef will suffice.
> 
> I don't agree that encapsulating these classes of behavior in an enum makes less sense than adding ifdefs for them when the enum exists. I'd be happier covering this in layout tests than needing to disable/add specific expectations for Android, which would occur if just #ifdefing EditingUnixBehavior.

We already have an if-def for chromium. If we're adding a new editing behavior, then all existing layout tests need to be updated to test "android" editing behaviors as well as exiting ones.

[Ryosuke Niwa]

1. I would like to know why this behavior is desirable.
2. I would like to see a test that demonstrates the bug.
3. If we're adding a new editing behavior, then existing tests in LayoutTests/editing should be updated to test "android" editing behavior as well although I strongly advise against adding a new editing behavior in the first place.

The criteria for whether we should add "android" editing behavior for this bug is whether such a behavior is platform de-facto standard and every sane Android browser should exhibit the same behavior or not. If not, i.e. this behavior is specific to Chromium Android port, then adding a new editing behavior is not the right solution.

[Levi Weintraub]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 183268 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=183268&action=review
> > 
> > >> Source/WebCore/ChangeLog:11
> > >> +        of behavior for other platforms.
> > > 
> > > I don't think adding a new editing behavior makes sense here. r-.
> > > The only reason we decided to have these editing behaviors is so that we can test these behaviors in layout tests.
> > > Given that we're not doing it, ifdef will suffice.
> > 
> > I don't agree that encapsulating these classes of behavior in an enum makes less sense than adding ifdefs for them when the enum exists. I'd be happier covering this in layout tests than needing to disable/add specific expectations for Android, which would occur if just #ifdefing EditingUnixBehavior.
> 
> We already have an if-def for chromium. If we're adding a new editing behavior, then all existing layout tests need to be updated to test "android" editing behaviors as well as exiting ones.

The existing #ifdef seems like more of a design choice by Chromium than an editing behavior related to a platform. It seems to me that mobile does represent a new class of editing behavior.

[Ryosuke Niwa]

(In reply to comment #11)
> The existing #ifdef seems like more of a design choice by Chromium than an editing behavior related to a platform. It seems to me that mobile does represent a new class of editing behavior.

Right. So you're saying that this is not a design choose of Chromium port, and it's a platform standard? If so, could someone point to me to an application that exhibit this behavior?

[Chris Hopman]

Comment on attachment 183268 [details]
Patch

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

>>>>> Source/WebCore/ChangeLog:11
>>>>> +        of behavior for other platforms.
>>>> 
>>>> I don't think adding a new editing behavior makes sense here. r-.
>>>> The only reason we decided to have these editing behaviors is so that we can test these behaviors in layout tests.
>>>> Given that we're not doing it, ifdef will suffice.
>>> 
>>> I don't agree that encapsulating these classes of behavior in an enum makes less sense than adding ifdefs for them when the enum exists. I'd be happier covering this in layout tests than needing to disable/add specific expectations for Android, which would occur if just #ifdefing EditingUnixBehavior.
>> 
>> We already have an if-def for chromium. If we're adding a new editing behavior, then all existing layout tests need to be updated to test "android" editing behaviors as well as exiting ones.
> 
> The existing #ifdef seems like more of a design choice by Chromium than an editing behavior related to a platform. It seems to me that mobile does represent a new class of editing behavior.

rniwa: Are you suggesting adding something like: "#if PLATFORM(CHROMIUM) && defined(ANDROID) \ return false; \ #endif" to EditingBehavior::shouldMove[...]? That seems reasonable to me.

I think it would probably be best if these settings could be set individually (rather than as a group based on the existing EditingBehaviorTypes) by an embedder like all of WebCore::Settings, then we wouldn't need to stick any CHROMIUM stuff in here. What do you think?

[Ryosuke Niwa]

(In reply to comment #13)
> (From update of attachment 183268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183268&action=review
>
> I think it would probably be best if these settings could be set individually (rather than as a group based on the existing EditingBehaviorTypes) by an embedder like all of WebCore::Settings, then we wouldn't need to stick any CHROMIUM stuff in here. What do you think?

The whole point of editing behavior is so that we don't have to add a new setting for every method we have in EditingBehavior.h, and each port doesn't need to figure things on their own.

I would much prefer ifdef solution over adding new settings.

[Levi Weintraub]

The ifdef solution seems a fine path for this. I agree with rniwa that complicating the test surface here would be a big disadvantage of adding an Android behavior. It does seem flawed to me that we have 3 editing behavior modes, but none of them really seem to fit mobile.

[Ryosuke Niwa]

(In reply to comment #15)
> The ifdef solution seems a fine path for this. I agree with rniwa that complicating the test surface here would be a big disadvantage of adding an Android behavior. It does seem flawed to me that we have 3 editing behavior modes, but none of them really seem to fit mobile.

I'm fine with adding a new editing behavior for mobile in general or Android (I'd expect Android and iOS have very different editing behavior though) but only if that's not specific to a particular browser.

But if we're doing that, then we need to make sure all tests that explicitly set editing behaviors need to be updated to test the new value "android".

Regardless, this bug really needs a reduced test case.

[Chris Hopman]

Created attachment 183554 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 183554 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        * editing/selection/script-tests/test-if-should-move-caret-to-horizontal-boundary-forced-false.js: Added.
> +        (checkSelection):
> +        (clickShouldResultInRange):
> +        * editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false-expected.txt: Added.
> +        * editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false.html: Added.

Please don't add a separate js file for the script tests. That's the old way. The new way is to put the script in the single html file.
Also, this test will have a different result on Windows as well as Chromium Windows so you'll need to add test expectations there as well.

> LayoutTests/editing/selection/script-tests/test-if-should-move-caret-to-horizontal-boundary-forced-false.js:40
> +function clickShouldResultInRange(x, y, offsetIfNormal, offsetIfForced) {
> +    if (window.eventSender) {
> +        clickAt(x, y);
> +        checkSelection(offsetIfNormal, offsetIfForced);
> +    }
> +}

A more idiomatic way of writing a test case like this is to add a helper function to verify things and call both of these functions inside assertEqual. e.g.
assertEqual("clickAt(pointImmediatelyAfterText); locationOfCaretInTextNode()", "0");

But this isn't really great because the output depends on the platform.
You might want your test to look something like:
evalAndLog(" clickAt(pointImmediatelyAfterText);");
debug("Caret location:" + caretLocation());

> LayoutTests/editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false-expected.txt:10
> +PASS EditingBehavior::shouldMove[...] not forced to false. Selection is [anchorNode: [object Text](XX) anchorOffset: 0 focusNode: [object Text](XX) focusOffset: 0 isCollapsed: true]
> +PASS EditingBehavior::shouldMove[...] not forced to false. Selection is [anchorNode: [object Text](XX) anchorOffset: 0 focusNode: [object Text](XX) focusOffset: 0 isCollapsed: true]
> +PASS EditingBehavior::shouldMove[...] not forced to false. Selection is [anchorNode: [object Text](XX) anchorOffset: 2 focusNode: [object Text](XX) focusOffset: 2 isCollapsed: true]
> +PASS EditingBehavior::shouldMove[...] not forced to false. Selection is [anchorNode: [object Text](XX) anchorOffset: 2 focusNode: [object Text](XX) focusOffset: 2 isCollapsed: true]
> +PASS successfullyParsed is true

Looking at this output, I can't tell what it's testing and why those outputs are correct.
The test output should answer questions.
Also, it's not clear to me using testPassed/testFailed is the right thing to do in this test given that the output depends on the platform.

> LayoutTests/platform/chromium-android/TestExpectations:114
> +# On Android, EditingBehavior::shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom()
> +# always returns false. That breaks these tests.
> +# https://bugs.webkit.org/show_bug.cgi?id=105574
> +# This may be revisited after http://crbug.com/135959 is finished.
> +editing/selection/click-in-margins-inside-editable-div.html [ WontFix ]
> +editing/selection/click-in-padding-with-multiple-line-boxes.html [ WontFix ]
> +fast/writing-mode/flipped-blocks-hit-test-line-edges.html [ WontFix ]

It doesn't seem like a good idea to regress these tests. If anything the said chromium issue should be fixed first.

[Chris Hopman]

Comment on attachment 183554 [details]
Patch

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

>> LayoutTests/ChangeLog:12
>> +        * editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false.html: Added.
> 
> Please don't add a separate js file for the script tests. That's the old way. The new way is to put the script in the single html file.
> Also, this test will have a different result on Windows as well as Chromium Windows so you'll need to add test expectations there as well.

Sure, I'll combine them. I don't think this test has different results on Windows - see below.

> LayoutTests/editing/selection/script-tests/test-if-should-move-caret-to-horizontal-boundary-forced-false.js:43
> +    internals.settings.setEditingBehavior("mac");

My understanding is that this call will make the test use the mac editing behavior.
I.e. the test behavior will be the same for all platforms except for chromium-android (which forces at compile time that EditingBehavior::shouldMoveCaret[...] returns false regardless of what editing behavior is set).

>> LayoutTests/platform/chromium-android/TestExpectations:114
>> +fast/writing-mode/flipped-blocks-hit-test-line-edges.html [ WontFix ]
> 
> It doesn't seem like a good idea to regress these tests. If anything the said chromium issue should be fixed first.

These tests use internals.settings.setEditingBehavior("somePlatform") and then depend on EditingBehavior::shouldMoveCaret[...] to behave according to that platform's editing behavior. This is not the case if it is #ifdeffed for chromium-android to always return false. So I see 3 options:

1. Rebaseline the tests for the new behavior. I didn't like this because these tests use testPassed and testFailed and the new behavior has a lot of FAILs (i.e. for all the parts of the test that assume mac or linux editing behavior). That makes the expected behavior look wrong and I don't know of a good way to notify someone looking at that new expected behavior that the FAILs are actually "PASSes".

2. Change the test to detect that it's actually on chromium-android and then understand that they won't get mac or linux behavior for shouldMoveCaret[...] and change all their checks accordingly. With this approach, chromium-android would still need new baselines for these tests, but the expected behavior wouldn't look like it was wrong. However, I don't really like the idea of such an invasive change to the tests just to support this odd chromium-android behavior.

3. Let the tests fail and mark them WontFix. This is clean and we can have a notification at the same point saying what's wrong with the tests and when they should be fixed. However, it's major failing is that chromium-android loses the coverage of these tests.

I did (1) and was working on (2) when I decided to go with (3). You know better than me, though, so if you'd prefer one of the other 2 options (or something else I didn't list) that'll be fine with me.

The mentioned chromium issue is a rather long term goal.

[Ryosuke Niwa]

(In reply to comment #19)
> (From update of attachment 183554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183554&action=review
>
> >> LayoutTests/platform/chromium-android/TestExpectations:114
> >> +fast/writing-mode/flipped-blocks-hit-test-line-edges.html [ WontFix ]
> > 
> > It doesn't seem like a good idea to regress these tests. If anything the said chromium issue should be fixed first.
> 
> These tests use internals.settings.setEditingBehavior("somePlatform") and then depend on EditingBehavior::shouldMoveCaret[...] to behave according to that platform's editing behavior. This is not the case if it is #ifdeffed for chromium-android to always return false. So I see 3 options:
> 
> 1. Rebaseline the tests for the new behavior. I didn't like this because these tests use testPassed and testFailed and the new behavior has a lot of FAILs (i.e. for all the parts of the test that assume mac or linux editing behavior). That makes the expected behavior look wrong and I don't know of a good way to notify someone looking at that new expected behavior that the FAILs are actually "PASSes".
> 
> 2. Change the test to detect that it's actually on chromium-android and then understand that they won't get mac or linux behavior for shouldMoveCaret[...] and change all their checks accordingly. With this approach, chromium-android would still need new baselines for these tests, but the expected behavior wouldn't look like it was wrong. However, I don't really like the idea of such an invasive change to the tests just to support this odd chromium-android behavior.
> 
> 3. Let the tests fail and mark them WontFix. This is clean and we can have a notification at the same point saying what's wrong with the tests and when they should be fixed. However, it's major failing is that chromium-android loses the coverage of these tests.
> 
> I did (1) and was working on (2) when I decided to go with (3). You know better than me, though, so if you'd prefer one of the other 2 options (or something else I didn't list) that'll be fine with me.

Option 1 is what we do in WebKit. When you land thrse changes, just mention that FAILs are expected.

[Chris Hopman]

Created attachment 184024 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 184024 [details]
Patch

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

> LayoutTests/editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false-expected.txt:3
> +clickAt(782, 94)

Doesn't this coordinate depend on how the text above wrap? You should probably hide the description text during the test so that it won't affect the clicked coordinate and make it platform dependent.

> LayoutTests/editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false.html:32
> +    var sel = window.getSelection();

Please don't use an abbreviation like sel. Spell out selection.
Also, you don't need window. Simply call getSelection() as in:
var anchorNode = getSelection().anchorNode;
return anchorNode + ' ' + anchorNode.data + ' ' + getSelection().anchorOffset;

You should also check that the resultant selection is collapsed. i.e. getSelection().isCollapsed is true.

[Chris Hopman]

Created attachment 184048 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 184048 [details]
Patch

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

> LayoutTests/editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false-expected.txt:4
> +Caret location: [object Text] XX 0 true

I can't tell what "true" means here. In general, the test output should be self explanatory such that
someone who has never looked at the test should be able to tell what the test is testing and whether it's passing or failing and why.

[Chris Hopman]

Comment on attachment 184048 [details]
Patch

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

>> LayoutTests/editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false-expected.txt:4
>> +Caret location: [object Text] XX 0 true
> 
> I can't tell what "true" means here. In general, the test output should be self explanatory such that
> someone who has never looked at the test should be able to tell what the test is testing and whether it's passing or failing and why.

Ok, how about just not outputting isCollapsed? This test doesn't actually care about it, and if it's ever false then there is certainly something more fundamental wrong with selection.

But, on that note, do you think I should label these other values (anchorNode, anchorNode.data, anchorOffset)?

[Ryosuke Niwa]

(In reply to comment #25)
> (From update of attachment 184048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184048&action=review
> 
> >> LayoutTests/editing/selection/test-if-should-move-caret-to-horizontal-boundary-forced-false-expected.txt:4
> >> +Caret location: [object Text] XX 0 true
> > 
> > I can't tell what "true" means here. In general, the test output should be self explanatory such that
> > someone who has never looked at the test should be able to tell what the test is testing and whether it's passing or failing and why.
> 
> Ok, how about just not outputting isCollapsed? This test doesn't actually care about it, and if it's ever false then there is certainly something more fundamental wrong with selection.

We do care about that because if isCollapsed is false, then we're getting a non-collapsed selection, which is clearly not intended.

> But, on that note, do you think I should label these other values (anchorNode, anchorNode.data, anchorOffset)?

Probably not.

[Darin Adler]

Comment on attachment 184048 [details]
Patch

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

This is not

> Source/WebCore/editing/EditingBehavior.h:49
> -    bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return m_type != EditingWindowsBehavior; }
> +    bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const
> +    {
> +#if PLATFORM(CHROMIUM) && defined(ANDROID)
> +        return false;
> +#else
> +        return m_type != EditingWindowsBehavior;
> +#endif
> +    }

This is not the way to fix this. It’s really bad to be putting #if statements in this header. The whole idea here was to define editing behavior types instead of having a pile of ifdefs. I am extremely unhappy to see shouldAllowSpellingSuggestionsWithoutSelection special cased for Chromium; too bad I was not able to stop this then when it first happened. The right way to do this is to add more editing behavior types, not add #if statements.

[Ryosuke Niwa]

(In reply to comment #27)
> (From update of attachment 184048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184048&action=review
> 
> This is not
> 
> > Source/WebCore/editing/EditingBehavior.h:49
> > -    bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return m_type != EditingWindowsBehavior; }
> > +    bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const
> > +    {
> > +#if PLATFORM(CHROMIUM) && defined(ANDROID)
> > +        return false;
> > +#else
> > +        return m_type != EditingWindowsBehavior;
> > +#endif
> > +    }
> 
> This is not the way to fix this. It’s really bad to be putting #if statements in this header. The whole idea here was to define editing behavior types instead of having a pile of ifdefs. I am extremely unhappy to see shouldAllowSpellingSuggestionsWithoutSelection special cased for Chromium; too bad I was not able to stop this then when it first happened. The right way to do this is to add more editing behavior types, not add #if statements.

I don't think that makes sense in this particular case because this behavior is specific to Chromium port. Namely, I don't think we want to be adding EditingChromiumWindowsBehavior, EditingChromiumMacBehavior, EditingChromiumLinuxBehavior, and EditingChromiumAndroidBehavior in addition to ones we already have.

[Chris Hopman]

(In reply to comment #27)
> (From update of attachment 184048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184048&action=review
> 
> This is not
> 
> > Source/WebCore/editing/EditingBehavior.h:49
> > -    bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return m_type != EditingWindowsBehavior; }
> > +    bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const
> > +    {
> > +#if PLATFORM(CHROMIUM) && defined(ANDROID)
> > +        return false;
> > +#else
> > +        return m_type != EditingWindowsBehavior;
> > +#endif
> > +    }
> 
> This is not the way to fix this. It’s really bad to be putting #if statements in this header. The whole idea here was to define editing behavior types instead of having a pile of ifdefs. I am extremely unhappy to see shouldAllowSpellingSuggestionsWithoutSelection special cased for Chromium; too bad I was not able to stop this then when it first happened. The right way to do this is to add more editing behavior types, not add #if statements.

Ok, so here's the problem:
chromium-android needs EditingBehavior::shouldMoveCaret[...] to return true
chromium-linux needs EditingBehavior::shouldAllowSpelling[...] to return false
both of these otherwise want EditingUnixBehavior.

I would really prefer if these two issues (and future cases where a specific port needs changes to EditingBehavior) were handled in a consistent way. Here are my thoughts on some possible approaches.

The approach that has been taken in the past (i.e. for chromium-linux) is to add an ifdef in EditingBehavior.h. darin@ argues that that is wrong, we should not be adding #ifs here. I think that's a reasonable stance.

We could introduce a new EditingBehaviorType each time we need behavior that diverges from the current types (i.e. we could add EditingChromiumLinuxBehavior and EditingChromiumAndroidBehavior), but I agree with rniwa@ that that is not a very good solution.

We could require that every port use one of a small set of bundles of behavior (i.e. 3?). We already have 2 ports that want unique behavior, and I don't think this approach is practical.

Every time that a port wants behavior for one of these functions that differs from the platform-default, we could move that function to WebCore::Settings and allow the port to set the behavior to whatever it wants. This weakens the benefit of bundling these together as platform editing behaviors.

The approach that I would prefer/think is best would be to modify EditingBehavior such that each of the behaviors can be set individually by an embedder (like WebCore::Settings). We could keep the EditingBehaviorTypes and have a constructor (or setter) that takes an EditingBehaviorType and sets all of the behaviors based on that EditingBehaviorType. Then if a particular port (chromium-linux or chromium-android, for example) wanted different behavior for a single function, they could initialize the EditingBehavior based on a platform and then change only those (few) functions/behaviors that they wanted.

You guys know better than I do what approach should be taken, so I'm happy to defer the decision to you, I just thought I'd get some ideas out there.

[Ryosuke Niwa]

(In reply to comment #29)
> Ok, so here's the problem:
> chromium-android needs EditingBehavior::shouldMoveCaret[...] to return true
> chromium-linux needs EditingBehavior::shouldAllowSpelling[...] to return false
> both of these otherwise want EditingUnixBehavior.

First off, it's unclear to me why Chromium Linux wants to return false in shouldAllowSpellingSuggestionsWithoutSelection. As far as I can tell, nobody from Chromium port voiced such need: https://bugs.webkit.org/show_bug.cgi?id=103520

We need to see if we can make Chromium Linux behave like the rest of Linux ports.

> The approach that I would prefer/think is best would be to modify EditingBehavior such that each of the behaviors can be set individually by an embedder (like WebCore::Settings). We could keep the EditingBehaviorTypes and have a constructor (or setter) that takes an EditingBehaviorType and sets all of the behaviors based on that EditingBehaviorType. Then if a particular port (chromium-linux or chromium-android, for example) wanted different behavior for a single function, they could initialize the EditingBehavior based on a platform and then change only those (few) functions/behaviors that they wanted.

The whole point of editing behavior was so that we didn't have to add a setting for each new editing behavior we want. Given that, I don't think making everything configurable by each port appears to defeat the point of having this class. We might as well as use settings at that point.

[Peter Beverloo]

Resolving as WontFix given that Chromium moved to Blink.