| Summary: | Dictionary text extraction is not correctly detecting word boundaries on bing.com | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||
| Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | ||||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Attachments: |
|
||||||
|
Description
Sam Weinig
2011-03-23 21:25:00 PDT
Created attachment 86740 [details]
Patch
Comment on attachment 86740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86740&action=review > Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm:429 > + // Convert to screen coordinates. > + textBaselineOrigin = [m_wkView convertPoint:textBaselineOrigin toView:nil]; > + textBaselineOrigin = [m_wkView.window convertRectToScreen:NSMakeRect(textBaselineOrigin.x, textBaselineOrigin.y, 0, 0)].origin; Did you test this in scale factors other than 1X? > Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm:431 > + NSMutableDictionary *options = [NSMutableDictionary dictionaryWithCapacity:dictionaryPopupInfo.options.size()]; Normally we avoid autoreleased objects. I would have expected alloc/init and RetainPtr here instead. Probably fine either way, though. > Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm:436 > + HashMap<String, String>::const_iterator it = dictionaryPopupInfo.options.begin(); > + HashMap<String, String>::const_iterator end = dictionaryPopupInfo.options.end(); > + for (; it != end; ++it) > + [options setObject:nsStringFromWebCoreString(it->second) forKey:nsStringFromWebCoreString(it->first)]; Seems to me we should have a helper function to turn a HashMap<String, String> into a NSDictionary *, rather than doing it in the middle of this function. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:251 > +static bool shouldUseSelection(const VisiblePosition& position, const VisibleSelection& selection) I find this function slightly confusing. Too much of it is connective tissue converting things to Range and then Range’s API is so awkward. If the whole body of the function once the ranges were created was a named helper function then I could probably understand it perfectly. It’s not clear what compareBoundaryPoints and isPointInRange are accomplishing and the function name would probably clear that up. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:318 > + VisiblePosition wordStart = startOfWord(position); > + VisiblePosition wordEnd = endOfWord(position); > + > + RefPtr<Range> finalRange = makeRange(wordStart, wordEnd); I don’t think the wordStart and wordEnd locals are needed or helpful. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:322 > + NSDictionary *options = nil; Would be nice to only define this once instead of defining it in both sides of the #if. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:335 > + if (selection.isNone()) > + return; > + > + RefPtr<Range> selectedRange = selection.toNormalizedRange(); > + if (!selectedRange) > + return; The isNone check is redundant and could be omitted. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:355 > + NSDictionary *options = nil; Would be nice to only define this once instead of on both sides of the #if. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:390 > + for (NSString *key in options) > + dictionaryPopupInfo.options.add(key, (NSString *)[options valueForKey:key]); Would be nice to have a function to convert a NSDictionary of strings into a HashMap<String, String> instead of doing it inline in a loop. Although that is a pretty tight loop! |