[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

Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dbates at webkit.org

--- Comment #38 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 327868
  --> https://bugs.webkit.org/attachment.cgi?id=327868
Patch

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

> Source/WebKit/Shared/TouchBarMenuData.cpp:33
> +#include <WebCore/HTMLElement.h>

It is unnecessary to include this header because HTMLMenuElement.h (below) will include it.

> Source/WebKit/Shared/TouchBarMenuData.cpp:36
> +#include <WebCore/Node.h>

Ditto.

> Source/WebKit/Shared/TouchBarMenuData.cpp:40
> +TouchBarMenuData::TouchBarMenuData()

It is sufficient to use = default:

TouchMenuBarData::TouchBarMenuData() = default;

> Source/WebKit/Shared/TouchBarMenuData.cpp:48
> +    m_id = element.attributeWithoutSynchronization(WebCore::HTMLNames::idAttr);

We are not making use of m_id in this patch. I take it you plan to make use of it in a follow up?

> Source/WebKit/Shared/TouchBarMenuData.cpp:76
> +    if (!decoder.decode(data.m_items))

I would have written this body in one line:

return decoder.decode(data.m_items);

> Source/WebKit/Shared/TouchBarMenuData.h:29
> +#include <WebCore/HTMLMenuElement.h>

It is unnecessary to include this header. It is sufficient to forward declare HTMLMenuElement.

> Source/WebKit/Shared/TouchBarMenuData.h:53
> +    void setID(String);

This member function is never defined. Do we still need this function? Do you plan to implement in a follow up? If not, then please remove.

> Source/WebKit/Shared/TouchBarMenuItemData.cpp:44
> +TouchBarMenuItemData::TouchBarMenuItemData()

Please default this constructor. See my remark in file TouchBarMenuData.cpp for an example.

> Source/WebKit/Shared/TouchBarMenuItemData.cpp:56
> +TouchBarMenuItemData::TouchBarMenuItemData(const TouchBarMenuItemData& other)

Is  it necessary that this copy constructor be explicit? If so, the. Please “= default” this function (see my remark above about doing the same for the constructor). Otherwise, remove this implementation and the declaration for it and let the compiler generate this for us.

> Source/WebKit/UIProcess/WebPageProxy.messages.in:317
> +

I do not see the need to add this empty line. One empty line is sufficient to demarcate the section above from this code.

-- 
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/20171202/fb11ac25/attachment.html>