Bug 135311 – Web Inspector: shown() called on a content view when stepping over an instruction in the debugger

• Component: Web Inspector
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Radar WebKit Bug Importer]

<rdar://problem/17817144>

[Timothy Hatcher]

It wasn't intended to be called multiple times.

[Saam Barati]

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?

[Timothy Hatcher]

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.

[Timothy Hatcher]

We probably should not push new back forward entries for each pause/resume if the scroll position didn't change.

[Saam Barati]

(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.

[Timothy Hatcher]

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.

[Joseph Pecoraro]

(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).

[Joseph Pecoraro]

(In reply to comment #8)
> the source sidebar

Resource* sidebar

[Saam Barati]

(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.

[Timothy Hatcher]

(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.

[Saam Barati]

(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.

[Saam Barati]

(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)

[Timothy Hatcher]

(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.

[Timothy Hatcher]

(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.

[Saam Barati]

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.

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.

[Saam Barati]

(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.

[Saam Barati]

(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.

[Saam Barati]

(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

[Saam Barati]

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?

[Timothy Hatcher]

Comment on attachment 235750 [details]
another patch proposition

I like this.

[Joseph Pecoraro]

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.

[Saam Barati]

(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.

[Saam Barati]

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.

[WebKit Commit Bot]

Comment on attachment 236000 [details]
patch

Clearing flags on attachment: 236000

Committed r172038: <http://trac.webkit.org/changeset/172038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.