Bug 54969 - [Chromium] Implement WebKit methods to assist with Cocoa NSTextInput implementation
Summary: [Chromium] Implement WebKit methods to assist with Cocoa NSTextInput implemen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://code.google.com/p/chromium/iss...
Keywords:
Depends on: 59558
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-22 09:29 PST by Robert Sesek
Modified: 2011-04-26 16:45 PDT (History)
12 users (show)

See Also:


Attachments
Patch (23.21 KB, patch)
2011-02-25 09:58 PST, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch (33.17 KB, patch)
2011-03-22 09:19 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch (38.33 KB, patch)
2011-03-22 14:46 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch (38.77 KB, patch)
2011-03-24 14:27 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch (40.69 KB, patch)
2011-03-25 13:11 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch v6 WebCore (20.22 KB, patch)
2011-04-06 08:53 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch v6 Chromium (20.78 KB, patch)
2011-04-06 09:02 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch v7 WebCore (20.29 KB, patch)
2011-04-06 10:09 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch v7 Chromium (20.64 KB, patch)
2011-04-11 14:09 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Patch v7.1 Chromium (20.69 KB, patch)
2011-04-26 09:06 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Sesek 2011-02-22 09:29:36 PST
In order to enable the Mac OS X system dictionary popup, Chromium's WebKit API needs to support retrieving the necessary information used to implement the NSTextInput protocol (firstRectForRange, characterIndexForPoint, substringInRange). I have a patch to make these changes to the API.
Comment 1 Robert Sesek 2011-02-25 09:58:41 PST
Created attachment 83828 [details]
Patch
Comment 2 Robert Sesek 2011-02-25 10:00:53 PST
Darin, do you have time to review these changes to the Chromium WebKit API? If not, can you suggest a reviewer?
Comment 3 WebKit Review Bot 2011-02-25 10:05:06 PST
Attachment 83828 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8043342
Comment 4 Dimitri Glazkov (Google) 2011-02-28 15:07:20 PST
Comment on attachment 83828 [details]
Patch

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

Pls fix EWS breakage and help to unduplicate.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2298
> +// This function is copied from /WebKit2/WebPage/mac/WebPageMac.mm.

Sounds like this should just live in WebCore. There's nothing WebKitty here. Can you move and unduplicate?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2325
> +bool WebFrameImpl::getLocationAndLengthFromRange(Range* range, unsigned& location, unsigned& length) const

Ditto.

> Source/WebKit/chromium/src/mac/WebTextHelper.mm:74
> +// This function is copied from /WebKit/mac/Misc/WebNSAttributedStringExtras.mm.

