Bug 48234

Summary: Size of text in popup menu doesn't match size of text in <select> itself in WebKit2
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: FormsAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: mitz, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
URL: data:text/html,%3Cselect%20style=%22font-size:5px%22%3E%3Coption%3Ea%3Coption%3Eb
Description Flags
Patch darin: review+

Description Adam Roben (:aroben) 2010-10-25 06:35:06 PDT
To reproduce:

1. Go to data:text/html,%3Cselect%20style=%22font-size:5px%22%3E%3Coption%3Ea%3Coption%3Eb
2. Click on the <select>

The menu that opens has text that is larger than the text in the <select> in the page. The font sizes are supposed to match.
Comment 1 Adam Roben (:aroben) 2010-10-25 06:35:42 PDT
Comment 2 Sam Weinig 2011-02-26 19:36:53 PST
Created attachment 83960 [details]
Comment 3 mitz 2011-02-26 20:01:08 PST
Comment on attachment 83960 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=83960&action=review

> Source/WebKit2/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=48234


> Source/WebKit2/ChangeLog:13
> +        Add class to encapsulate passing a font description over the wire.

Why isn’t it called FontDescription, then?

> Source/WebKit2/Shared/PlatformPopupMenuData.cpp:32
> +    


> Source/WebKit2/Shared/PlatformPopupMenuData.h:39
> +    

Here too

> Source/WebKit2/UIProcess/WebPageProxy.cpp:33
>  #include "DrawingAreaProxy.h"
> +#include "DictionaryPopupInfo.h"

Not in ASCII order.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.h:53
> +    void populate(const Vector<WebPopupItem>&, NSFont*, WebCore::TextDirection);

Missing space after “NSFont”

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:51
> +void WebPopupMenuProxyMac::populate(const Vector<WebPopupItem>& items, NSFont* font, TextDirection menuTextDirection)

Ditto. Also extra space before “font”.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:94
> +    NSFont *font = nil;

No need to initialize this to nil.
Comment 4 mitz 2011-02-27 00:06:40 PST
Comment on attachment 83960 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=83960&action=review

r- since this breaks Qt and Windows. But please also consider my previous comments and the next one.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebPopupMenuMac.mm:53
> +    data.fontInfo.fontAttributeDictionary = fontDescriptorAttributes;
> +    data.fontInfo.fontOverrideSize = frame->pageScaleFactor() == 1 ? 0 : ([[font fontDescriptor] pointSize] * frame->pageScaleFactor());

The “override” concept seems unnecessarily cumbersome. Why not simply make a mutable copy of the dictionary and change the font size attribute? Alternatively, since the UI process knows the page scale factor, why not leave this multiplication exercise up to it
Comment 5 Sam Weinig 2011-02-27 10:42:33 PST
Created attachment 83980 [details]
Comment 6 Sam Weinig 2011-02-27 11:45:59 PST
Created attachment 83983 [details]
Comment 7 Sam Weinig 2011-02-27 12:27:28 PST
(In reply to comment #6)
> Created an attachment (id=83983) [details]
> Patch

This patch addresses many of the review comments but does not change FontInfo to be named FontDescription because that led to a name clash with WebCore that was confusing the message generator and does not remove the overrideSize concept, since that existed prior to this patch (for the dictionary lookup stuff) and I can change them both in a follow up patch.
Comment 8 Sam Weinig 2011-02-28 10:38:53 PST
Created attachment 84081 [details]
Comment 9 Darin Adler 2011-02-28 10:53:04 PST
Comment on attachment 84081 [details]

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
> +#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();
Comment 10 Sam Weinig 2011-02-28 11:19:14 PST
Landed in 79886.