[Webkit-unassigned] [Bug 151513] REGRESSION(r192247): [GTK] ASSERTION FAILED: type == WebCore::ActionType || type == WebCore::CheckableActionType || type == WebCore::SeparatorType

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=151513

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #266243|review?                     |review+
              Flags|                            |

--- Comment #6 from Martin Robinson <mrobinson at webkit.org> ---
Comment on attachment 266243
  --> https://bugs.webkit.org/attachment.cgi?id=266243
Patch

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

>>> Source/WebKit2/Shared/gtk/WebContextMenuItemGtk.cpp:128
>>> +    : WebContextMenuItemData(data.type() == SubmenuType ? ActionType : data.type(), data.action(), data.title(), data.enabled(), data.checked())
>> 
>> I think you should add this check to the constructor above, as well...
> 
> I don't think so, the constructor above receives a type, the caller should provide the right type. We internally don't use the submenu type,l so we only need to check it when constructing from an external source like WebContextMenuItemData, but not from our own implementation

There should probably be an assertion there instead. The assertion can also serve as a type of documentation for what is going on. Can you do that before landing?

>>> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:212
>>> +    item->priv->menuItem = std::make_unique<WebContextMenuItemGtk>(ActionType, ContextMenuItemBaseApplicationTag, String::fromUTF8(label));
>> 
>> ...so that you don't need to do this.
> 
> I prefer to make this explicit. This is building a WebContextMenuItemGtk, which doesn't use SubmenuType.

I agree with Michael here. I don't understand the motivation for having constructors that differ so much in behavior. On the other hand, I don't think it should block the patch. It's a somewhat minor point.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151201/9fc33da0/attachment.html>