[webkit-reviews] review granted: [Bug 239880] Web Inspector: Importing a timeline leaves timeline overview non-scrollable/non-zoomable until windows is resized : [Attachment 458693] Patch v1.2

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 239880: Web Inspector: Importing a timeline leaves timeline overview
non-scrollable/non-zoomable until windows is resized
https://bugs.webkit.org/show_bug.cgi?id=239880

Attachment 458693: Patch v1.2

https://bugs.webkit.org/attachment.cgi?id=458693&action=review




--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 458693
  --> https://bugs.webkit.org/attachment.cgi?id=458693
Patch v1.2

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

r=me, nice fix =D

> Source/WebInspectorUI/ChangeLog:29
> +	   `_dirtyDescendantsCount` and at what times relative to layout.

would be awesome to have some tests for this (e.g. calling `updateLayout`
inside the `layout` of a parent)

> Source/WebInspectorUI/ChangeLog:31
> +	   The only time in this patch that we do not do a +1/-1 adjustment to
the layout count is in the special case of

would be awesome to have some tests for this (e.g. ensuring that the accounting
is done correctly if a dirty `WI.View` is removed)

> Source/WebInspectorUI/ChangeLog:52
> +	   - A special case of recursively calling `_setDirty(false)` that is
able to optimize away the need to walk the

Aside: Reading this made me think a bit, and im pretty sure there's actually
another unrelated bug.	Nowhere in `WI.View` do we check to see if when adding
new subviews that the given subview is not already a subview somewhere else. 
We probably should add a `console.assert(!view.parentView)` somewhere inside
`WI.View.prototype.insertSubviewBefore` to catch this (or just add logic to
automatically call `view.parentView.removeSubview(view)` in this case).

> Source/WebInspectorUI/UserInterface/Views/View.js:251
> +	   this._setSelfAndDescendantsAttachedToRoot(isAttachedToRoot);

It seems like both `_setSelfAndDescendantsNotDirty` and
`_setSelfAndDescendantsAttachedToRoot` crawl down the entire view hierarchy
underneath the `parentView`.  Can we perhaps combine those two methods into a
single function (or inlined here) so that we only have to crawl down once?

> Source/WebInspectorUI/UserInterface/Views/View.js:268
> +	       let view = views.shift();

NIT: rather than call `shift()`, which is pretty expensive, can we instead just
keep an index (e.g. `for (let i = 0; i < views.length; ++i)`)?	We might wanna
do that in `WI.View._visitViewTreeForLayout` too ��