[Webkit-unassigned] [Bug 28908] Support conditional breakpoints in the frontend
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 11 11:51:09 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=28908
Timothy Hatcher <timothy at hatcher.name> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #38993|review? |review-
Flag| |
--- Comment #4 from Timothy Hatcher <timothy at hatcher.name> 2009-09-11 11:51:09 PDT ---
(From update of attachment 38993)
> + 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}.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list