[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