Bug 135311 - Web Inspector: shown() called on a content view when stepping over an instruction in the debugger
Summary: Web Inspector: shown() called on a content view when stepping over an instruc...
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:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-25 16:19 PDT by Saam Barati
Modified: 2014-08-05 11:30 PDT (History)
6 users (show)

See Also:


Attachments
Idea for patch (1.30 KB, patch)
2014-07-29 19:28 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
another patch proposition (3.56 KB, patch)
2014-07-30 10:59 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (5.17 KB, patch)
2014-08-04 19:04 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2014-07-25 16:19:57 PDT
shown() is called on a ContentView every time the debugger steps over the instruction.

Do we want to keep these semantics? If so, I'd argue it's apt to rename this function because it doesn't necessarily match when the screen is actually 'shown'.
Or maybe, we should prevent screens that are already visible from having 'shown' called on them.

The origin of the shown() is probably caused by an event from DebuggerManager every time it steps over a function.
Comment 1 Radar WebKit Bug Importer 2014-07-25 16:20:11 PDT
<rdar://problem/17817144>
Comment 2 Timothy Hatcher 2014-07-26 09:12:33 PDT
It wasn't intended to be called multiple times.
Comment 3 Saam Barati 2014-07-29 10:56:19 PDT
It seems like the main problem is that when stepping over an instruction, the debugger on the JSC side skips to the next instruction, and then emits the paused event. When the Inspector sees this pause event, it adds a new backForwardEntry and shows that new backForwardEntry, even if the content view it shows is for the same Resource. So, if stepping over every instruction in a JS file, there will be backForwardEntries for every instruction that's been stepped over. So clicking the back button at the top of the content browser will just show older content views that are really just the same file that we've been stepping over instructions within that file. The problem, then, doesn't seem to be that shown() is called multiple times on the same content view (because new content views for the same resource are repeatedly created), but rather, that we create new content views for every instruction we step over. This also seems bad from the user experience point of view of stepping over ten instructions, and then wanting to click back to go back to the previous JS file, and having to click back 10 times just to skip over these 10 content views that were created for each pause event.

I'm not sure what the correct solution for this is, but I think the way it is set up now isn't the correct architecture. 

What do you all think?
Comment 4 Timothy Hatcher 2014-07-29 11:05:18 PDT
The back/forward list is not just resources (views) but also scroll positions and other cookie data.

So the back button should be jumping around in the view to previous scroll positions. This is how Xcode behaves.

shown() should still probably not be called if the view does not change.
Comment 5 Timothy Hatcher 2014-07-29 11:06:38 PDT
We probably should not push new back forward entries for each pause/resume if the scroll position didn't change.
Comment 6 Saam Barati 2014-07-29 11:09:21 PDT
(In reply to comment #5)
> We probably should not push new back forward entries for each pause/resume if the scroll position didn't change.

Yeah, that seems reasonable to me. I think saving them for each pause/resume is overkill when stepping over instructions.
Comment 7 Timothy Hatcher 2014-07-29 11:12:33 PDT
It looks like this code is at fault:

        if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie))
            return currentEntry.contentView;


