Bug 179714 – [Touch Bar Web API] Object representing Touch Bar Menu to send between Web and UI Processes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Aishwarya Nirmal]

This change defines an object that contains data for the touch bar menu, and the information stored in this object will be obtained from attributes in the menu and menuitem html elements that represent the touch bar. This object will be sent between the web and ui processes.

[Aishwarya Nirmal]

Created attachment 326957 [details]
Patch

[Aishwarya Nirmal]

Created attachment 326997 [details]
Patch

[Aishwarya Nirmal]

Created attachment 326998 [details]
Patch

[Build Bot]

Comment on attachment 326998 [details]
Patch

Attachment 326998 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5248702

New failing tests:
fast/events/tabindex-focus-blur-all.html
imported/w3c/web-platform-tests/html/dom/reflection-misc.html

[Build Bot]

Created attachment 327006 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326998 [details]
Patch

Attachment 326998 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5248816

New failing tests:
fast/events/tabindex-focus-blur-all.html
imported/w3c/web-platform-tests/html/dom/reflection-misc.html

[Build Bot]

Created attachment 327010 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326998 [details]
Patch

Attachment 326998 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5249433

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-misc.html

[Build Bot]

Created attachment 327014 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

Comment on attachment 326998 [details]
Patch

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

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

This code is almost certainly wrong. This function gets called whenever a this menu element is inserted into some ancestor
regardless of whether it's in a document or not. You need to check type to see if it's in the document or not.

It's very important that didInsertMenuElement never tries to execute any scripts.
Are you sure this new ChromeClient callback wouldn't do that?

> Source/WebCore/html/HTMLMenuElement.cpp:54
> +            page->chrome().client().didRemoveMenuElement(*this);

Ditto.

> Source/WebCore/html/HTMLMenuElement.cpp:66
> +        bool oldIsTouchBarMenu = m_isTouchBarMenu;

We usually call this variable wasTouchBarMenu

