[webkit-reviews] review denied: [Bug 128334] Web Inspector: animate breakpoint tree elements when probe samples are received : [Attachment 223747] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 10 15:19:43 PST 2014


Timothy Hatcher <timothy at apple.com> has denied Brian Burg <bburg at apple.com>'s
request for review:
Bug 128334: Web Inspector: animate breakpoint tree elements when probe samples
are received
https://bugs.webkit.org/show_bug.cgi?id=128334

Attachment 223747: patch
https://bugs.webkit.org/attachment.cgi?id=223747&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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.


More information about the webkit-reviews mailing list