Bug 28908 – Support conditional breakpoints in the frontend

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

This bug is to track the conditional breakpoints implementation in the Web
Inspector frontend.

The parent bug for conditional breakpoints is
https://bugs.webkit.org/show_bug.cgi?id=21449

[Alexander Pavlov (apavlov)]

Created attachment 38993 [details]
patch

This patch uses a workaround for https://bugs.webkit.org/show_bug.cgi?id=28913 (alternative popup parenting/layout)

[Timothy Hatcher]

Can you attach a screenshot of the new layout?

[Alexander Pavlov (apavlov)]

Created attachment 39422 [details]
Popup layout inside source frame

The new layout is only slightly different from the original one: the popup is bound by the source frame rather than the entire WebInspector window.

[Timothy Hatcher]

Comment on attachment 38993 [details]
patch


> +    this._document = element ? element.ownerDocument : undefined;
> +    this._contentElement = element;
> +    this._glasspaneElement = undefined;
> +    this._keyHandler = this._keyEventHandler.bind(this);
> +    this._mouseDownHandler = this._mouseDownEventHandler.bind(this);
> +    this._anchorElement = undefined;
> +    this._visible = false;
> +    this._autoHide = true;
> +    this._position = {x: 0, y: 0};

You don't need to explicitly set things to undefined. They will be undefined by default. Either use null or remove them. I don't think you should keep a reference to the document either, just ask the element when you need it.

> +        var doc = this._document;
> +        if (!doc)
> +            return;

Don't abbrivate doc, "ownerDocument" would be best.

> +        this._setDisplayed(this._contentElement, false);
> +        doc.body.appendChild(this._contentElement);
> +        this._contentElement.style.visibility = "hidden";
> +        this._contentElement.positionAt(0, 0);
> +        this._setDisplayed(this._contentElement, true);
> +        this.positionElement();
> +        this._contentElement.style.visibility = "visible";

Why do you do this hidden then visible dance? Nothing is rendered until JavaScript finishes execution, so you just need to make it visible. You shouldn't need any of the _setDisplayed or _contentElement.style.visibility lines.

Also, if you do need to toggle visibility, that should be done with a style class not a direct manipulation of the style. Only variable things like position should be set on the style.

> +            this._contentElement.style.visibility = "hidden";
> +            this._setDisplayed(this._contentElement, false);

This should be doen with a style class. Also why do you need to toggle both display: none and visibility: hidden? Just doing display: none on the top most element is sufficient. (Again do that with a style class change. You can use our existing "hidden" class for that.)

> +        this._document = x ? x.ownerDocument : undefined;

Don't reference the document. Just use the this._contentElement.ownerDocument when you need the document. (Which you already do in _positionAtAnchor.)

> +    positionElement: function()
> +    {
> +        this._positionAtAnchor();
> +    },

You should just put the implementation of _positionAtAnchor in positionElement, since it is only called from here.

> +        var anchorPos = {x: anchorElement.totalOffsetLeft, y: anchorElement.totalOffsetTop};
> +        anchorPos = this._pointToFrame(anchorPos, anchorElement.ownerDocument, element.ownerDocument);
> +        var anchorBox = {x: anchorPos.x, y: anchorPos.y, width: anchorElement.offsetWidth, height: anchorElement.offsetHeight};
> +        var elementBox = {x: element.totalOffsetLeft, y: element.totalOffsetTop, width: element.offsetWidth, height: element.offsetHeight};
> +        var newElementPos = {x: 0, y: 0};

Don't abbrivate "pos".

> +    _setDisplayed: function(element, isDisplayed)
> +    {
> +        element.style.display = isDisplayed ? "block" : "none";
> +    },

You should be able to remove this based on my earlier comments.

> +    _mouseDownEventHandler: function(event)
> +    {
> +        if (this._autoHide) {
> +            var popupBox = {x: this._contentElement.totalOffsetLeft, y: this._contentElement.totalOffsetTop, width: this._contentElement.offsetWidth, height: this._contentElement.offsetHeight};
> +            if (!this._rectangleContains(popupBox, {x: event.clientX, y: event.clientY}))
> +                this.hide();
> +        }
> +    },

You should be able to call hide() when event.originalTarget === this._glasspaneElement and remove all the bounding box stuff.

