[Webkit-unassigned] [Bug 107171] shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom should return false on Android
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 23 16:04:09 PST 2013
https://bugs.webkit.org/show_bug.cgi?id=107171
--- Comment #29 from Chris Hopman <cjhopman at chromium.org> 2013-01-23 16:06:01 PST ---
(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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list