[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

--- Comment #13 from Darin Adler <darin 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 patch looks really good. There is more work that might be needed to get this completely solid.

Likely the best analogy for the global "current menu element" is the current title element, the one that determines the document’s title. Having clear rules about which menu element takes effect if there is more than one and what happens is menu elements are added or removed can easily become important for cross-browser interoperability, like they did for the title element.

It also seems wrong to call the menu element for the touch bar just the "current menu element". It’s an important menu element, the one with a menu that should be seen, but there could also (some day) be a menu element for the current right mouse click, or something like that. So "current menu element" sounds not *quite* right as the name for the concept we are implementing, but maybe I am wrong about that.

The pattern for the title element is that the element itself just notifies the document and it’s the document that has to adjudicate which one is the effective title element. Thus the title element itself doesn’t do things like directly notifying a client that it exists, since that’s unwanted for an extra title element that is not the one in effect.

Sadly, the title element code is a bit complicated because of special cases for SVG, so we wouldn’t want to replicate all of its code, but the general pattern is probably right.

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

The most likely correct pattern for object lifetimes would be to match the other RefPtr<Element> type data members in Document and add code to null it out in Document::removedLastRef. This is important to avoid a possible massive storage leak.

Separately, it’s also likely critical for correctness reasons to null it out when disconnecting from the document as Wenson says. So for that a FIXME, might be sufficient. I think what we need most for that is a test case.

> Source/WebCore/dom/Document.h:481
> +    void setCurrentMenuElement(RefPtr<HTMLMenuElement>&&);
> +    RefPtr<HTMLMenuElement>& currentMenuElement() { return m_currentMenuElement; }

Given that setCurrentMenuElement is only called when m_isTouchBarMenu is true, this is not a "current menu element" at all. It’s a "current touch bar menu".

The argument to setCurrentMenuElement does not need to be a RefPtr&& unless callers are going to pass in a newly created object or need to hand over ownership for some reason. I suggest HTMLMenuElement& or HTMLMenuElement* for the argument type.

It’s peculiar to have both a set function and also a function that returns a non-const reference. Those two idioms are both ways to allow someone to set and we don’t need both. I suggest changing currentMenuElement() to be a const member function that returns HTMLMenuElement*, or if I am out of date and it’s important to return RefPtr for our future design for security, then it should be a const member function that returns RefPtr<HTMLMenuElement> rather than a non-const member function that returns RefPtr<HTMLMenuElement>&.

These two new functions should *not* be paragraphed in with adoptNode, which is a very different type of function with a more generic purpose. They should be their own paragraph or be grouped with functions that similarly track a document-wide special element.

> Source/WebCore/html/HTMLMenuElement.cpp:46
> +void HTMLMenuElement::didInteractWithItem(const String& identifier)

I think this function should be called clickItem or something like that. Doesn’t seem to require the "did" naming.

But given Wenson’s comment about UUIDs this may be changing significantly anyway.

> Source/WebCore/html/HTMLMenuElement.cpp:61
> +    Ref<MouseEvent> mouseEvent = MouseEvent::create(eventNames().clickEvent, document().defaultView(), platformEvent, 0, 0);
> +    tappedItem->dispatchEvent(mouseEvent);

I suggest combining these two into a single line.

> Source/WebCore/html/HTMLMenuElement.cpp:68
> +        document().setCurrentMenuElement(this);

This is a peculiar rule. It means that whenever we insert a new HTMLMenuElement that is a touch bar menu, then it will displace the already existing one. It’s a "last one inserted wins" rule. We should be sure that’s the rule we want. Arbitrary rules like this often lead to cross-browser incompatibility so it’s typically best to come up with an explicit rule.

> Source/WebCore/html/HTMLMenuElement.cpp:70
>          if (auto* page = document().page())
>              page->chrome().client().didInsertMenuElement(*this);

Is it a good idea to call the chrome client every time we insert a touch bar menu element? What do we expect clients to do about multiple touch bar menu elements? I think maybe we actually want to call the client when the current menu element changes; could be a Document thing instead of an HTMLMenuElement thing. And also, seems not quite right to do this only for touch bar menus, yet have the client function be named generally "did insert menu element". Name and semantics should match.

> Source/WebCore/html/HTMLMenuElement.h:33
> +    WEBCORE_EXPORT void didInteractWithItem(const WTF::String& identifier);

No need for the "WTF::" prefix here.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1109
> +        RetainPtr<NSCustomTouchBarItem> touchBarItem = adoptNS([[NSCustomTouchBarItem alloc] initWithIdentifier:item.title]);

Can use "auto" here.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5413
> +    if (auto* frame = mainFrame()) {
> +        if (auto element = frame->document()->currentMenuElement())
> +            element->didInteractWithItem(touchBarMenuItemData.title);
> +    }

Seems to me this should be calling a function on the Document, instead of explicitly getting at the current menu element. This function we would add on Document would replace the Document::currentMenuElement function entirely.

-- 
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/20171217/359e8f7a/attachment-0001.html>