It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions.
Comment 8 Joseph Pecoraro 2014-07-29 11:14:20 PDT
(In reply to comment #3)
> The problem, then, doesn't seem to be that shown() is called multiple times on the same content view (because new content views for the same resource are repeatedly created), but rather, that we create new content views for every instruction we step over.

Do we really create a new ContentView?


> This also seems bad from the user experience point of view of stepping over ten instructions, and then wanting to click back to go back to the previous JS file, and having to click back 10 times just to skip over these 10 content views that were created for each pause event.

That sounds unintentional. I think the back forward list should be for specific user events.

    1. Showing a ContentView, by clicking a Script/Resource Tree Element in the source sidebar
    2. Jumping to a location by clicking a Breakpoint Tree Element in the debugger sidebar
    3. Jumping to a location by clicking a CallFrame Tree Element in the debugger sidebar
    4. Jumping to a location by clicking a (source:line:column) link anywhere in the UI
    5. Hiding a ContentView, this may want to add a new entry or replace the current entry

I think stepping in the debugger it would be unexpected that that adds a new back/forward entry other than the possibility of normal navigation with (1) or (5).
Comment 9 Joseph Pecoraro 2014-07-29 11:15:00 PDT
(In reply to comment #8)
> the source sidebar

Resource* sidebar
Comment 10 Saam Barati 2014-07-29 11:16:13 PDT
(In reply to comment #7)
> It looks like this code is at fault:
> 
>         if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie))
>             return currentEntry.contentView;
> 
> 
> It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions.

Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails.
Comment 11 Timothy Hatcher 2014-07-29 11:17:22 PDT
(In reply to comment #10)
> (In reply to comment #7)
> > It looks like this code is at fault:
> > 
> >         if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie))
> >             return currentEntry.contentView;
> > 
> > 
> > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions.
> 
> Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails.

Three times sounds like two too many.
Comment 12 Saam Barati 2014-07-29 11:19:57 PDT
(In reply to comment #8)
> 
> Do we really create a new ContentView?
> 

I'm not 100% sure on this. I think the more accurate statement is new BackForwardEntries are created.

> 
> That sounds unintentional. I think the back forward list should be for specific user events.
> 
>     1. Showing a ContentView, by clicking a Script/Resource Tree Element in the source sidebar
>     2. Jumping to a location by clicking a Breakpoint Tree Element in the debugger sidebar
>     3. Jumping to a location by clicking a CallFrame Tree Element in the debugger sidebar
>     4. Jumping to a location by clicking a (source:line:column) link anywhere in the UI
>     5. Hiding a ContentView, this may want to add a new entry or replace the current entry
> 
> I think stepping in the debugger it would be unexpected that that adds a new back/forward entry other than the possibility of normal navigation with (1) or (5).

Yeah that seems reasonable. I would also add that a new BackForwardEntry should be added when a debugger paused event causes you to move to a completely new location within the same file.
Comment 13 Saam Barati 2014-07-29 11:21:56 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > It looks like this code is at fault:
> > > 
> > >         if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie))
> > >             return currentEntry.contentView;
> > > 
> > > 
> > > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions.
> > 
> > Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails.
> 
> Three times sounds like two too many.

Yeah this is a stack trace after stepping over an instruction:

