[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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #326998|review?                     |review-
              Flags|                            |

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

-- 
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/20171115/cc1847fd/attachment.html>