Bug 132112 - Web Inspector: Selecting frame in debugger sidebar doesn't reveal code
Summary: Web Inspector: Selecting frame in debugger sidebar doesn't reveal code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://graougraou.com/tests/inspector...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-24 01:02 PDT by Antoine Quint
Modified: 2014-05-08 15:09 PDT (History)
6 users (show)

See Also:


Attachments
Behaviour in Safari 7.0.3 (849.91 KB, video/quicktime)
2014-04-24 01:02 PDT, Antoine Quint
no flags Details
Behaviour in WebKit nightly r167733 (2.38 MB, video/quicktime)
2014-04-24 01:03 PDT, Antoine Quint
no flags Details
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter (4.43 KB, patch)
2014-05-07 15:59 PDT, jonowells
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 1 (4.46 KB, patch)
2014-05-08 11:49 PDT, jonowells
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2014-04-24 01:02:55 PDT
Created attachment 230060 [details]
Behaviour in Safari 7.0.3

In WebKit nightlies, when pausing in the debugger, selecting a frame, any frame, in the debugger sidebar doesn't reveal the expected code in the content view. See URL for a test that automatically breaks in the debugger when loaded and attached movies showing the behaviour in Safari 7.0.3 and Nightly r167733 on 10.9.3.
Comment 1 Antoine Quint 2014-04-24 01:03:59 PDT
Created attachment 230061 [details]
Behaviour in WebKit nightly r167733
Comment 2 Radar WebKit Bug Importer 2014-04-24 01:04:11 PDT
<rdar://problem/16709980>
Comment 3 Timothy Hatcher 2014-04-24 07:31:35 PDT
Dupe of <rdar://problem/16491500>.
Comment 4 jonowells 2014-05-07 15:59:11 PDT
Created attachment 231026 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter
Comment 5 Brian Burg 2014-05-08 11:12:32 PDT
Comment on attachment 231026 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter

View in context: https://bugs.webkit.org/attachment.cgi?id=231026&action=review

This seems good to me, since we shouldn't assume that the navigation sidebar has only one outline (or that its selection is within the content outline).

I think you need to rebase the patch.

> Source/WebInspectorUI/UserInterface/Base/Main.js:-1838
> -

Are you adding or removing trailing whitespace here?
Comment 6 Joseph Pecoraro 2014-05-08 11:30:07 PDT
Comment on attachment 231026 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter

View in context: https://bugs.webkit.org/attachment.cgi?id=231026&action=review

r=me with some comments

> Source/WebInspectorUI/ChangeLog:8
> +        `WebInspector._revealAndSelectRepresentedObjectInNavigationSidebar` was checking

No need for all the backticks here.

> Source/WebInspectorUI/UserInterface/Base/Main.js:925
> +    if (!selectedSidebarPanel.elementIsSelected)

I think the name "elementIsSelected" is weird. Maybe "hasSelectedElement" or "hasSelection"?

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:132
> +    get elementIsSelected() {

Style: Brace on newline.
Comment 7 jonowells 2014-05-08 11:49:25 PDT
Created attachment 231085 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 1
Comment 8 Joseph Pecoraro 2014-05-08 12:03:42 PDT
Comment on attachment 231085 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 1

r=me
Comment 9 WebKit Commit Bot 2014-05-08 12:40:54 PDT
Comment on attachment 231085 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 1

Clearing flags on attachment: 231085

Committed r168484: <http://trac.webkit.org/changeset/168484>
Comment 10 WebKit Commit Bot 2014-05-08 12:40:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Timothy Hatcher 2014-05-08 15:09:07 PDT
Comment on attachment 231085 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 1

View in context: https://bugs.webkit.org/attachment.cgi?id=231085&action=review

> Source/WebInspectorUI/ChangeLog:10
> +        base getter elementIsSelected to NavigationSidebarPanel and extended it in 

"getter" should be "function" with the change below.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:135
> +    get hasSelectedElement()
> +    {
> +        return !!this._breakpointsContentTreeOutline.selectedTreeElement || !!this._callStackContentTreeOutline.selectedTreeElement;
> +    },

We don't use verb prefixes with getters. This should be a plain function.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:150
> +    get hasSelectedElement()

Ditto.