[webkit-reviews] review denied: [Bug 179714] [Touch Bar Web API] Object representing Touch Bar Menu to send between Web and UI Processes : [Attachment 326998] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Aishwarya Nirmal
<anirmal at apple.com>'s request for review:
Bug 179714: [Touch Bar Web API] Object representing Touch Bar Menu to send
between Web and UI Processes
https://bugs.webkit.org/show_bug.cgi?id=179714

Attachment 326998: Patch

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




--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 326998
  --> https://bugs.webkit.org/attachment.cgi?id=326998
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.