[Webkit-unassigned] [Bug 54969] [Chromium] Implement WebKit methods to assist with Cocoa NSTextInput implementation

[bugzilla-daemon at webkit.org]

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





--- Comment #29 from Robert Sesek <rsesek at chromium.org>  2011-04-06 09:05:47 PST ---
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.

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