[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 #12 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 327018
  --> https://bugs.webkit.org/attachment.cgi?id=327018
Patch

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

(just some minor nits here and there)

> Source/WebCore/html/HTMLMenuElement.cpp:86
> +        HTMLElement::parseAttribute(name, value);

We typically prefer early returns to if-else, so this would be something like:

if (name != typeAttr) {
    HTMLElement::parseAttribute(name, value);
    return;
}

...rest of logic...

> Source/WebKit/Shared/TouchBarMenuData.cpp:37
> +#include <vector>

I don't think we need to include <vector>...unless we're using the standard library vector instead of WTF's Vector somewhere here?

> Source/WebKit/Shared/TouchBarMenuData.cpp:42
> +    : itemType(ItemType::Button)

You can initialize these members directly in the header, e.g. by doing `ItemType itemType { ItemType::Button };`.

> Source/WebKit/Shared/TouchBarMenuData.cpp:43
> +    , identifier("")

Do these need to be initialized to the empty string, or is null String (i.e. the result of the default String() constructor) sufficient? If null strings are okay, these will be implicitly initialized to the value of String() by default, so we shouldn't need to explicitly set their values.

> Source/WebKit/Shared/TouchBarMenuData.cpp:120
> +    // default button

Comments in WebKit code should begin with a capital and end with a period (for reference, here's the style guide: https://webkit.org/code-style-guidelines/)

> Source/WebKit/Shared/TouchBarMenuData.cpp:124
> +// Touch Bar Menu Items will be ordered based on priority

This comment should end with a period.

> Source/WebKit/Shared/TouchBarMenuData.cpp:125
> +inline bool operator< (const TouchBarMenuItemData lhs, TouchBarMenuItemData rhs)

Extra space between < and (.

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

m_items will be initialized to an empty vector by default, so we won't need this here.

> Source/WebKit/Shared/TouchBarMenuData.cpp:186
> +TouchBarMenuData* TouchBarMenuData::copy() const

Hm...do we need this method? I don't see it being used anywhere else in this patch.

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

This should probably be (const TouchBarMenuItemData& data)

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

const TouchBarMenuItemData& data

> Source/WebKit/Shared/TouchBarMenuData.cpp:206
> +void TouchBarMenuData::setID(String id)

(const String& id)

> Source/WebKit/Shared/TouchBarMenuData.h:42
> +        CandidateList,

Interesting! I'm not sure we want to support all of these item types though...perhaps we should just limit it to buttons and maybe popover items for now? We can always add more touch bar item types here as we expose richer customization options to web content.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5386
> +    TouchBarMenuData* data = new TouchBarMenuData(element);

I'm not sure we need to allocate this TouchBarMenuData on the heap...can we just do something like:

m_currTouchBarMenuData = { element };
send(Messages::WebPageProxy::TouchBarMenuDataChanged(m_currTouchBarMenuData));

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5396
> +    TouchBarMenuItemData* data = new TouchBarMenuItemData(element);

Same here, we should just be able to do something like:

TouchBarMenuItemData data { element };
m_currTouchBarMenuData.addMenuItem(data);
send(Messages::WebPageProxy::TouchBarMenuDataChanged(m_currTouchBarMenuData));

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5405
> +    TouchBarMenuItemData* data = new TouchBarMenuItemData(element);

Ditto, let's create this on the stack instead.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1615
> +    TouchBarMenuData m_currTouchBarMenuData;

We generally use full words instead of abbreviations, so this should probably be m_currentTouchBarMenuData; (you can refer to WebKit style guidelines for more information: https://webkit.org/code-style-guidelines/)

-- 
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/20171116/9b98597d/attachment.html>