> +    _pointToFrame: function(point, originalDoc, targetDoc)
> +    {
> +        var result = {x: point.x, y: point.y};
> +        if (originalDoc != targetDoc) {
> +            var originalBody = originalDoc.body;
> +            var targetWindow = targetDoc.defaultView;
> +            var offset = originalBody.offsetRelativeToWindow(targetWindow);
> +            result.x += offset.x - originalBody.totalOffsetLeft;
> +            result.y += offset.y - originalBody.totalOffsetTop;
> +        }
> +        return result;
> +    },

Why do you need this? Isn't the popup in the same frame?

> +    _rectangleContains: function(rect, point)
> +    {
> +        return point.x >= rect.x && point.x <= (rect.x + rect.height) && point.y >= rect.y && point.y <= (rect.y + rect.height);
> +    }
> +}

Should be able to remove based on my earlier comment.

> +    this._popup = undefined;

Undefined is the default, remove this.


> +        if (event.button != 0 || event.altKey || event.ctrlKey || event.metaKey || event.shiftKey) {
> +            return;
> +        }

No need for the braces, per our style guide.

> +        var conditionElement = popupDoc.getElementById("bp-condition");

You should have another way to get this element. Using getElementById is fraggle.


> +        var labelElement = document.createElement("div");
> +        labelElement.className = "popup-message";
> +        labelElement.style.margin = "0 0 2px 0";
> +        labelElement.innerText = WebInspector.UIString("The breakpoint on line %d will stop only if this expression is true:", lineNumber);
> +        popupContentElement.appendChild(labelElement);

Consider using the <label> element. Also the margin should be in inspector.css, maybe with a enw class you use.


> +.breakpoint-list a.breakpoint-has-condition::after {
> +    content: " (?)";
> +}

What is this for?

> -        this.innerText = oldText;
> +        if (this.tagName === "INPUT" && this.type === "text")
> +            this.value = oldText;
> +        else
> +            this.innerText = oldText;

This changed in TOT to use textContent instead of innerText. You will need to merge with that.

> +Element.prototype.offsetRelativeToWindow = function(targetWindow)
> +{
> +    var elementOffset = [0, 0];
> +    var curElement = this;
> +    var curWindow = this.ownerDocument.defaultView;
> +    while (curWindow && curElement) {
> +        var pageOffset;
> +        if (curWindow === targetWindow) {
> +            pageOffset = [curElement.totalOffsetLeft, curElement.totalOffsetTop];
> +        } else {
> +            var clientPos = curElement.getBoundingClientRect();
> +            pageOffset = [clientPos.left, clientPos.top];
> +        }
> +        elementOffset[0] += pageOffset[0];
> +        elementOffset[1] += pageOffset[1];
> +
> +        if (!curWindow || curWindow === targetWindow)
> +            break;
> +
> +        curElement = curWindow.frameElement;
> +        curWindow = curWindow.parent;
> +    }
> +
> +    return elementOffset;
> +}

I think you can simplify this by using offsetTop and offsetLeft and walk the offsetParent chain (like totalOffsetLeft). This should also return an object not an array. So {x: nn, y: nn}.

[Alexander Pavlov (apavlov)]

Created attachment 39545 [details]
patch (fixed)

This patch version fixes the nits and enables condition editing for disabled breakpoints.

[Alexander Pavlov (apavlov)]

