[webkit-reviews] review granted: [Bug 56995] Dictionary text extraction is not correctly detecting word boundaries on bing.com : [Attachment 86740] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 56995: Dictionary text extraction is not correctly detecting word
boundaries on bing.com
https://bugs.webkit.org/show_bug.cgi?id=56995

Attachment 86740: Patch
https://bugs.webkit.org/attachment.cgi?id=86740&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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!