Bug 56995 - Dictionary text extraction is not correctly detecting word boundaries on bing.com
Summary: Dictionary text extraction is not correctly detecting word boundaries on bing...
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Sam Weinig
Depends on:
Reported: 2011-03-23 21:25 PDT by Sam Weinig
Modified: 2011-03-24 13:06 PDT (History)
0 users

See Also:

Patch (18.96 KB, patch)
2011-03-23 21:32 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2011-03-23 21:25:00 PDT
Dictionary text extraction is not correctly detecting word boundaries on bing.com
Comment 1 Sam Weinig 2011-03-23 21:32:18 PDT
Created attachment 86740 [details]
Comment 2 Darin Adler 2011-03-24 10:05:26 PDT
Comment on attachment 86740 [details]

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!
Comment 3 Sam Weinig 2011-03-24 13:06:20 PDT
Fixed in r81890.