Bug 54969

Created attachment 83828 [details] Patch

Darin, do you have time to review these changes to the Chromium WebKit API? If not, can you suggest a reviewer?

Attachment 83828 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8043342

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?

(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 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?

Created attachment 86464 [details] Patch

(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.

Attachment 86464 [details] did not build on qt: Build output: http://queues.webkit.org/results/8221385

Attachment 86464 [details] did not build on win: Build output: http://queues.webkit.org/results/8224304

It seems very strange that implementing something that already exists in another port requires adding code to WebCore.

(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?

I see that there was a little code removed from WebKit2, but nothing from WebKit/mac.

Created attachment 86512 [details] Patch

(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 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.

Created attachment 86837 [details] Patch

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.

Created attachment 86977 [details] Patch

(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.

Sorry that reviewing this patch takes so long, I'll try to get to it soon.

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 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 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 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.

*pre-commit

Created attachment 88434 [details] Patch v6 WebCore

Created attachment 88438 [details] Patch v6 Chromium

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.

Attachment 88438 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8342230

> 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 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.

Created attachment 88458 [details] Patch v7 WebCore

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 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.

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 on attachment 88458 [details] Patch v7 WebCore Clearing flags on attachment: 88458 Committed r83081: <http://trac.webkit.org/changeset/83081>

> 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.

Created attachment 89084 [details] Patch v7 Chromium

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

Created attachment 91115 [details] Patch v7.1 Chromium

Comment on attachment 89084 [details] Patch v7 Chromium Rebased changes. Darin and Dimitri: please take a look at Chromium v7.1 patch.

Comment on attachment 91115 [details] Patch v7.1 Chromium Clearing flags on attachment: 91115 Committed r84951: <http://trac.webkit.org/changeset/84951>

All reviewed patches have been landed. Closing bug.