file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22: CONSOLE TRACE
0: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22)
1: showContentViewForRepresentedObject(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentBrowser.js:158:58)
2: showSourceCode(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:181:72)
3: showSourceCodeLocation(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:186:28)
4: _treeElementSelected(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:525:69)
5: (unknown)([native code])
6: select(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/TreeOutline.js:976:34)
7: _debuggerCallFramesDidChange(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:395:39)
8: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:180:55)
9: dispatchEventToListeners(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:187:17)
10: debuggerDidPause(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Controllers/DebuggerManager.js:379:38)
11: paused(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/DebuggerObserver.js:58:54)
12: dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:309:42)
13: _dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:215:32)
14: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:89:32)
15: dispatchNextQueuedMessageFromBackend(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js:31:34)
16: (unknown)([native code])
17: global code(file:///Volumes/Data/test.js:2:1)
file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22: CONSOLE TRACE
0: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22)
1: _showContentViewForIdentifier(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceClusterContentView.js:236:57)
2: restoreFromCookie(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceClusterContentView.js:147:61)
3: _restoreFromCookie(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Models/BackForwardEntry.js:77:43)
4: prepareToShow(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Models/BackForwardEntry.js:57:32)
5: _showEntry(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:402:28)
6: showBackForwardEntryForIndex(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:203:24)
7: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:180:42)
8: showContentViewForRepresentedObject(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentBrowser.js:158:58)
9: showSourceCode(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:181:72)
10: showSourceCodeLocation(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:186:28)
11: _treeElementSelected(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:525:69)
12: (unknown)([native code])
13: select(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/TreeOutline.js:976:34)
14: _debuggerCallFramesDidChange(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:395:39)
15: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:180:55)
16: dispatchEventToListeners(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:187:17)
17: debuggerDidPause(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Controllers/DebuggerManager.js:379:38)
18: paused(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/DebuggerObserver.js:58:54)
19: dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:309:42)
20: _dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:215:32)
21: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:89:32)
22: dispatchNextQueuedMessageFromBackend(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js:31:34)
23: (unknown)([native code])
24: global code(file:///Volumes/Data/test.js:2:1)
file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22: CONSOLE TRACE
0: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:138:22)
1: _showContentViewForIdentifier(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceClusterContentView.js:236:57)
2: shown(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceClusterContentView.js:130:43)
3: prepareToShow(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Models/BackForwardEntry.js:60:31)
4: _showEntry(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:402:28)
5: showBackForwardEntryForIndex(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:203:24)
6: showContentView(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentViewContainer.js:180:42)
7: showContentViewForRepresentedObject(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ContentBrowser.js:158:58)
8: showSourceCode(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:181:72)
9: showSourceCodeLocation(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/ResourceSidebarPanel.js:186:28)
10: _treeElementSelected(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:525:69)
11: (unknown)([native code])
12: select(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/TreeOutline.js:976:34)
13: _debuggerCallFramesDidChange(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Views/DebuggerSidebarPanel.js:395:39)
14: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:180:55)
15: dispatchEventToListeners(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Base/Object.js:187:17)
16: debuggerDidPause(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Controllers/DebuggerManager.js:379:38)
17: paused(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/DebuggerObserver.js:58:54)
18: dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:309:42)
19: _dispatchEvent(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:215:32)
20: dispatch(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:89:32)
21: dispatchNextQueuedMessageFromBackend(file:///Volumes/Data/WK/OpenSource/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js:31:34)
22: (unknown)([native code])
23: global code(file:///Volumes/Data/test.js:2:1)
Comment 14 Timothy Hatcher 2014-07-29 11:50:25 PDT
(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #7)
> > > > It looks like this code is at fault:
> > > > 
> > > >         if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie))
> > > >             return currentEntry.contentView;
> > > > 
> > > > 
> > > > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions.
> > > 
> > > Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails.
> > 
> > Three times sounds like two too many.
> 
> Yeah this is a stack trace after stepping over an instruction:
> 
> [snip]

So this is a case of ResourceClusterContentView showing a sub-view, so this is likely correct.

I think we should add an isEqual function to BackForwardEntry that also looks at _contentView, _cookie and _scrollPositions.
Comment 15 Timothy Hatcher 2014-07-29 12:29:01 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > (In reply to comment #7)
> > > > > It looks like this code is at fault:
> > > > > 
> > > > >         if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie))
> > > > >             return currentEntry.contentView;
> > > > > 
> > > > > 
> > > > > It only considers the cookie and view when testing if a new entry needs to be pushed. It should also compare scroll positions.
> > > > 
> > > > Okay, I'll look into it. I was actually looking at that code path earlier, and it actually does what it's supposed two thirds of the time. It seems like that code path is travelled down 3 times for each step-over, and on one of the three paths when stepping over, that test fails.
> > > 
> > > Three times sounds like two too many.
> > 
> > Yeah this is a stack trace after stepping over an instruction:
> > 
> > [snip]
> 
> So this is a case of ResourceClusterContentView showing a sub-view, so this is likely correct.
> 
> I think we should add an isEqual function to BackForwardEntry that also looks at _contentView, _cookie and _scrollPositions.

While that would fix some other cases, it will not fix views that use CodeMirror since _scrollPositions is not used for those. So there is another bug with CodeMirror views not saving scroll positions. CodeMirror views do save the last "jump" location to the cookie, which will be used for this with a showSourceCodeLocation call. And the cookie is already compared.

So i think the behavior is correct as-is, where each jump to a new line in the same view pushes a new b/f entry. Not really the best behavior.

