[Webkit-unassigned] [Bug 180513] [Touch Bar Web API] Add support for custom Touch Bar buttons

[bugzilla-daemon at webkit.org]

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

Wenson Hsieh <wenson_hsieh at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #329380|review?                     |review+
              Flags|                            |

--- 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.

-- 
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/20171216/557e4ee1/attachment.html>