> Source/WebCore/html/HTMLMenuElement.cpp:70
> +        if (!RuntimeEnabledFeatures::sharedFeatures().menuItemElementEnabled() || !m_initialTypeAttributeParsed)
> +            m_initialTypeAttributeParsed = true;
> +        else {

Why is the initial attribute value important? I don't think it makes sense to lock the type of an element like that.
It's more natural that changing the type value would change the semantics of the element (like when you removed an menu element).

> Source/WebCore/html/HTMLMenuItemElement.cpp:45
> +    if (auto* page = document().page())
> +        page->chrome().client().didInsertMenuItemElement(*this);

Ditto. This code is almost certainly wrong. You should run this code only when the node is newly connected to a document,
and we need to make sure didInsertMenuItemElement callback doesn't try to execute any scripts or otherwise callback into WebKit to do so.

> Source/WebCore/html/HTMLMenuItemElement.cpp:52
> +    if (auto* page = document().page())

Ditto.

> Source/WebCore/html/HTMLMenuItemElement.cpp:76
> +    if (name == typeAttr)
> +        m_type = value;
> +    else if (name == idAttr)
> +        m_identifier = value;
> +    else if (name == hiddenAttr)
> +        m_visible = !equalLettersIgnoringASCIICase(value, "true");
> +    else if (name == titleAttr)
> +        m_customizationLabel = value;
> +    else if (name == onclickAttr)
> +        m_commandName = value;
> +    else if (name == srcAttr)
> +        m_viewReference = value;
> +    else if (name == valueAttr)
> +        m_priority = value.toFloat();

What's the point of storing all these values in a member variable. We can just get the value and covert on demand via getAttribute.
r- because we shouldn't be making copies of content attributes like this.

> Source/WebKit/Shared/TouchBarMenuData.cpp:47
> +    , identifier("")
> +    , visible(false)
> +    , viewRef("")
> +    , commandName("")
> +    , customizationLabel("")

No need to initialize strings like this. They're initialized to empty string by default.

> Source/WebKit/Shared/TouchBarMenuData.cpp:125
> +inline bool operator< (const TouchBarMenuItemData lhs, TouchBarMenuItemData rhs)

Nit: we don't put a space after operator< like this.

> Source/WebKit/Shared/TouchBarMenuData.cpp:130
> +inline bool operator> (const TouchBarMenuItemData lhs, TouchBarMenuItemData rhs)

Ditto. The space between operator< and ( should be removed.

> Source/WebKit/Shared/TouchBarMenuData.cpp:135
> +inline bool operator<= (const TouchBarMenuItemData lhs, TouchBarMenuItemData rhs)

Ditto.

> Source/WebKit/Shared/TouchBarMenuData.cpp:140
> +inline bool operator>= (const TouchBarMenuItemData lhs, TouchBarMenuItemData rhs)

Ditto.

> Source/WebKit/Shared/TouchBarMenuData.cpp:145
> +inline bool operator== (const TouchBarMenuItemData lhs, TouchBarMenuItemData rhs)

Ditto.

> Source/WebKit/Shared/TouchBarMenuData.cpp:156
> +inline bool operator!= (const TouchBarMenuItemData lhs, TouchBarMenuItemData rhs)

Ditto.

> Source/WebKit/Shared/TouchBarMenuData.cpp:209
> +Vector<TouchBarMenuItemData> TouchBarMenuData::items()
> +{
> +    return m_items;
> +}
> +    
> +void TouchBarMenuData::setID(String id)
> +{
> +    m_id = id;
> +}

We're better of inlining these one-line functions in the header files.

> Source/WebKit/Shared/TouchBarMenuData.h:39
> +class TouchBarMenuItemData {
> +public:

Nit: need a black line after namespace WebKit {.

Since everything in this class is public, it's more appropriate to use struct.

> Source/WebKit/Shared/TouchBarMenuData.h:56
> +    WEBCORE_EXPORT ~TouchBarMenuItemData();

The default destructor is sufficient.

We usually add an empty destructor in CPP file to avoid #include in header files
since the default constructor must be able to invoke destructors of member variables.
However, here, all member variables are of POD types or String,
and none of them require additional header files to be included.

> Source/WebKit/Shared/TouchBarMenuData.h:71
> +private:

If there are no private members, we should get rid of this private access specifier.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5378
> +    m_currTouchBarMenuData = *(new TouchBarMenuData());

r- this leaks TouchBarMenuData.

Since m_currTouchBarMenuData is of type TouchBarMenuData, this code would simply construct
a new instance of TouchBarMenuData, then assign its value to m_currTouchBarMenuData.

Also, please don't abbreviate current as curr.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5388
> +    TouchBarMenuData* data = new TouchBarMenuData(element);
> +    m_currTouchBarMenuData = *data;
> +    send(Messages::WebPageProxy::TouchBarMenuDataChanged(*data), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);

Again, this leaks the TouchBarMenuData created at line 5386 for no good reason.

Since you're encoding the entire object, what is the point of keeping it in the web content process?
Can't we just send it to UI process without storing it in WebContent process?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5398
> +    TouchBarMenuItemData* data = new TouchBarMenuItemData(element);
> +    m_currTouchBarMenuData.addMenuItem(*data);
> +    send(Messages::WebPageProxy::TouchBarMenuDataChanged(m_currTouchBarMenuData), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);

Ditto.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5407
> +    TouchBarMenuItemData* data = new TouchBarMenuItemData(element);
> +    m_currTouchBarMenuData.removeMenuItem(*data);
> +    send(Messages::WebPageProxy::TouchBarMenuDataChanged(m_currTouchBarMenuData), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);

Ditto.

[Aishwarya Nirmal]

Created attachment 327018 [details]
Patch

[Wenson Hsieh]

Comment on attachment 327018 [details]
Patch

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

(just some minor nits here and there)

> Source/WebCore/html/HTMLMenuElement.cpp:86
> +        HTMLElement::parseAttribute(name, value);

We typically prefer early returns to if-else, so this would be something like:

if (name != typeAttr) {
    HTMLElement::parseAttribute(name, value);
    return;
}

...rest of logic...

> Source/WebKit/Shared/TouchBarMenuData.cpp:37
> +#include <vector>

I don't think we need to include <vector>...unless we're using the standard library vector instead of WTF's Vector somewhere here?

> Source/WebKit/Shared/TouchBarMenuData.cpp:42
> +    : itemType(ItemType::Button)

You can initialize these members directly in the header, e.g. by doing `ItemType itemType { ItemType::Button };`.

> Source/WebKit/Shared/TouchBarMenuData.cpp:43
> +    , identifier("")

Do these need to be initialized to the empty string, or is null String (i.e. the result of the default String() constructor) sufficient? If null strings are okay, these will be implicitly initialized to the value of String() by default, so we shouldn't need to explicitly set their values.

> Source/WebKit/Shared/TouchBarMenuData.cpp:120
> +    // default button

Comments in WebKit code should begin with a capital and end with a period (for reference, here's the style guide: https://webkit.org/code-style-guidelines/)

> Source/WebKit/Shared/TouchBarMenuData.cpp:124
> +// Touch Bar Menu Items will be ordered based on priority

This comment should end with a period.

> Source/WebKit/Shared/TouchBarMenuData.cpp:125
> +inline bool operator< (const TouchBarMenuItemData lhs, TouchBarMenuItemData rhs)

Extra space between < and (.

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

m_items will be initialized to an empty vector by default, so we won't need this here.

> Source/WebKit/Shared/TouchBarMenuData.cpp:186
> +TouchBarMenuData* TouchBarMenuData::copy() const

Hm...do we need this method? I don't see it being used anywhere else in this patch.

> Source/WebKit/Shared/TouchBarMenuData.cpp:191
> +void TouchBarMenuData::addMenuItem(TouchBarMenuItemData data)

This should probably be (const TouchBarMenuItemData& data)

> Source/WebKit/Shared/TouchBarMenuData.cpp:196
> +void TouchBarMenuData::removeMenuItem(TouchBarMenuItemData data)

const TouchBarMenuItemData& data

> Source/WebKit/Shared/TouchBarMenuData.cpp:206
> +void TouchBarMenuData::setID(String id)

(const String& id)

> Source/WebKit/Shared/TouchBarMenuData.h:42
> +        CandidateList,

Interesting! I'm not sure we want to support all of these item types though...perhaps we should just limit it to buttons and maybe popover items for now? We can always add more touch bar item types here as we expose richer customization options to web content.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5386
> +    TouchBarMenuData* data = new TouchBarMenuData(element);

I'm not sure we need to allocate this TouchBarMenuData on the heap...can we just do something like:

m_currTouchBarMenuData = { element };
send(Messages::WebPageProxy::TouchBarMenuDataChanged(m_currTouchBarMenuData));

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5396
> +    TouchBarMenuItemData* data = new TouchBarMenuItemData(element);

Same here, we should just be able to do something like:

TouchBarMenuItemData data { element };
m_currTouchBarMenuData.addMenuItem(data);
send(Messages::WebPageProxy::TouchBarMenuDataChanged(m_currTouchBarMenuData));

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5405
> +    TouchBarMenuItemData* data = new TouchBarMenuItemData(element);

Ditto, let's create this on the stack instead.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1615
> +    TouchBarMenuData m_currTouchBarMenuData;

We generally use full words instead of abbreviations, so this should probably be m_currentTouchBarMenuData; (you can refer to WebKit style guidelines for more information: https://webkit.org/code-style-guidelines/)

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #10)
> Comment on attachment 326998 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326998&action=review
> 
> > Source/WebCore/html/HTMLMenuElement.cpp:44
> > +        if (auto* page = document().page())
> > +            page->chrome().client().didInsertMenuElement(*this);
> 
> This code is almost certainly wrong. This function gets called whenever a
> this menu element is inserted into some ancestor
> regardless of whether it's in a document or not. You need to check type to
> see if it's in the document or not.

+1

> It's very important that didInsertMenuElement never tries to execute any
> scripts.

Indeed!

> Are you sure this new ChromeClient callback wouldn't do that?

The purpose of these ChromeClient callbacks should just be to grab some (non-layout-dependent) information out of the element and send it over to the UI process, so I think we should be okay...we just need to be careful we don't ask the HTMLMenuElement or HTMLMenuItemElement any questions that trigger layout :/

[Aishwarya Nirmal]

Created attachment 327130 [details]
Patch

[Wenson Hsieh]

Comment on attachment 327130 [details]
Patch

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

> Source/WebCore/html/HTMLMenuElement.cpp:69
> +    if (!RuntimeEnabledFeatures::sharedFeatures().menuItemElementEnabled())

I think we'd still want to defer to the superclass' implementation by calling HTMLElement::parseAttribute(name, value), in the case where the menu item element isn't enabled...unless there's good reason we should skip the super call in this case?

If not, let's fold this `!RuntimeEnabledFeatures::sharedFeatures().menuItemElementEnabled()` check in with the early return above, so we bail `if (name != typeAttr || !RuntimeEnabledFeatures::sharedFeatures().menuItemElementEnabled())`.

> Source/WebCore/html/HTMLMenuElement.h:33
> +    InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) final;

We usually make these private instead of public.

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

Nit - We don't need to explicitly initialize this, since it'll be initialized to an empty vector by default.

> Source/WebKit/Shared/TouchBarMenuData.cpp:42
> +    , m_isPageCustomized(false)

Nit - We typically initialize these in the header file (e.g. bool m_isPageCustomized { false };)

> Source/WebKit/Shared/TouchBarMenuData.cpp:65
> +void TouchBarMenuData::addMenuItem(const TouchBarMenuItemData data)

const reference! (const TouchBarMenuItemData&)

> Source/WebKit/Shared/TouchBarMenuData.cpp:70
> +void TouchBarMenuData::removeMenuItem(const TouchBarMenuItemData data)

This should be a const reference (const TouchBarMenuItemData&)

> Source/WebKit/Shared/TouchBarMenuItemData.cpp:62
> +    : itemType(ItemType::Button)

These should be initialized in the header (e.g. ItemType itemType { ItemType::Button };)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5382
> +    send(Messages::WebPageProxy::TouchBarMenuDataChanged(temp), pageID(), IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);

I don't think we'll need to use the DispatchMessageEvenWhenWaitingForSyncReply option for these IPC message sends...

[Aishwarya Nirmal]

Created attachment 327138 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327176 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327180 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327196 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327207 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327211 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 327211 [details]
Patch

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

> Source/WebCore/html/HTMLMenuElement.cpp:43
> +#if PLATFORM(COCOA)

I don't think we want to guard this behind PLATFORM(COCOA).
There is no reason these code need to be COCOA specific beyond menuItemElementEnabled() check.
In general, we should try avoiding adding if PLATFORM(~) in WebCore
because it increases the cognitive stress for people reading the code.
It's much simpler to see the code as a runtime enabled feature.

> Source/WebCore/html/HTMLMenuElement.cpp:75
> +            for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +                if (!is<HTMLMenuItemElement>(node))

Use childrenOfType<HTMLMenuItemElement> instead.

> Source/WebCore/html/HTMLMenuItemElement.cpp:50
> +    if (type.connectedToDocument) {
> +        if (auto* page = document().page())

Don't we need to check that the parent node is a HTMLMenuElement here?

> Source/WebCore/html/HTMLMenuItemElement.cpp:60
> +        if (auto* page = document().page())

Do we need to check that the parent was a HTMLMenuElement here?

> Source/WebKit/Shared/TouchBarMenuData.h:44
> +    TouchBarMenuData();
> +    TouchBarMenuData(WebCore::HTMLMenuElement&);
> +    TouchBarMenuData(const TouchBarMenuData&);

Please make these constructors explicit.
Otherwise, we may accidentally create TouchBarMenuData from HTMLMenuElement/TouchBarMenuData in implicit coercion.

> Source/WebKit/Shared/TouchBarMenuData.h:46
> +    WEBCORE_EXPORT ~TouchBarMenuData();

As I stated in the previous patch, I don't think there is any need to define an explicit destructor.
Inlining it here should work just fine.

> Source/WebKit/Shared/TouchBarMenuItemData.h:55
> +    TouchBarMenuItemData();
> +    TouchBarMenuItemData(WebCore::HTMLMenuItemElement&);
> +    TouchBarMenuItemData(const TouchBarMenuItemData&);

Ditto to make these constructors explicit.

[Wenson Hsieh]

Comment on attachment 327211 [details]
Patch

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

>> Source/WebCore/html/HTMLMenuElement.cpp:43
>> +#if PLATFORM(COCOA)
> 
> I don't think we want to guard this behind PLATFORM(COCOA).
> There is no reason these code need to be COCOA specific beyond menuItemElementEnabled() check.
> In general, we should try avoiding adding if PLATFORM(~) in WebCore
> because it increases the cognitive stress for people reading the code.
> It's much simpler to see the code as a runtime enabled feature.

I think making just the touch-bar-specific bits in WebKit fall under PLATFORM(MAC) or PLATFORM(COCOA) is sensible. I agree, the menu/menuitem element code in WebCore should definitely be platform agnostic.

>> Source/WebKit/Shared/TouchBarMenuData.h:46
>> +    WEBCORE_EXPORT ~TouchBarMenuData();
> 
> As I stated in the previous patch, I don't think there is any need to define an explicit destructor.
> Inlining it here should work just fine.

Also, I don't think we need to export this destructor.

[Aishwarya Nirmal]

Created attachment 327686 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327693 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327700 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327709 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327710 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327712 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327720 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327765 [details]
Patch

[Aishwarya Nirmal]

Created attachment 327780 [details]
Patch

[Wenson Hsieh]

Comment on attachment 327780 [details]
Patch

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

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

This seems like this should be const Vector<TouchBarMenuItemData>&.

> Source/WebKit/Shared/TouchBarMenuItemData.h:59
> +    String customizationLabel;

The customizationLabel is the text that's shown when customizing the items on the touch bar via View > Customize Touch Bar…. It looks like this is set to the value of the menu item's title attribute, however — perhaps this could just be label?

> Source/WebKit/Shared/TouchBarMenuItemData.h:60
> +    float priority { 0.0 };

I would prefer to pare down this list of member variables, so that we only add what we'll need in this patch and the next.

[Aishwarya Nirmal]

Created attachment 327868 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 327868 [details]
Patch

Clearing flags on attachment: 327868

Committed r225438: <https://trac.webkit.org/changeset/225438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35810449>

[Daniel Bates]

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.

[Wenson Hsieh]

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

[Wenson Hsieh]

Comment on attachment 327868 [details]
Patch

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

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

Aishwarya is planning to make use of it in a followup, though this may change. This is a work in progress patch, so there are going to be a few methods and interfaces we define and implement but don't use until the next patch in the sequence.