[webkit-reviews] review canceled: [Bug 113198] Web Inspector: provide minimized mode. : [Attachment 195567] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has canceled Dmitry Gozman
<dgozman at chromium.org>'s request for review:
Bug 113198: Web Inspector: provide minimized mode.
https://bugs.webkit.org/show_bug.cgi?id=113198

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

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=195567&action=review


Nice, we are almost there.

> Source/WebCore/inspector/front-end/Toolbar.js:53
> +    if (!WebInspector.experimentsSettings.minimizedMode.isEnabled() ||
!Preferences.canMinimize) {

Would it be a good idea to create the experiment ONLY if
Preferences.canMinimize === true?

> Source/WebCore/inspector/front-end/Toolbar.js:394
> +	   var controlsLeft = document.getElementById("toolbar-controls-left");


This should be moved next to its usage (I though it was unused at the first
glance). If we bail out early, this computation is dropped on the floor.

> Source/WebCore/inspector/front-end/Toolbar.js:396
> +	   if (controlsRight.parentNode !== this.element)

parentElement for clarity?

> Source/WebCore/inspector/front-end/Toolbar.js:410
> +	   var controlsLeft = document.getElementById("toolbar-controls-left");


This should be moved next to its usage.

> Source/WebCore/inspector/front-end/Toolbar.js:412
> +	   if (controlsRight.parentNode === this.element)

parentElement

> Source/WebCore/inspector/front-end/Toolbar.js:422
> +	  
this._minimizedToolbar.parentNode.removeChild(this._minimizedToolbar);

You can write this._minimizedToolbar.removeSelf() (defined in DOMExtension.js
on Element.prototype)

> Source/WebCore/inspector/front-end/inspector.css:630
> +    opacity: 0.7;

It's a bit strange to decrease a button's opacity upon activation (:active
implies :hover)