[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

--- Comment #39 from Wenson Hsieh <wenson_hsieh at apple.com> ---
(In reply to Daniel Bates from comment #38)
> Comment on attachment 327868 [details]
> 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.

I'm tracking cleanup with a followup bug here: https://bugs.webkit.org/show_bug.cgi?id=180305. Some of our comments overlap — I'll adjust for your review feedback as well.

-- 
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/ca71a92d/attachment-0001.html>