Bug 189308

Created attachment 348934 [details]
Patch

Comment on attachment 348934 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:262
> +            excludeBreakpointItems: treeElement.statusImageElement.contains(event.target),

Can you explain this logic a little bit? Or, the regression...

Comment on attachment 348934 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:262
>> +            excludeBreakpointItems: treeElement.statusImageElement.contains(event.target),
> 
> Can you explain this logic a little bit? Or, the regression...

If you contextmenu on the breakpoint icon that's shown in the DOM tree, a few different items get added to the contextmenu:

```
Break on... >
----------
Disable Breakpoints
Delete Breakpoints
```

The `WI.DOMTreeOutline`, however, also tries to add some contextmenu items, since it also always wants to provide a way to set breakpoints on a DOM node.  As such, an additional item gets added:

```
Break on... >
----------
Disable Breakpoints
Delete Breakpoints
----------
Break on... >
```

Notice that the second "Break on... >" gets added by the `WI.DOMTreeOutline`.  This patch just detects if the contextmenu came from the breakpoint icon in the DOM tree, and if so, prevents the `WI.DOMTreeOutline` from adding it's own breakpoint items.

Comment on attachment 348934 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:262
>>> +            excludeBreakpointItems: treeElement.statusImageElement.contains(event.target),
>> 
>> Can you explain this logic a little bit? Or, the regression...
> 
> If you contextmenu on the breakpoint icon that's shown in the DOM tree, a few different items get added to the contextmenu:
> 
> ```
> Break on... >
> ----------
> Disable Breakpoints
> Delete Breakpoints
> ```
> 
> The `WI.DOMTreeOutline`, however, also tries to add some contextmenu items, since it also always wants to provide a way to set breakpoints on a DOM node.  As such, an additional item gets added:
> 
> ```
> Break on... >
> ----------
> Disable Breakpoints
> Delete Breakpoints
> ----------
> Break on... >
> ```
> 
> Notice that the second "Break on... >" gets added by the `WI.DOMTreeOutline`.  This patch just detects if the contextmenu came from the breakpoint icon in the DOM tree, and if so, prevents the `WI.DOMTreeOutline` from adding it's own breakpoint items.

Seems this could be done in another way.

We could put a symbol / flag on the ContextMenu to say whether or not we've added breakpoint items:

    appendBreakpointItems(contextMenu, ...)
    {
        if (contextMenu.__hasBreakpointItems)
            return;
        contextMenu.__hasBreakpointItems = true;
        ...
    }

Then no matter how many times this gets called, we only add breakpoint items once.

Created attachment 349741 [details]
Patch

Comment on attachment 349741 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/DOMBreakpointTreeController.js:52
> +        if (contextMenu[WI.DOMBreakpointTreeController._contextMenuItemsAddedSymbol])
> +            return;
> +
> +        contextMenu[WI.DOMBreakpointTreeController._contextMenuItemsAddedSymbol] = true;

I still prefer the double underscore approach. It's just cheaper (a symbol doesn't need to exist at all times) and simpler in my opinion (no need to jump around code and its easy to read).

Created attachment 349859 [details]
Patch

Comment on attachment 349859 [details]
Patch

Clearing flags on attachment: 349859

Committed r236033: <https://trac.webkit.org/changeset/236033>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/44490747>