[Webkit-unassigned] [Bug 81882] [Gtk] display:none has no effect on <option> element

[bugzilla-daemon at webkit.org]

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


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #202313|review?                     |review-
               Flag|                            |




--- Comment #18 from Benjamin Poulain <benjamin at webkit.org>  2014-08-02 14:17:48 PST ---
(From update of attachment 202313)
View in context: https://bugs.webkit.org/attachment.cgi?id=202313&action=review

r-. Rationale: You need a good explanation in the ChangeLog. You should have layout tests or an explanation why this is not tested.

> Source/WebCore/platform/PopupMenuStyle.h:43
> +        , m_textDirection(RTL)

Default to RTL? That's odd.

> Source/WebKit2/ChangeLog:9
> +        Added PopupMenuStyle as styling for WebPopupMenu.
> +        It's needed for support display:none property on GTK WK2 popu menu style.

Typo: "popu menu style".

This change log is not good enough. First, you need to explain what the bug is, why it happens, how it happens, etc. Then you explain how you solve it.
Finally you describe the implementation details, you can use the fields bellow to give information for each function.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:898
> +    encoder << static_cast<int>(font.letterSpacing());
> +    encoder << static_cast<int>(font.wordSpacing());

Do not convert floating point values to integer across the IPC, that is an invitation for bugs!

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:928
> +void ArgumentCoder<FontDescription>::encode(ArgumentEncoder& encoder, const FontDescription& fontDescription)
> +{
> +    // FIXME: Add support for styling the font.
> +}
> +
> +bool ArgumentCoder<FontDescription>::decode(ArgumentDecoder& decoder, FontDescription& fontDescription)
> +{
> +    fontDescription = FontDescription();
> +    return true;
> +}

Don't add code that does not do anything. You can add the font description later with a proper implementation in a follow up patch.

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