(In reply to comment #4)
> (From update of attachment 38993 [details])
> 
> > +    this._document = element ? element.ownerDocument : undefined;
> > +    this._contentElement = element;
> > +    this._glasspaneElement = undefined;
> > +    this._keyHandler = this._keyEventHandler.bind(this);
> > +    this._mouseDownHandler = this._mouseDownEventHandler.bind(this);
> > +    this._anchorElement = undefined;
> > +    this._visible = false;
> > +    this._autoHide = true;
> > +    this._position = {x: 0, y: 0};
> 
> You don't need to explicitly set things to undefined. They will be undefined by
> default. Either use null or remove them. I don't think you should keep a
Sorry, I wrongfully assumed it was necessary to initialize fields with WebKit.

> reference to the document either, just ask the element when you need it.
Done.

> > +        var doc = this._document;
> > +        if (!doc)
> > +            return;
> 
> Don't abbrivate doc, "ownerDocument" would be best.
Done.

> > +        this._setDisplayed(this._contentElement, false);
> > +        doc.body.appendChild(this._contentElement);
> > +        this._contentElement.style.visibility = "hidden";
> > +        this._contentElement.positionAt(0, 0);
> > +        this._setDisplayed(this._contentElement, true);
> > +        this.positionElement();
> > +        this._contentElement.style.visibility = "visible";
> 
> Why do you do this hidden then visible dance? Nothing is rendered until
> JavaScript finishes execution, so you just need to make it visible. You
Thanks for the hint!

> shouldn't need any of the _setDisplayed or _contentElement.style.visibility
> lines.
We might need the two that make the element visible, since the element could have been created with its visibility/display turned off. Added a JsDoc.

> Also, if you do need to toggle visibility, that should be done with a style
> class not a direct manipulation of the style. Only variable things like
> position should be set on the style.
> 
> > +            this._contentElement.style.visibility = "hidden";
> > +            this._setDisplayed(this._contentElement, false);
> 
> This should be doen with a style class. Also why do you need to toggle both
> display: none and visibility: hidden? Just doing display: none on the top most
> element is sufficient. (Again do that with a style class change. You can use
> our existing "hidden" class for that.)
> 
> > +        this._document = x ? x.ownerDocument : undefined;
> 
> Don't reference the document. Just use the this._contentElement.ownerDocument
> when you need the document. (Which you already do in _positionAtAnchor.)
Done.

> > +    positionElement: function()
> > +    {
> > +        this._positionAtAnchor();
> > +    },
> 
> You should just put the implementation of _positionAtAnchor in positionElement,
> since it is only called from here.
Done.

> > +        var anchorPos = {x: anchorElement.totalOffsetLeft, y: anchorElement.totalOffsetTop};
> > +        anchorPos = this._pointToFrame(anchorPos, anchorElement.ownerDocument, element.ownerDocument);
> > +        var anchorBox = {x: anchorPos.x, y: anchorPos.y, width: anchorElement.offsetWidth, height: anchorElement.offsetHeight};
> > +        var elementBox = {x: element.totalOffsetLeft, y: element.totalOffsetTop, width: element.offsetWidth, height: element.offsetHeight};
> > +        var newElementPos = {x: 0, y: 0};
> 
> Don't abbrivate "pos".
Done.

> > +    _setDisplayed: function(element, isDisplayed)
> > +    {
> > +        element.style.display = isDisplayed ? "block" : "none";
> > +    },
> 
> You should be able to remove this based on my earlier comments.
Done.

> > +    _mouseDownEventHandler: function(event)
> > +    {
> > +        if (this._autoHide) {
> > +            var popupBox = {x: this._contentElement.totalOffsetLeft, y: this._contentElement.totalOffsetTop, width: this._contentElement.offsetWidth, height: this._contentElement.offsetHeight};
> > +            if (!this._rectangleContains(popupBox, {x: event.clientX, y: event.clientY}))
> > +                this.hide();
> > +        }
> > +    },
> 
> You should be able to call hide() when event.originalTarget ===
> this._glasspaneElement and remove all the bounding box stuff.
Correct, done.

> > +    _pointToFrame: function(point, originalDoc, targetDoc)
> > +    {
> > +        var result = {x: point.x, y: point.y};
> > +        if (originalDoc != targetDoc) {
> > +            var originalBody = originalDoc.body;
> > +            var targetWindow = targetDoc.defaultView;
> > +            var offset = originalBody.offsetRelativeToWindow(targetWindow);
> > +            result.x += offset.x - originalBody.totalOffsetLeft;
> > +            result.y += offset.y - originalBody.totalOffsetTop;
> > +        }
> > +        return result;
> > +    },
> 
> Why do you need this? Isn't the popup in the same frame?
It is, as long as a workaround for https://bugs.webkit.org/show_bug.cgi?id=28913 is in place. I still hope to move the popup into the topmost frame when the bug is fixed. Removed for now.

> > +    _rectangleContains: function(rect, point)
> > +    {
> > +        return point.x >= rect.x && point.x <= (rect.x + rect.height) && point.y >= rect.y && point.y <= (rect.y + rect.height);
> > +    }
> > +}
> Should be able to remove based on my earlier comment.
Done.

> 
> > +    this._popup = undefined;
> 
> Undefined is the default, remove this.
Done.

> > +        if (event.button != 0 || event.altKey || event.ctrlKey || event.metaKey || event.shiftKey) {
> > +            return;
> > +        }
> No need for the braces, per our style guide.
Done.

