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.
<rdar://problem/8589450>
Created attachment 83960 [details] Patch
Comment on attachment 83960 [details] Patch 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 <rdar://problem/8589450> > 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 > + Whitespace > 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 on attachment 83960 [details] Patch 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
Created attachment 83980 [details] Patch
Created attachment 83983 [details] Patch
(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.
Created attachment 84081 [details] Patch
Comment on attachment 84081 [details] Patch 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();
Landed in 79886.