[webkit-reviews] review denied: [Bug 30282] Web Inspector: Add console only layout mode : [Attachment 43543] [PATCH] Console Panel (Improved)

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 30282: Web Inspector: Add console only layout mode
https://bugs.webkit.org/show_bug.cgi?id=30282

Attachment 43543: [PATCH] Console Panel (Improved)
https://bugs.webkit.org/attachment.cgi?id=43543&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> +	   WebInspector.Panel.prototype.show.call(this);

I like to put a blank line after these.

> +	   WebInspector.drawer.triggerConsoleMode(true);

I don't think triggerConsoleMode is a good name. Maybe enterPanelMode and
exitPanelMode?

> +	   WebInspector.Panel.prototype.hide.call(this);

Add a blank line.

> +	   if (this.previousConsoleState == WebInspector.Drawer.State.Hidden)

Use ===.


> -    var anchoredStatusBar =
document.getElementById("anchored-status-bar-items");
> -    anchoredStatusBar.appendChild(this.toggleConsoleButton);

Where did this go?


> +    this.fullConsole = false;

This should be be something like "fullPanel" since this is Drawer.js, and
should not have console mentions. Why not just use this.state?

> +	       if (this.visible && WebInspector.currentPanel ===
WebInspector.panels.console)

Can this not check the current panel and just use this.fullPanel? I'd like to
not add this dependancy and console reference if you can avoid it.

> +	       height = parseInt(this.element.style.height, 10);

Isn't base 10 the default?


> +	       WebInspector.cancelAnimation(this.currentAnimation);
> +	       delete this._animating;

Should you delete this.currentAnimation too?

> +	   this.fullConsole = fullConsole;
> +	   if (fullConsole) {
> +	       if (this.visible)
> +		   this._transitionToFullConsole();
> +	   } else {
> +	       if (this.visible)
> +		   this._transitionToVariableConsole();
> +	   }
> +    },
> +
> +    _transitionToFullConsole: function()
> +    {
> +	   if (this._animating)
> +	       return;
> +
> +	   this.savedHeight = this.element.offsetHeight;
> +
> +	   var height = window.innerHeight - this.toolbarElement.offsetHeight;
> +	   this._animateDrawerHeight(height, WebInspector.Drawer.State.Full);
> +    },
> +
> +    _transitionToVariableConsole: function()
> +    {
> +	   if (this._animating)
> +	       return;
> +
> +	   // If this animation gets cancelled, we want the state of the drawer
to be Variable,
> +	   // so that the new animation can't do an immediate transition
between Hidden/Full states.
> +	   this.state = WebInspector.Drawer.State.Variable;
> +
> +	   var height = this.savedHeight;
> +	   this._animateDrawerHeight(height,
WebInspector.Drawer.State.Variable);
> +    },

More console mentions, should be generic.


> +	   function animationFinished()
> +	   {
> +	       delete this._animating;
> +	       this.state = finalState;
> +	   }

Delete this.currentAnimation too?

> +	   WebInspector.animations[key] = setTimeout(WebInspector.animateStyle,
slice, animations, duration, callback, complete + slice, key);

Why not just make the key be the timeout id?