[Webkit-unassigned] [Bug 110487] Ctrl+Shift+Right in Windows should select the spacing after the word

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=110487





--- Comment #39 from Claudio Saavedra <csaavedra at igalia.com>  2013-03-05 22:44:49 PST ---
Thanks for your comments!

(In reply to comment #38)
> (From update of attachment 190658 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review
> 
> I think that Ryosuke's request is not to implement Windows behavior using NSString APIs, but to make sure that editing behavior is not determined at compile time.

Editing behavior is not determined at compile-time. I'll explain why there are ifdef's more carefully: the way the next stop is determined, in all platforms, is through a call to the findNextWordFromIndex() method. What my patch is doing is to add a boolean parameter to the signature of this method, so that it looks like this:

  findNextWordFromIndex(UChar const* buffer, int len, int position, bool forward, bool skipSpaces);

This way, the calling methods can decide whether to request from this method to skip spaces when searching for the next word. Calling methods, of course, rely on the run-time property EditingBehavior. As you can see, the editing behavior is never determined at run-time.

As to the ifdefs: This method has different implementations: a generic one, one for the Qt port (using QString), and one for the Mac one (using NSString). My patch modifies both the generic implementation and the one for the Qt port. To make the patch complete one would need to modify the Mac implementation as well so that it works as we now need. Admittedly, since I am not familiar with NSString at all I've left this out. I am open to suggestions as to how to complete this patch so that findNextWordFromIndex() in Mac works in the same way and the ifdefs are not necessary.


> 
> We want to test all platforms behavior in regression tests, using a window.internals switch to choose editing behavior in each test. This way, running tests locally would be a better indication that a patch is correct.
> 
> Since the patch still does not have tests, it's automatically r-.

I will add tests soon, of course. However, at this point I am mostly concerned in what we're discussing above.

> 
> > Source/WTF/wtf/unicode/icu/UnicodeIcu.h:179
> > +inline bool isSpace(UChar32 c)
> > +{
> > +    return !!u_isWhitespace(c);
> > +}
> 
> This is not a good name for this function (isWhitespace is not so great either, but it's OK in ICU context).
> 
> There are multiple definitions of whitespace used in the Web platform. They may or may not include Unicode spaces, form feeds etc.

Do you have any suggestion? Maybe just isWhitespace() or isSpacing()?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.