Bug 127328 – Web Inspector: Move the Popover code out of the Breakpoint model object

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

The popover and DOM code in Breakpoint.js should move out and into a new class. This is a layering violation. Maybe BreakpointEditingController?

[Radar WebKit Bug Importer]

<rdar://problem/16070891>

[Timothy Hatcher]

The context menu code should move too.

[Matt Baker]

Created attachment 260275 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 260275 [details]
[Patch] Proposed Fix

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

Better, but I think this needs to be a controller that owns the Popover.

> Source/WebInspectorUI/UserInterface/Views/BreakpointPopoverView.js:117
> +            let popover = new WebInspector.Popover();
> +            let popoverView = new WebInspector.BreakpointPopoverView(breakpoint, popover);
> +            popover.delegate = popoverView;
> +            popover.content = popoverView.element;

It is a bit odd that BreakpointPopoverView takes the popover and then popover.content and popover.delegate end up pointing to popoverView.

That makes BreakpointPopoverView more of a controller. That would likely let you internalize this.

> Source/WebInspectorUI/UserInterface/Views/BreakpointPopoverView.js:297
> +WebInspector.BreakpointPopoverView.PopoverClassName = "edit-breakpoint-popover-content";

Any single use class names we have just been inlining them as strings.

[Matt Baker]

(In reply to comment #4)
> Comment on attachment 260275 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260275&action=review
> 
> Better, but I think this needs to be a controller that owns the Popover.
> 
> > Source/WebInspectorUI/UserInterface/Views/BreakpointPopoverView.js:117
> > +            let popover = new WebInspector.Popover();
> > +            let popoverView = new WebInspector.BreakpointPopoverView(breakpoint, popover);
> > +            popover.delegate = popoverView;
> > +            popover.content = popoverView.element;
> 
> It is a bit odd that BreakpointPopoverView takes the popover and then
> popover.content and popover.delegate end up pointing to popoverView.
> 
> That makes BreakpointPopoverView more of a controller. That would likely let
> you internalize this.

Yeah this is pretty awkward. A controller class is better suited to this.

[Matt Baker]

Created attachment 260317 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 260317 [details]
[Patch] Proposed Fix

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

Nice!

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:50
> +        this._breakpointPopoverController = new WebInspector.BreakpointPopoverController();

Drop the ().

[Matt Baker]

Created attachment 260322 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 260322 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 260322

Committed r189189: <http://trac.webkit.org/changeset/189189>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.