[webkit-reviews] review granted: [Bug 41620] Web Inspector: preserve scroll positions in source frame when switching between panes. : [Attachment 60549] [PATCH] Proposed fix.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 5 10:38:31 PDT 2010
Joseph Pecoraro <joepeck at webkit.org> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 41620: Web Inspector: preserve scroll positions in source frame when
switching between panes.
https://bugs.webkit.org/show_bug.cgi?id=41620
Attachment 60549: [PATCH] Proposed fix.
https://bugs.webkit.org/attachment.cgi?id=60549&action=review
------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> +++ b/WebCore/inspector/front-end/SourceFrame.js
> + if (visible) {
> + if (this._textViewer && this._scrollTop)
> + this._textViewer.element.scrollTop = this._scrollTop;
> + if (this._textViewer && this._scrollLeft)
> + this._textViewer.element.scrollLeft = this._scrollLeft;
> + } else {
You could have prevented a duplicate check by doing an if (this._textViewer)
block like you already do in the else.
> this._hidePopup();
> - if (this._textViewer)
> + if (this._textViewer) {
> + this._scrollTop = this._textViewer.element.scrollTop;
> + this._scrollLeft = this._textViewer.element.scrollLeft;
> this._textViewer.freeCachedElements();
> + }
There is a call to this._createViewerIfNeeded above this if/else. I
think that would have created a viewer in this case so that this
check wouldn't be necessary, but I think its good to be safe.
Thanks!
More information about the webkit-reviews
mailing list