[webkit-reviews] review granted: [Bug 180513] [Touch Bar Web API] Add support for custom Touch Bar buttons : [Attachment 329380] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Aishwarya Nirmal
<anirmal at apple.com>'s request for review:
Bug 180513: [Touch Bar Web API] Add support for custom Touch Bar buttons
https://bugs.webkit.org/show_bug.cgi?id=180513

Attachment 329380: Patch

https://bugs.webkit.org/attachment.cgi?id=329380&action=review




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

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

This mostly looks good! There are still a few important issues, but core
functionality is there and tested — let's add a couple of FIXMEs.

> Source/WebCore/dom/Document.cpp:1017
> +    m_currentMenuElement = element;

Nothing in this patch seems to clear out the current menu element (i.e.
setCurrentMenuElement(nullptr)). Can we note this in a FIXME?

We probably should clear this out when a menu element is disconnected from the
document, if the disconnected menu element is the current menu element.

> Source/WebCore/html/HTMLMenuElement.cpp:52
> +	   if (item.attributeWithoutSynchronization(titleAttr) == identifier) {

I don't think checking against the button title will work if there are multiple
buttons with the same title...can we note this in a FIXME?

I think we should probably assign a UUID for each menu element, and propagate
that over IPC to the UI process and back instead.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:667
> +- (nullable NSTouchBarItem *)itemForIdentifier:(NSString *)identifier

We won't need to override this method if we're just calling super.