Still shown() should not be called if it is already showing the view.
Comment 16 Saam Barati 2014-07-29 19:28:15 PDT
Created attachment 235724 [details]
Idea for patch

What do you guys think of this patch?

It solves the issues I'm having with shown() being called multiple times. But maybe calling _restoreFromCookie() is an abstraction violation. But this is the essence of what a patch to fix this would be, I think.
Comment 17 Timothy Hatcher 2014-07-29 20:55:00 PDT
Comment on attachment 235724 [details]
Idea for patch

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

Looks right!

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:205
> +            currentEntry._restoreFromCookie();

Yeah, restoreFromCookie should drop the underscore and be a public method if this needs to call it.
Comment 18 Timothy Hatcher 2014-07-29 23:31:59 PDT
Comment on attachment 235724 [details]
Idea for patch

Actually it might be best to make a helper to do this. There are a couple other places where we do the _hideEntry(previousEntry) / _showEntry(currentEntry) dance.
Comment 19 Saam Barati 2014-07-30 10:46:42 PDT
(In reply to comment #17)
> (From update of attachment 235724 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235724&action=review
> 
> Looks right!
> 
> > Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:205
> > +            currentEntry._restoreFromCookie();
> 
> Yeah, restoreFromCookie should drop the underscore and be a public method if this needs to call it.

Okay. I'll probably end up doing that. Is it the intention of restoreFromCookie to be called more than once? Or maybe should there be another interface to updating the lineNumber info.
Comment 20 Saam Barati 2014-07-30 10:47:07 PDT
(In reply to comment #18)
> (From update of attachment 235724 [details])
> Actually it might be best to make a helper to do this. There are a couple other places where we do the _hideEntry(previousEntry) / _showEntry(currentEntry) dance.

Okay. I'll make this a helper.
Comment 21 Saam Barati 2014-07-30 10:50:42 PDT
(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 235724 [details] [details])
> > Actually it might be best to make a helper to do this. There are a couple other places where we do the _hideEntry(previousEntry) / _showEntry(currentEntry) dance.
> 
> Okay. I'll make this a helper.

It actually doesn't seem like there is a good way to generalize this to all other place that call both _showEntry/_hideEntry
Comment 22 Saam Barati 2014-07-30 10:59:56 PDT
Created attachment 235750 [details]
another patch proposition

This could also be a way to implement that patch, that passes in a flag indicating whether BackForwardEntry should call shown().

Which do you all like better?
Comment 23 Timothy Hatcher 2014-07-30 20:11:38 PDT
Comment on attachment 235750 [details]
another patch proposition

I like this.
Comment 24 Joseph Pecoraro 2014-07-30 21:39:37 PDT
Comment on attachment 235750 [details]
another patch proposition

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

> Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js:57
>          this._restoreFromCookie();

Did we also determine that _restoreFromCookie might call shown()? That was being tracked separately though.
Comment 25 Saam Barati 2014-07-31 13:09:54 PDT
(In reply to comment #24)
> (From update of attachment 235750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235750&action=review
> 
> > Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js:57
> >          this._restoreFromCookie();
> 
> Did we also determine that _restoreFromCookie might call shown()? That was being tracked separately though.

I haven't looked into it, specifically. Anecdotally, this hasn't happened in my testing of the above patches. That's not to say it might not.
Comment 26 Saam Barati 2014-08-04 19:04:08 PDT
Created attachment 236000 [details]
patch

I've tested this patch, and it solves the problem of shown not being repeatedly called when stepping over instructions in the debugger, 
but it doesn't solve all the problems of shown being called multiple times. I think for further fixing the redundant calls to shown, 
I should do more investigation on: https://bugs.webkit.org/show_bug.cgi?id=135000.
Comment 27 WebKit Commit Bot 2014-08-05 11:30:04 PDT
Comment on attachment 236000 [details]
patch

Clearing flags on attachment: 236000

Committed r172038: <http://trac.webkit.org/changeset/172038>
Comment 28 WebKit Commit Bot 2014-08-05 11:30:09 PDT
All reviewed patches have been landed.  Closing bug.