Web Inspector: Layout flashing for internal View.prototype.layout

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 319921 [details]
[Animated GIF] Demo

I added a debug checkbox to draw an orange outline around an element every time View.prototype.layout is called.
This is similar to paint flashing, but just for View.prototype.layout.

I find it helpful to see unnecessary layout calls that would be hard to find out with console logging.

[Nikita Vasilyev]

Created attachment 319922 [details]
Patch

[Matt Baker]

Comment on attachment 319922 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/View.js:248
> +        }

To guarantee that this is even for views that don't call super.layout(), this should go in View.prototype._layoutSubtree immediately after `layout` is called:

    _layoutSubtree()
    {
        ...

        this.layout();

        this._drawLayoutFlashingOutline();

        ...
    }

[Matt Baker]

Comment on attachment 319922 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/View.js:370
> +};

The View's backing element doesn't change, so layout flashing metadata should be members of the View instead of hanging off the DOM.

[Nikita Vasilyev]

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

>> Source/WebInspectorUI/UserInterface/Views/View.js:370
>> +};
> 
> The View's backing element doesn't change, so layout flashing metadata should be members of the View instead of hanging off the DOM.

Do we assume that this._element is never set outside of the constructor?

If that's the case, should I replace the symbols with _layoutFlashingTimeout and _layoutFlashingPreviousOutline properties?

[Matt Baker]

(In reply to Nikita Vasilyev from comment #4)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319922&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/View.js:370
> >> +};
> > 
> > The View's backing element doesn't change, so layout flashing metadata should be members of the View instead of hanging off the DOM.
> 
> Do we assume that this._element is never set outside of the constructor?

Correct. View sets _element during construction, and exposes it as a getter only. Derived classes and other objects would have to go through the underscore prefixed name to change the element, which would be bad behavior. If View changes its _element in the future, it can take the necessary steps to ensure layout flashing works correctly.

> If that's the case, should I replace the symbols with _layoutFlashingTimeout
> and _layoutFlashingPreviousOutline properties?

Sounds good. Those were the names I had in mind too.

[Nikita Vasilyev]

Created attachment 319931 [details]
Patch

[Matt Baker]

Comment on attachment 319931 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:295
> +        this._debugSettingsView.addSetting(WI.unlocalizedString("Layout Flashing:"), WI.settings.enableLayoutFlashing, WI.unlocalizedString("Draw orange borders on every View.prototype.layout call"));

Even though this is intended for Inspector developers, I think we should refrain from including implementation details in UI text. The details of View could change in the future.

What about: "Draw borders when a view performs a layout"

> Source/WebInspectorUI/UserInterface/Views/View.js:284
> +        this._layoutFlashingPreviousOutline = previousOutline;

We don't need to set `this._layoutFlashingPreviousOutline` every time the timer is reset. You can remove `previousOutline` too:

if (this._layoutFlashingTimeout)
    clearTimeout(this._layoutFlashingTimeout);
else
    this._layoutFlashingPreviousOutline = this._element.style.outline;

> Source/WebInspectorUI/UserInterface/Views/View.js:288
> +            this._element.style.outline = previousOutline;

Check that `this._element` is defined, just to be safe. Currently the lifetime of the element is the same as its View, but that could always change.

> Source/WebInspectorUI/UserInterface/Views/View.js:289
> +            this._layoutFlashingTimeout = 0;

Elsewhere we use `undefined` instead of zero, when unsetting stored timer IDs. The HTML5 spec states that the return value must be greater than zero, but `undefined` is more explicit.

> Source/WebInspectorUI/UserInterface/Views/View.js:290
> +            this._layoutFlashingPreviousOutline = "";

= null;

[Nikita Vasilyev]

Created attachment 319937 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 319931 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/View.js:284
>> +        this._layoutFlashingPreviousOutline = previousOutline;
> 
> We don't need to set `this._layoutFlashingPreviousOutline` every time the timer is reset. You can remove `previousOutline` too:
> 
> if (this._layoutFlashingTimeout)
>     clearTimeout(this._layoutFlashingTimeout);
> else
>     this._layoutFlashingPreviousOutline = this._element.style.outline;

This was unnecessary complicated, thanks!

[Nikita Vasilyev]

Created attachment 319943 [details]
Patch

Rebaselined.

[Matt Baker]

Comment on attachment 319943 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 319943 [details]
Patch

Clearing flags on attachment: 319943

Committed r221648: <http://trac.webkit.org/changeset/221648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693560>