[Webkit-unassigned] [Bug 48234] Size of text in popup menu doesn't match size of text in <select> itself in WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48234


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84081|review?                     |review+
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2011-02-28 10:53:04 PST ---
(From update of attachment 84081)
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();

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.