[Webkit-unassigned] [Bug 28908] Support conditional breakpoints in the frontend
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 14 08:30:54 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=28908
--- Comment #6 from apavlov at chromium.org 2009-09-14 08:30:52 PDT ---
(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.
--
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