[webkit-reviews] review granted: [Bug 48234] Size of text in popup menu doesn't match size of text in <select> itself in WebKit2 : [Attachment 84081] 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 48234: Size of text in popup menu doesn't match size of text in <select>
itself in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48234

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84081&action=review

> Source/WebKit2/ChangeLog:10
> +	   * Shared/TextInfo.cpp: Removed.
> +	   * Shared/TextInfo.h: Removed.
> +	   Replace this with the more appropriately DictionaryPopupInfo.

I am not a big fan of the “info” suffix. Not sure what it means, since data is
info and most classes have data.

> Source/WebKit2/Shared/DictionaryPopupInfo.h:43
> -class TextInfo {
> +class DictionaryPopupInfo {
>  public:
> -    TextInfo() { }
> +    DictionaryPopupInfo()
> +    {
> +    }

Normally we use struct when we have something with all public data members.

The explicit constructor isn’t needed. The compiler does this by default unless
you tell it not to. It is valid to do this if you want to make the constructor
private or protected, but that is not the case here.

> Source/WebKit2/Shared/FontInfo.cpp:39
> +#if PLATFORM(MAC)
> +#endif

What is this about? Please remove it.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:97
> +	   NSFontDescriptor *fontDescriptor = [NSFontDescriptor
fontDescriptorWithFontAttributes:(NSDictionary
*)data.fontInfo.fontAttributeDictionary.get()];
> +	   font = [NSFont fontWithDescriptor:fontDescriptor size:((scaleFactor
!= 1) ? [fontDescriptor pointSize] * scaleFactor : 0)];

Can we share this code with PageClientImpl.mm?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:305
> +    FloatPoint origin(finalRangeRect.x(), finalRangeRect.y());

I don’t think we need a local variable for this. Can’t we just do this in the
assignment statement?

    dictionaryPopupInfo.origin = finalRangeRect.location();