[webkit-reviews] review denied: [Bug 81882] [Gtk] display:none has no effect on <option> element : [Attachment 202313] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Aug 2 14:17:35 PDT 2014


Benjamin Poulain <benjamin at webkit.org> has denied Oleg Smirnov
<oleg_smirnov at hotmail.com>'s request for review:
Bug 81882: [Gtk] display:none has no effect on <option> element
https://bugs.webkit.org/show_bug.cgi?id=81882

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.


More information about the webkit-reviews mailing list