[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 #22 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 327211
  --> https://bugs.webkit.org/attachment.cgi?id=327211
Patch

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

> Source/WebCore/html/HTMLMenuElement.cpp:43
> +#if PLATFORM(COCOA)

I don't think we want to guard this behind PLATFORM(COCOA).
There is no reason these code need to be COCOA specific beyond menuItemElementEnabled() check.
In general, we should try avoiding adding if PLATFORM(~) in WebCore
because it increases the cognitive stress for people reading the code.
It's much simpler to see the code as a runtime enabled feature.

> Source/WebCore/html/HTMLMenuElement.cpp:75
> +            for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +                if (!is<HTMLMenuItemElement>(node))

Use childrenOfType<HTMLMenuItemElement> instead.

> Source/WebCore/html/HTMLMenuItemElement.cpp:50
> +    if (type.connectedToDocument) {
> +        if (auto* page = document().page())

Don't we need to check that the parent node is a HTMLMenuElement here?

> Source/WebCore/html/HTMLMenuItemElement.cpp:60
> +        if (auto* page = document().page())

Do we need to check that the parent was a HTMLMenuElement here?

> Source/WebKit/Shared/TouchBarMenuData.h:44
> +    TouchBarMenuData();
> +    TouchBarMenuData(WebCore::HTMLMenuElement&);
> +    TouchBarMenuData(const TouchBarMenuData&);

Please make these constructors explicit.
Otherwise, we may accidentally create TouchBarMenuData from HTMLMenuElement/TouchBarMenuData in implicit coercion.

> Source/WebKit/Shared/TouchBarMenuData.h:46
> +    WEBCORE_EXPORT ~TouchBarMenuData();

As I stated in the previous patch, I don't think there is any need to define an explicit destructor.
Inlining it here should work just fine.

> Source/WebKit/Shared/TouchBarMenuItemData.h:55
> +    TouchBarMenuItemData();
> +    TouchBarMenuItemData(WebCore::HTMLMenuItemElement&);
> +    TouchBarMenuItemData(const TouchBarMenuItemData&);

Ditto to make these constructors explicit.

-- 
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/20171121/f4b1a908/attachment.html>