Bug 128334 – Web Inspector: animate breakpoint tree elements when probe samples are received

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

This may require a new asset for the animation glyph.

[Radar WebKit Bug Importer]

<rdar://problem/16005998>

[Blaze Burg]

Created attachment 223747 [details]
patch

[Timothy Hatcher]

Comment on attachment 223747 [details]
patch

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

> Source/WebInspectorUI/ChangeLog:8
> +        * UserInterface/BreakpointIcons.css: Removed, rules migrated to the following file.

You need to remove this from Main.html. Keeping it in will make the Production build fail when it tries to combine resources.

> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:48
> +        this._probeSetAdded({data: probeSet});

This seems fragile. There should be a separate function for adding a probe set that is not event driven, and the _probeSetAdded version that takes an event can call it.

> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:122
> +            this._probeSetRemoved({data: this._probeSet});

Ditto.

> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:170
> +        var probeSet = event.data;

The data is normally not a special object, it is just a container. This should be event.data.probeSet. That allows event.data to have other properties later.

> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:213
> +        this._currentTimeout = setTimeout(function() {

This timeout found use a better name. _probeDataUpdatedTimeoutIdentifier? rAF?

> Source/WebInspectorUI/UserInterface/GeneralTreeElement.js:312
> +        this._iconWrapperElement = document.createElement("div");
> +        this._iconWrapperElement.className = WebInspector.GeneralTreeElement.IconWrapperElementStyleClassName;

This makes me a little sad. GeneralTreeElement is used everywhere and this adds another DOM element to all those uses, even though the wrapper is only needed for this specific probe case. Can this be optional for specific GeneralTreeElement subclasses to opt into?

> Source/WebInspectorUI/UserInterface/GeneralTreeElement.js:323
> +        this._mainTitleElement = this._titlesElement.createChild("span");

I was never fond of these Google added helper functions.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.css:302
> +.navigation-sidebar-panel-content-tree-outline.small .item .icon-wrapper,
> +.navigation-sidebar-panel-content-tree-outline .children.small .item .icon-wrapper,
> +.navigation-sidebar-panel-content-tree-outline .item.small .icon-wrapper {

This breaks the styles in TimelineSidebarPanel.css:

.sidebar > .panel.timeline > .timelines-content li.item .icon

This also breaks our existing pattern of ".foo-icon .icon" being generic for anywhere an icon needs displayed. It is now sometimes .foo-icon .icon-wrapper.

> Source/WebInspectorUI/UserInterface/ProbeSet.js:89
> -        this.dispatchEventToListeners(WebInspector.ProbeSet.Event.SamplesCleared, this);
> +        this.dispatchEventToListeners(WebInspector.ProbeSet.Event.SamplesCleared, {oldTable: oldTable});

A case where "this" should have been "{probeSet: this}" before.

[Blaze Burg]

Comment on attachment 223747 [details]
patch

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

>> Source/WebInspectorUI/ChangeLog:8
>> +        * UserInterface/BreakpointIcons.css: Removed, rules migrated to the following file.
> 
> You need to remove this from Main.html. Keeping it in will make the Production build fail when it tries to combine resources.

OK

>> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:48
>> +        this._probeSetAdded({data: probeSet});
> 
> This seems fragile. There should be a separate function for adding a probe set that is not event driven, and the _probeSetAdded version that takes an event can call it.

oK

>> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:170
>> +        var probeSet = event.data;
> 
> The data is normally not a special object, it is just a container. This should be event.data.probeSet. That allows event.data to have other properties later.

Oops, fixed.

>> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:213
>> +        this._currentTimeout = setTimeout(function() {
> 
> This timeout found use a better name. _probeDataUpdatedTimeoutIdentifier? rAF?

rAF is only going to help if we want to draw on the next frame.

>> Source/WebInspectorUI/UserInterface/ProbeSet.js:89
>> +        this.dispatchEventToListeners(WebInspector.ProbeSet.Event.SamplesCleared, {oldTable: oldTable});
> 
> A case where "this" should have been "{probeSet: this}" before.

Better to use event.target here; passing `this` was superfluous.

[Blaze Burg]

Created attachment 224083 [details]
revised patch

[Blaze Burg]

Created attachment 224088 [details]
revised patch 2

[Timothy Hatcher]

Comment on attachment 224088 [details]
revised patch 2

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

> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:126
> +    ondetach: function()
> +    {
> +        WebInspector.GeneralTreeElement.prototype.ondetach.call(this);
> +
> +        this._listeners.uninstall(true);
> +
> +        if (this._probeSet)
> +            this._probeSetRemoved({data: this._probeSet});
> +    },

I'm not sure this is the right place to do this. A tree element can be later reattached and then the listeners are not reinstalled.

Just because the tree element is detached does the mean the probe set should be removed. It might be reattached to the tree outline again — like if the sidebar is sorted or something.

[Blaze Burg]

Created attachment 224097 [details]
sync probe set lifetime and listeners to attach/detach.

[Timothy Hatcher]

Comment on attachment 224097 [details]
sync probe set lifetime and listeners to attach/detach.

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

> Source/WebInspectorUI/UserInterface/BreakpointTreeElement.js:121
> +        for (var probeSet of WebInspector.probeManager.probeSets)
> +            if (probeSet.breakpoint === this._breakpoint)
> +                this._addProbeSet(probeSet);

This should return early to prevent walking the whole probe set array once the right one is found. Returning early also ensures _addProbeSet and _removeProbeSet are balanced.

[Build Bot]

Comment on attachment 224097 [details]
sync probe set lifetime and listeners to attach/detach.

Attachment 224097 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6226749834657792

New failing tests:
inspector-protocol/model/probe-manager-add-remove-actions.html

[Build Bot]

Created attachment 224115 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Blaze Burg]

Oops. Since I corrected the parameters of ProbeSetAdded/Removed events in ProbeManager, need to fix the raw protocol test (doesn't use models) so that it looks for event.data.probeSet, not just event.data.

[Build Bot]

Comment on attachment 224097 [details]
sync probe set lifetime and listeners to attach/detach.

Attachment 224097 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5071957414379520

New failing tests:
inspector-protocol/model/probe-manager-add-remove-actions.html

[Build Bot]

Created attachment 224155 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Blaze Burg]

Committed r164510: <http://trac.webkit.org/changeset/164510>