> > +        var conditionElement = popupDoc.getElementById("bp-condition");
> 
> You should have another way to get this element. Using getElementById is
> fraggle.
Done.

> 
> > +        var labelElement = document.createElement("div");
> > +        labelElement.className = "popup-message";
> > +        labelElement.style.margin = "0 0 2px 0";
> > +        labelElement.innerText = WebInspector.UIString("The breakpoint on line %d will stop only if this expression is true:", lineNumber);
> > +        popupContentElement.appendChild(labelElement);
> 
> Consider using the <label> element. Also the margin should be in inspector.css,
> maybe with a enw class you use.
Done.

> > +.breakpoint-list a.breakpoint-has-condition::after {
> > +    content: " (?)";
> > +}
> 
> What is this for?
This is to denote conditional breakpoints in the Breakpoints sidebar pane (a question mark after the breakpoint label). I can as well remove it if we don't want those to be displayed differently.

> > -        this.innerText = oldText;
> > +        if (this.tagName === "INPUT" && this.type === "text")
> > +            this.value = oldText;
> > +        else
> > +            this.innerText = oldText;
> 
> This changed in TOT to use textContent instead of innerText. You will need to
> merge with that.
Thanks, merged.

> > +Element.prototype.offsetRelativeToWindow = function(targetWindow)
> > +{
> > +    var elementOffset = [0, 0];
> > +    var curElement = this;
> > +    var curWindow = this.ownerDocument.defaultView;
> > +    while (curWindow && curElement) {
> > +        var pageOffset;
> > +        if (curWindow === targetWindow) {
> > +            pageOffset = [curElement.totalOffsetLeft, curElement.totalOffsetTop];
> > +        } else {
> > +            var clientPos = curElement.getBoundingClientRect();
> > +            pageOffset = [clientPos.left, clientPos.top];
> > +        }
> > +        elementOffset[0] += pageOffset[0];
> > +        elementOffset[1] += pageOffset[1];
> > +
> > +        if (!curWindow || curWindow === targetWindow)
> > +            break;
> > +
> > +        curElement = curWindow.frameElement;
> > +        curWindow = curWindow.parent;
> > +    }
> > +
> > +    return elementOffset;
> > +}
> 
> I think you can simplify this by using offsetTop and offsetLeft and walk the
> offsetParent chain (like totalOffsetLeft). This should also return an object
Right, both "this" element and iframe hierarchies...

> not an array. So {x: nn, y: nn}.
Done.

[Timothy Hatcher]

Comment on attachment 39545 [details]
patch (fixed)

> +WebInspector.Popup.prototype =
> +{

The brace for prototypes should be next to the equals on one line to match the other classes.

> +        var popupDoc = this.element.contentDocument;

We don't abbreviate Document as Doc in WebKit. This should be popupDocument.

> +.breakpoint-list a.breakpoint-has-condition::after {
> +    content: " (?)";
> +}

I now see what this is for. I don't think this is the best way to show conditional break points. I think we should show the condition somehow in the Breakpoints pane. (This also isn't localizable in CSS.)

Remove this for now and file a bug about showing them in the Breakpoints pane.

[Alexander Pavlov (apavlov)]

Created attachment 39562 [details]
patch (fixed v2)

Fixed remaining 3 nits

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> (From update of attachment 39545 [details])
> > +WebInspector.Popup.prototype =
> > +{
> 
> The brace for prototypes should be next to the equals on one line to match the
> other classes.
Sorry, I confused them a bit.
 
> > +        var popupDoc = this.element.contentDocument;
> 
> We don't abbreviate Document as Doc in WebKit. This should be popupDocument.
Oops, fixed.

> > +.breakpoint-list a.breakpoint-has-condition::after {
> > +    content: " (?)";
> > +}
> 
> I now see what this is for. I don't think this is the best way to show
> conditional break points. I think we should show the condition somehow in the
> Breakpoints pane. (This also isn't localizable in CSS.)
Yes, L10N was one of my concerns about this solution, too. I considered several approaches, but no consistent decision yet.

> Remove this for now and file a bug about showing them in the Breakpoints pane.
Filed https://bugs.webkit.org/show_bug.cgi?id=29251 - let's discuss it there.

[Anthony Ricaud]

Landed in 48392