<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mrobinson@webkit.org" title="Martin Robinson <mrobinson@webkit.org>"> <span class="fn">Martin Robinson</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - REGRESSION(r192247): [GTK] ASSERTION FAILED: type == WebCore::ActionType || type == WebCore::CheckableActionType || type == WebCore::SeparatorType"
href="https://bugs.webkit.org/show_bug.cgi?id=151513">bug 151513</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #266243 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - REGRESSION(r192247): [GTK] ASSERTION FAILED: type == WebCore::ActionType || type == WebCore::CheckableActionType || type == WebCore::SeparatorType"
href="https://bugs.webkit.org/show_bug.cgi?id=151513#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - REGRESSION(r192247): [GTK] ASSERTION FAILED: type == WebCore::ActionType || type == WebCore::CheckableActionType || type == WebCore::SeparatorType"
href="https://bugs.webkit.org/show_bug.cgi?id=151513">bug 151513</a>
from <span class="vcard"><a class="email" href="mailto:mrobinson@webkit.org" title="Martin Robinson <mrobinson@webkit.org>"> <span class="fn">Martin Robinson</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=266243&action=diff" name="attach_266243" title="Patch">attachment 266243</a> <a href="attachment.cgi?id=266243&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=266243&action=review">https://bugs.webkit.org/attachment.cgi?id=266243&action=review</a>
<span class="quote">>>> 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</span >
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?
<span class="quote">>>> 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.</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>