[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
----------------------------------------------------------------------------
                 CC|                            |wenson_hsieh at apple.com

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

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

> Source/WebCore/ChangeLog:9
> +        No new tests.

I think we're at the point where we should be able to test end-to-end now...we should chat about a testing strategy for this. I could imagine either API tests that ask AppKit directly for touch bar state, or layout tests that involve plumbing touch bar state through UIScriptController in WebKitTestRunner.

> Source/WebCore/html/HTMLMenuElement.cpp:51
> +    for (unsigned i = 0; i < children->length(); i++) {

I think childrenOfType<HTMLMenuItemElement>() is the preferred way to iterate over menuitems in the subtree. We just have to make sure we don't dispatch the event while the iterator is still in scope, otherwise we'll hit a security assertion. We could do something like:

- iterate over childrenOfType
- bail if we find an appropriate menuitem
- then dispatch a click on the menuitem

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

I don't think we need the explicit WTF:: namespacing here.

> Source/WebKit/Shared/TouchBarMenuData.h:51
> +    Vector<TouchBarMenuItemData> items() const { return m_items; }

We should be able to keep this a const Vector& if we tweak the call site to be something like

auto& menuItems = touchBarMenuData.items();

Unless I'm missing something, I don't think this really needs to return a copy?

> Source/WebKit/Shared/TouchBarMenuData.h:57
> +    String identifier() const { return m_id; }

Let's rename identifier => title, to reflect the change in attribute.

> Source/WebKit/Shared/TouchBarMenuData.h:-59
> -    

Nit - stray whitespace change.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:650
> +    [[NSNotificationCenter defaultCenter] removeObserver:self];

Hm...it's not obvious to me why we need to remove ourselves as an observer. Does this object start listening for notifications elsewhere in this patch?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:670
> +    return item;

This should probably return an -autorelease object, since the name (-itemForIdentifier:) doesn't reference "new" or "create".

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:971
> +        touchBar = m_customizedTouchBar.get();

It's a bit weird to refer to this page-controlled touch bar as the "customized" touch bar, since there's a notion of touch bar "customization" already (if you go into View > Customize Touch Bar, you can rearrange or add or remove touch bar items) and it sounds confusing to mix the terms...

I think the "pageCustomizedTouchBar" terminology you use elsewhere in this patch makes this distinction clearer. Can we rename m_customizedTouchBar to m_pageCustomizedTouchBar to be consistent?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1107
> +    NSMutableSet<NSTouchBarItem *> *templateItems = [[NSMutableSet alloc] init];

adoptNS() here to fix the leak!

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1108
> +    NSMutableArray<NSTouchBarItemIdentifier> *defaultItems = [[NSMutableArray alloc] init];

Ditto.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1112
> +        NSCustomTouchBarItem *touchBarItem = [[NSCustomTouchBarItem alloc] initWithIdentifier:item.identifier];

adoptNS() to fix the leak!

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5411
> +        if (auto* element = frame->document()->currentMenuElement().get())

We can remove the `.get()` at the end if we just make this `auto element`.

-- 
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/20171207/71cf2220/attachment-0001.html>