Bug 189308 – Web Inspector: REGRESSION: breakpoint context menu appears twice in DOM tree

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Created attachment 348933 [details]
[Image] Screenshot of Issue

.

[Devin Rousso]

Created attachment 348934 [details]
Patch

[Blaze Burg]

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

[Devin Rousso]

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.

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 349741 [details]
Patch

[Joseph Pecoraro]

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

[Devin Rousso]

Created attachment 349859 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 349859 [details]
Patch

Clearing flags on attachment: 349859

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44490747>