[webkit-reviews] review granted: [Bug 103496] [mac] Dictionary lookup bubble loses intrarange formatting : [Attachment 176435] patch (+remove pageScaleFactor argument from PageClient::didPerformDictionaryLookup)

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 103496: [mac] Dictionary lookup bubble loses intrarange formatting
https://bugs.webkit.org/show_bug.cgi?id=103496

Attachment 176435: patch (+remove pageScaleFactor argument from
PageClient::didPerformDictionaryLookup)
https://bugs.webkit.org/attachment.cgi?id=176435&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176435&action=review


Looks great!

> Source/WebKit2/ChangeLog:11
> +	   when showing dictionary popups, so that we preserve all formatting
in the yellow dictionary
> +	   highlight. Also, remove the fontInfo member from
DictionaryPopupInfo, since we don't need it anymore.

"All" is a strong word - there are things that cannot be preserved, such as web
fonts that are only enabled in WebProcess.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:-557
> -    // We won't be able to get an NSFont in the case that a Web Font is
being used, so use
> -    // the default system font at the same size instead.

How did you test that nothing bad happens because of losing this logic here? I
assume WebHTMLConverter does the same thing, but explicit testing would be
useful anyway.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:568
> +    RetainPtr<NSAttributedString> nsAttributedString = [WebHTMLConverter
editingAttributedStringFromRange:range];

Why retain it? I think that this should be a plain pointer.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:580
> +	       font = [fontManager convertFont:font toSize:[font pointSize] *
pageScaleFactor()];
> +	       [scaledAttributes setObject:font forKey:NSFontAttributeName];

Does this work correctly with all scaling/zooming styles (Cmd+'+' as well as
pinch to zoom, and Retina displays with a default scale factor)?