Bug 128334 - Web Inspector: animate breakpoint tree elements when probe samples are received
Summary: Web Inspector: animate breakpoint tree elements when probe samples are received
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: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-06 14:32 PST by BJ Burg
Modified: 2014-02-21 15:56 PST (History)
5 users (show)

See Also:


Attachments
patch (20.24 KB, patch)
2014-02-10 14:26 PST, BJ Burg
no flags Details | Formatted Diff | Diff
revised patch (20.05 KB, patch)
2014-02-13 10:11 PST, BJ Burg
no flags Details | Formatted Diff | Diff
revised patch 2 (20.05 KB, patch)
2014-02-13 10:55 PST, BJ Burg
no flags Details | Formatted Diff | Diff
sync probe set lifetime and listeners to attach/detach. (20.43 KB, patch)
2014-02-13 13:25 PST, BJ Burg
timothy: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (561.24 KB, application/zip)
2014-02-13 14:51 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (593.42 KB, application/zip)
2014-02-13 20:42 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2014-02-06 14:32:44 PST
This may require a new asset for the animation glyph.
Comment 1 Radar WebKit Bug Importer 2014-02-06 14:33:23 PST
<rdar://problem/16005998>
Comment 2 BJ Burg 2014-02-10 14:26:26 PST
Created attachment 223747 [details]
patch
Comment 3 Timothy Hatcher 2014-02-10 15:17:01 PST
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.
Comment 4 BJ Burg 2014-02-13 10:10:53 PST
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.
Comment 5 BJ Burg 2014-02-13 10:11:01 PST
Created attachment 224083 [details]
revised patch
Comment 6 BJ Burg 2014-02-13 10:55:06 PST
Created attachment 224088 [details]
revised patch 2
Comment 7 Timothy Hatcher 2014-02-13 12:23:46 PST
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.
Comment 8 BJ Burg 2014-02-13 13:25:03 PST
Created attachment 224097 [details]
sync probe set lifetime and listeners to attach/detach.
Comment 9 Timothy Hatcher 2014-02-13 13:32:17 PST
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.
Comment 10 Build Bot 2014-02-13 14:51:30 PST
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
Comment 11 Build Bot 2014-02-13 14:51:31 PST
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
Comment 12 BJ Burg 2014-02-13 15:49:18 PST
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.
Comment 13 Build Bot 2014-02-13 20:42:27 PST
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
Comment 14 Build Bot 2014-02-13 20:42:29 PST
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
Comment 15 BJ Burg 2014-02-21 15:56:23 PST
Committed r164510: <http://trac.webkit.org/changeset/164510>