[Webkit-unassigned] [Bug 179714] [Touch Bar Web API] Object representing Touch Bar Menu to send between Web and UI Processes

[bugzilla-daemon at webkit.org]

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

--- Comment #15 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 327130
  --> https://bugs.webkit.org/attachment.cgi?id=327130
Patch

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

> Source/WebCore/html/HTMLMenuElement.cpp:69
> +    if (!RuntimeEnabledFeatures::sharedFeatures().menuItemElementEnabled())

I think we'd still want to defer to the superclass' implementation by calling HTMLElement::parseAttribute(name, value), in the case where the menu item element isn't enabled...unless there's good reason we should skip the super call in this case?

If not, let's fold this `!RuntimeEnabledFeatures::sharedFeatures().menuItemElementEnabled()` check in with the early return above, so we bail `if (name != typeAttr || !RuntimeEnabledFeatures::sharedFeatures().menuItemElementEnabled())`.

> Source/WebCore/html/HTMLMenuElement.h:33
> +    InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) final;

We usually make these private instead of public.

> Source/WebKit/Shared/TouchBarMenuData.cpp:41
> +    : m_items()

Nit - We don't need to explicitly initialize this, since it'll be initialized to an empty vector by default.

> Source/WebKit/Shared/TouchBarMenuData.cpp:42
> +    , m_isPageCustomized(false)

Nit - We typically initialize these in the header file (e.g. bool m_isPageCustomized { false };)

> Source/WebKit/Shared/TouchBarMenuData.cpp:65
> +void TouchBarMenuData::addMenuItem(const TouchBarMenuItemData data)

const reference! (const TouchBarMenuItemData&)

> Source/WebKit/Shared/TouchBarMenuData.cpp:70
> +void TouchBarMenuData::removeMenuItem(const TouchBarMenuItemData data)

This should be a const reference (const TouchBarMenuItemData&)

> Source/WebKit/Shared/TouchBarMenuItemData.cpp:62
> +    : itemType(ItemType::Button)

These should be initialized in the header (e.g. ItemType itemType { ItemType::Button };)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5382
> +    send(Messages::WebPageProxy::TouchBarMenuDataChanged(temp), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);

I don't think we'll need to use the DispatchMessageEvenWhenWaitingForSyncReply option for these IPC message sends...

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171117/371ab7dc/attachment-0001.html>