Can we just compile WebNSAttributedStringExtras.mm?
Comment 5 Dimitri Glazkov (Google) 2011-03-01 08:49:08 PST
(In reply to comment #4)
> (From update of attachment 83828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83828&action=review
> 
> Pls fix EWS breakage and help to unduplicate.
> 
> > Source/WebKit/chromium/src/WebFrameImpl.cpp:2298
> > +// This function is copied from /WebKit2/WebPage/mac/WebPageMac.mm.
> 
> Sounds like this should just live in WebCore. There's nothing WebKitty here. Can you move and unduplicate?

These could probably go to Source/WebCore/platform/text/mac?

Sam, Enrica do you guys have any opinions?

> 
> > Source/WebKit/chromium/src/WebFrameImpl.cpp:2325
> > +bool WebFrameImpl::getLocationAndLengthFromRange(Range* range, unsigned& location, unsigned& length) const
> 
> Ditto.
> 
> > Source/WebKit/chromium/src/mac/WebTextHelper.mm:74
> > +// This function is copied from /WebKit/mac/Misc/WebNSAttributedStringExtras.mm.
> 
> Can we just compile WebNSAttributedStringExtras.mm?
Comment 6 Robert Sesek 2011-03-01 12:16:21 PST
Comment on attachment 83828 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2298
>>> +// This function is copied from /WebKit2/WebPage/mac/WebPageMac.mm.
>> 
>> Sounds like this should just live in WebCore. There's nothing WebKitty here. Can you move and unduplicate?
> 
> These could probably go to Source/WebCore/platform/text/mac?
> 
> Sam, Enrica do you guys have any opinions?

I've moved this function to be Frame::rangeForPoint(), which fits well with documentAtPoint() and visiblePositionForPoint(). Other platforms may need this for their text input systems.

>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2325

>> 
>> Ditto.
> 
> 

Moved this to Range::getLocationAndLength().

>> Source/WebKit/chromium/src/mac/WebTextHelper.mm:74
>> +// This function is copied from /WebKit/mac/Misc/WebNSAttributedStringExtras.mm.
> 
> Can we just compile WebNSAttributedStringExtras.mm?

We can't use it directly because WebKit/mac uses <WebCore/x.h> for #include and WebKit/chromium uses "x.h". Could I move WebNSAttributedStringExtras.{h,mm} wholesale into WebCore/platform/text/mac?
Comment 7 Robert Sesek 2011-03-22 09:19:53 PDT
Created attachment 86464 [details]
Patch
Comment 8 Robert Sesek 2011-03-22 09:23:43 PDT
(In reply to comment #6)
> We can't use it directly because WebKit/mac uses <WebCore/x.h> for #include and WebKit/chromium uses "x.h". Could I move WebNSAttributedStringExtras.{h,mm} wholesale into WebCore/platform/text/mac?

It turns out this isn't that easy, either. The version of this function in WebKit/mac encodes images as "attachments" on the attributed string, but doing so relies on WebKit/mac's resource loading and cache implementation. In WebKit/chromium, we (1) don't need these attachments and (2) have a different resource loader. This function may have to remain forked.
Comment 9 Early Warning System Bot 2011-03-22 09:32:01 PDT
Attachment 86464 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8221385
Comment 10 Build Bot 2011-03-22 10:16:18 PDT
Attachment 86464 [details] did not build on win:
Build output: http://queues.webkit.org/results/8224304
Comment 11 Alexey Proskuryakov 2011-03-22 12:01:15 PDT
It seems very strange that implementing something that already exists in another port requires adding code to WebCore.
Comment 12 Robert Sesek 2011-03-22 12:12:57 PDT
(In reply to comment #11)
> It seems very strange that implementing something that already exists in another port requires adding code to WebCore.

Dimitri suggested that I should push the shared code up into WebCore. It seems more strange to introduce hard dependencies between ports. Is there a specific method or function to which you are referring?
Comment 13 Alexey Proskuryakov 2011-03-22 12:36:44 PDT
I see that there was a little code removed from WebKit2, but nothing from WebKit/mac.
Comment 14 Robert Sesek 2011-03-22 14:46:14 PDT
Created attachment 86512 [details]
Patch
Comment 15 Robert Sesek 2011-03-22 14:47:03 PDT
(In reply to comment #13)
> I see that there was a little code removed from WebKit2, but nothing from WebKit/mac.

Thanks, I found some more code to de-dupe in WebKit/mac. The only remaining duplicated bits are from WebNSAttributedStringExtras.mm function, which I addressed in comment #8.
Comment 16 Dimitri Glazkov (Google) 2011-03-24 12:25:06 PDT
Comment on attachment 86512 [details]
Patch

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

The gist looks great to me. You've done well whittling down redudancies! I'd like two peeps to take a look at this before landing: rniwa (ranges/editing stuff) and weinig (webkit2 stuff). r- to nit.

> Source/WebCore/ChangeLog:16
> +        * WebCore.exp.in:
> +        * dom/Range.cpp:
> +        (WebCore::Range::getLocationAndLength):
> +        * dom/Range.h:
> +        * page/Frame.cpp:
> +        (WebCore::Frame::rangeForPoint):
> +        * page/Frame.h:

This is sad and lonely. I'd like it to be full of life and meaning.

> Source/WebKit/chromium/ChangeLog:28
> +        * WebKit.gyp:
> +        * public/WebFrame.h:
> +        * public/WebWidget.h:
> +        * public/mac/WebTextHelper.h: Added.
> +        * src/WebFrameImpl.cpp:
> +        (WebKit::WebFrameImpl::firstRectForCharacterRange):
> +        (WebKit::WebFrameImpl::characterIndexForPoint):
> +        * src/WebFrameImpl.h:
> +        * src/WebPopupMenuImpl.cpp:
> +        (WebKit::WebPopupMenuImpl::compositionRange):
> +        (WebKit::WebPopupMenuImpl::caretOrSelectionRange):
> +        * src/WebPopupMenuImpl.h:
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::compositionRange):
> +        (WebKit::WebViewImpl::caretOrSelectionRange):
> +        * src/WebViewImpl.h:
> +        * src/mac/WebTextHelper.mm: Added.
> +        (WebKit::getWebFrameImpl):
> +        (WebKit::rangeScope):
> +        (WebKit::WebTextHelper::WebTextHelper):
> +        (WebKit::WebTextHelper::substringInRange):

Ditto. At least explain what you're doing.
Comment 17 Robert Sesek 2011-03-24 14:27:57 PDT
Created attachment 86837 [details]
Patch
Comment 18 Alexey Proskuryakov 2011-03-24 14:53:42 PDT
Moving code down to WebCore is a good idea. It might be better to split chromium parts into a separate patch.

It seems strange to have code that knows about event handling in Range. For example, why Range::getLocationAndLength() knows anything about mouse events and TSM?! Also, this function asks TextIterator about length, but TextIterator works largely with render tree.
Comment 19 Robert Sesek 2011-03-25 13:11:57 PDT
Created attachment 86977 [details]
Patch
Comment 20 Robert Sesek 2011-03-25 13:21:29 PDT
(In reply to comment #18)
> Moving code down to WebCore is a good idea. It might be better to split chromium parts into a separate patch.

Ok. I'll split out the Chromium part when this review gets closer to completion. For now, while things are up in the air, it's easier for me to keep it in one patch/on a single git branch.

> It seems strange to have code that knows about event handling in Range. For example, why Range::getLocationAndLength() knows anything about mouse events and TSM?! Also, this function asks TextIterator about length, but TextIterator works largely with render tree.

Since what the clients of this function really want is the location/length from TextIterator, I decided to move Range::getLocationAndLength() to be a static function TextIterator::locationAndLengthFromRange(), which complements TextIterator::rangeFromLocationAndLength().

The comment about TSM was from the WebKit2 implementation. What that bit of code is really doing is protecting against spanning across the DOMs from text fields/textareas and the actual document. I've updated the comment to reflect that. Is this appropriate? If not, I would appreciate any advice on other potential solutions.
Comment 21 Alexey Proskuryakov 2011-03-31 13:46:08 PDT
Sorry that reviewing this patch takes so long, I'll try to get to it soon.
Comment 22 Ryosuke Niwa 2011-04-02 11:23:32 PDT
Comment on attachment 86977 [details]
Patch

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

> Source/WebKit/chromium/src/mac/WebTextHelper.mm:74
> +// This function is copied from /WebKit/mac/Misc/WebNSAttributedStringExtras.mm.

It seems like we need to copy the copyright information from that file as well then.
Comment 23 Darin Fisher (:fishd, Google) 2011-04-04 21:25:55 PDT
Comment on attachment 86977 [details]
Patch

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

> Source/WebKit/chromium/public/mac/WebTextHelper.h:44
> +class WebTextHelper {

nit: a more specific class name would be nice.

> Source/WebKit/chromium/public/mac/WebTextHelper.h:46
> +    explicit WebTextHelper(WebFrame*);

why bother instantiating this class?  why not just make substringInRange be
a static method that takes a WebFrame pointer?

> Source/WebKit/chromium/public/mac/WebTextHelper.h:50
> +    WebString substringInRange(size_t location, size_t length);

please remember to annotate entry points with WEBKIT_API.  even though it is
probably not strictly necessary for the OSX build, it is still nice to be
consistent with WEBKIT_API usage.
Comment 24 Alexey Proskuryakov 2011-04-05 12:03:42 PDT
Comment on attachment 86977 [details]
Patch

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

This will break Mac build, because the new functions need to be added to WebCore.base.exp.

Looks like a good improvement in cross platform code. r- for build breakage, and for many comments from the reviewers.

> Source/WebKit/chromium/src/mac/WebTextHelper.mm:129
> +    NSData* archivedData([NSArchiver archivedDataWithRootObject:string]);

I'm not sure if using NSArchiver is a good idea. Are you archiving in renderer process, and unarchiving in main one? If the renderer is compromised, it could send anything (like an NSWindow) in the serialized data.

> Source/WebKit/mac/WebView/WebFrame.mm:676
> +    if (range && TextIterator::locationAndLengthFromRange(range, location, length))
> +        return NSMakeRange(location, length);
> +    return NSMakeRange(NSNotFound, 0);

We generally prefer early return on error. It's more readable when the main code path is linear.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:164
> +    size_t loc, len;

Please don't abbreviate.
Comment 25 Alexey Proskuryakov 2011-04-05 12:04:51 PDT
Comment on attachment 86977 [details]
Patch

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

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

This need to be replaced with an explanation of why there are no tests. A re-commit hook won't let a patch with OOPS be landed.
Comment 26 Alexey Proskuryakov 2011-04-05 12:05:07 PDT
*pre-commit
Comment 27 Robert Sesek 2011-04-06 08:53:52 PDT
Created attachment 88434 [details]
Patch v6 WebCore
Comment 28 Robert Sesek 2011-04-06 09:02:13 PDT
Created attachment 88438 [details]
Patch v6 Chromium
Comment 29 Robert Sesek 2011-04-06 09:05:47 PDT
I've now split up the WebCore/Webkit parts and the Chromium part into two separate patches (v6).

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> This need to be replaced with an explanation of why there are no tests. A re-commit hook won't let a patch with OOPS be landed.

Done.

>> Source/WebKit/chromium/public/mac/WebTextHelper.h:44
>> +class WebTextHelper {
> 
> nit: a more specific class name would be nice.

Renamed to WebSubstringUtil.

>> Source/WebKit/chromium/public/mac/WebTextHelper.h:46
>> +    explicit WebTextHelper(WebFrame*);
> 
> why bother instantiating this class?  why not just make substringInRange be
> a static method that takes a WebFrame pointer?

Done. This class has slowly been whittled down as stuff moved to WebCore.

>> Source/WebKit/chromium/public/mac/WebTextHelper.h:50
>> +    WebString substringInRange(size_t location, size_t length);
> 
> please remember to annotate entry points with WEBKIT_API.  even though it is
> probably not strictly necessary for the OSX build, it is still nice to be
> consistent with WEBKIT_API usage.

Didn't know to do that. Done.

>> Source/WebKit/chromium/src/mac/WebTextHelper.mm:74
>> +// This function is copied from /WebKit/mac/Misc/WebNSAttributedStringExtras.mm.
> 
> It seems like we need to copy the copyright information from that file as well then.

I copied the copyright line from that file. I also left in the Google line though. Is this correct, or should it just be the Apple one?

>> Source/WebKit/chromium/src/mac/WebTextHelper.mm:129
>> +    NSData* archivedData([NSArchiver archivedDataWithRootObject:string]);
> 
> I'm not sure if using NSArchiver is a good idea. Are you archiving in renderer process, and unarchiving in main one? If the renderer is compromised, it could send anything (like an NSWindow) in the serialized data.

Thanks for bringing this up. I am indeed archiving in the renderer and unarchiving in the browser. While it can't send an NSWindow (not NSCoding compliant), it's stil something to worry about. Did you have any speciific solutions in mind? I talked with others on our Mac team and I came up with the following potential solutions:

* Create a custom NSKeyedUnarchiver that overrides |-classForClassName:| and reject any archive where the root object isn't an NSAttributedString.
* In the IPC request for the substring, include a one-time secret key which can be used to store the NSAttributedString in a keyed archive. The browser will only decode an object with that secret key one time.
* Just send the HTML fragment instead and construct the NSAttributedString in the browser. I'm not sure this is any less dangerous and it also has the issue of spinning a nested runloop to parse that HTML.

>> Source/WebKit/mac/WebView/WebFrame.mm:676
>> +    return NSMakeRange(NSNotFound, 0);
> 
> We generally prefer early return on error. It's more readable when the main code path is linear.

Done. I did this here simply because locationAndLengthFromRange() returns a bool indicating its success.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:164
>> +    size_t loc, len;
> 
> Please don't abbreviate.

Done.
Comment 30 WebKit Review Bot 2011-04-06 09:28:44 PDT
Attachment 88438 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8342230
Comment 31 Alexey Proskuryakov 2011-04-06 09:40:53 PDT
> While it can't send an NSWindow (not NSCoding compliant), it's stil something to worry about. Did you have any speciific solutions in mind?

I'm going to implement that for WebKit2 over the next few days.
Comment 32 Alexey Proskuryakov 2011-04-06 09:43:46 PDT
Comment on attachment 88434 [details]
Patch v6 WebCore

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

Great!

> Source/WebKit/mac/WebView/WebFrame.mm:676
> +    size_t location, length;

I don't know why I didn't mention that before, but we don't declare two variables on one line.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:281
> +    size_t locationSize, lengthSize;

Ditto.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:296
> +    size_t locationSize, lengthSize;

Ditto.
Comment 33 Robert Sesek 2011-04-06 10:09:25 PDT
Created attachment 88458 [details]
Patch v7 WebCore
Comment 34 Robert Sesek 2011-04-06 10:10:37 PDT
Comment on attachment 88434 [details]
Patch v6 WebCore

I've fixed all the instances of declaring two variables on a single line. Patch v7 is r? and cq? now.
Comment 35 Eric Seidel (no email) 2011-04-06 10:46:01 PDT
Comment on attachment 88434 [details]
Patch v6 WebCore

Cleared Alexey Proskuryakov's review+ from obsolete attachment 88434 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 36 WebKit Commit Bot 2011-04-06 12:22:18 PDT
The commit-queue encountered the following flaky tests while processing attachment 88458 [details]:

http/tests/xmlviewer/dumpAsText/frames.html bug 57970 (author: pfeldman@chromium.org)
The commit-queue is continuing to process your patch.
Comment 37 WebKit Commit Bot 2011-04-06 12:24:35 PDT
Comment on attachment 88458 [details]
Patch v7 WebCore

Clearing flags on attachment: 88458

Committed r83081: <http://trac.webkit.org/changeset/83081>
Comment 38 Alexey Proskuryakov 2011-04-07 14:42:04 PDT
> I'm going to implement that for WebKit2 over the next few days.

Done that in bug 58066. The code is CoreIPC specific, but you should be able to do the same for Chromium easily.
Comment 39 Robert Sesek 2011-04-11 14:09:36 PDT
Created attachment 89084 [details]
Patch v7 Chromium
Comment 40 Robert Sesek 2011-04-11 14:15:07 PDT
Comment on attachment 88438 [details]
Patch v6 Chromium

The Chromium side v7 is ready for another round of review.

(In reply to comment #29)
> Thanks for bringing this up. I am indeed archiving in the renderer and unarchiving in the browser. While it can't send an NSWindow (not NSCoding compliant), it's stil something to worry about. Did you have any speciific solutions in mind? I talked with others on our Mac team and I came up with the following potential solutions:
> 
> * Create a custom NSKeyedUnarchiver that overrides |-classForClassName:| and reject any archive where the root object isn't an NSAttributedString.
> * In the IPC request for the substring, include a one-time secret key which can be used to store the NSAttributedString in a keyed archive. The browser will only decode an object with that secret key one time.
> * Just send the HTML fragment instead and construct the NSAttributedString in the browser. I'm not sure this is any less dangerous and it also has the issue of spinning a nested runloop to parse that HTML.

I went with a solution that I didn't list here, which is to manually encode the NSAttributedString, and only the font information. Now WebSubstringUtil::attributedSubstringInRange() returns an NSAttributedString and lets the client take care of serialization.

In case you're interested, this is the Chromium class that will encode and send the object over our IPC system: http://codereview.chromium.org/6289009/diff/101001/chrome/common/attributed_string_coder.h
Comment 41 Robert Sesek 2011-04-26 09:06:08 PDT
Created attachment 91115 [details]
Patch v7.1 Chromium
Comment 42 Robert Sesek 2011-04-26 09:07:07 PDT
Comment on attachment 89084 [details]
Patch v7 Chromium

Rebased changes. Darin and Dimitri: please take a look at Chromium v7.1 patch.
Comment 43 WebKit Commit Bot 2011-04-26 14:06:20 PDT
Comment on attachment 91115 [details]
Patch v7.1 Chromium

Clearing flags on attachment: 91115

Committed r84951: <http://trac.webkit.org/changeset/84951>
Comment 44 WebKit Commit Bot 2011-04-26 14:06:28 PDT
All reviewed patches have been landed.  Closing bug.