Bug 149186 – Web Inspector: Create View base class, refactor some core view classes

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
Create View/ScrollableView base classes. View objects in the Inspector frontend use a needsLayout/updateLayout/requestAnimationFrame idiom that is heavily duplicated throughout the codebase. This should be encapsulated into a View base class:

Object <-- View

TimelineOverview includes logic and DOM elements (scrollbar and scroll sizer) needed to create scrolling/zooming behavior. This should be its own base class:

Object <-- View <-- ScrollableView

* NOTES
This issue to only covers the creation of View and ScrollableView, not the refactoring effort needed to convert our existing views.

[Radar WebKit Bug Importer]

<rdar://problem/22709647>

[Matt Baker]

Created attachment 262466 [details]
WIP

[Matt Baker]

Created attachment 263328 [details]
[Patch] Proposed Fix

[Blaze Burg]

Comment on attachment 263328 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:7
> +

Need to explain the change. The bugzilla comments are a good start.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:73
> +        return new WebInspector.Point(this.x * anotherPoint.x, this.y * anotherPoint.y);

this should be Point.scale(x, y), right? Points can't be multiplied.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:76
> +    offset(anotherPoint)

These names are not standard. Point.moveTo(Point) or translate are used elsewhere in the codebase.

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:-98
> -WebInspector.Size.ZERO_SIZE = new WebInspector.Size(0, 0);

best to just make Size.zero() a member function and return new instances.

[Matt Baker]

The scope of this patch has been changed to cover just the addition of View.js. Additionally, a few key view classes will be refactored to inherit from WebInspector.View:

UserInterface/Views/ContentBrowser.js
UserInterface/Views/ContentViewContainer.js
UserInterface/Views/NavigationBar.js
UserInterface/Views/TabBar.js
UserInterface/Views/TabBrowser.js
UserInterface/Views/Toolbar.js

The following global WebInspector methods (in Main.js) will also be updated to make use of the new WebInspector.View method `updateLayoutForResize`:

WebInspector._windowResized
WebInspector._tabBrowserSizeDidChange
WebInspector._quickConsoleDidResize

[Devin Rousso]

Comment on attachment 263328 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/Geometry.js:73
>> +        return new WebInspector.Point(this.x * anotherPoint.x, this.y * anotherPoint.y);
> 
> this should be Point.scale(x, y), right? Points can't be multiplied.

While I agree that points can't be multiplied, Point.scale(anotherPoint) is just a shortcut of vector scaling.  By using anotherPoint instead of x, y, and z, this just skips the matrix transformation step when scaling a vector (e.g. https://en.wikipedia.org/wiki/Scaling_(geometry)#Matrix_representation).  The end result is the same, so I think it would actually be prudent to allow for all three options: Point.scale(c), Point.scale(x, y), and Point.scale(anotherPoint)

>> Source/WebInspectorUI/UserInterface/Models/Geometry.js:76
>> +    offset(anotherPoint)
> 
> These names are not standard. Point.moveTo(Point) or translate are used elsewhere in the codebase.

I agree.  Translate would to fit here.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:34
> +        this._scrollDirection = scrollDirection;

Should this have a fallback (maybe WebInspector.ScrollableView.ScrollDirection.Vertical)?

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:37
> +        this._contentScaleFactor = new WebInspector.Point(1, 1);

Calling this "_contentScaleFactor" is a bit misleading to me as it implies a scalar quantity (single value c) by which other points are scaled.  I think something like "_contentScaleOrigin" may make more sense.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:38
> +        this._inverseContentScaleFactor = new WebInspector.Point(1, 1);

"inverse" isn't really the right word to describe this (the inverse of a point (x, y) would be (y, x)).  Technically, this is the reciprocal.  I get why this is necessary, but I think it may be much easier and more readable to just compute the point reciprocal at the moment it is necessary.  See line 323.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:45
> +        this._zoomFactor = 1;

From what I can tell, this only ever applies to horizontal values (x, width, etc).  Is there a reason why, in a view that can scroll both ways it wouldn't also effect the vertical values (y, height, etc)?

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:165
> +        // clamp the zoom factor and force an update.

NIT: I think constrain is a better word instead of clamp.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:185
> +        // clamp the zoom factor and force an update.

NIT: I think constrain is a better word instead of clamp.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:197
> +        x = Math.max(this._minimumZoomFactor, Math.min(this._maximumZoomFactor, x));

I think Number.constrain(x, this._minimumZoomFactor, this._maximumZoomFactor) would work here.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:207
> +        if (x)

You could use this.element.classList.toggle("debug", x).  I would also probably rename "x" to "flag"

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:229
> +        const maximumContentOffsetX = Math.max(0, this._contentSize.width - visibleContentSize.width);

I think this line is unnecessary.  You are already constraining the value to >0 on the next line, so you cloud just merge these two lines.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:233
> +        this._contentOffset.y = Math.min(maximumContentOffsetY, this._contentOffset.y);

Use Number.constrain() like you did above.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:245
> +        // Debug information: overlay scroll/zoom properties.

I think this could result in a lot of unnecessary DOM manipulation.  Instead of always having the debug element exist, you could save the debug info as
this._debugInfo = {offset: ..., scale: ..., content: ..., visible: ..., view: ..., zoom: ...}
and have two functions (this.showDebugInfo() and this.hideDebugInfo()) that add/remove the necessary DOM elements.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:323
> +        let mousePositionContentOffset = mousePositionView.piecewiseProduct(this._inverseContentScaleFactor);

If you do transition to Point.scale(x,y), I would remove this._inverseContentScaleFactor and just use mousePoisitionView.scale(1 / this._contentScaleFactor.x, 1 / this._contentScaleFactor.y)

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:328
> +        const delta = event.deltaY * WebInspector.ScrollableView.ScrollDeltaScale * deviceDirection;

Is it necessary to create this variable even though it's only used once?

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:333
> +            const contentOffsetX = mousePositionContent.x - (mousePositionView.x / this._contentScaleFactor.x) / this._zoomFactor;

See line 328.

> Source/WebInspectorUI/UserInterface/Views/View.js:32
> +        this._element = element || document.createElement("div");

Am I being too paranoid, or would it be safe to add a check to make sure the typeof element is HTMLElement?

> Source/WebInspectorUI/UserInterface/Views/View.js:34
> +        this._element.__view = this;

I'm a bit confused as to why this is necessary.  Purely for convenience?

> Source/WebInspectorUI/UserInterface/Views/View.js:62
> +        console.assert(!view.element.parentNode, "Subview element already exists in the DOM.");

It seems to me like if any of these assertions are triggered that we should have a return.  Is there a reason that we don't?

> Source/WebInspectorUI/UserInterface/Views/View.js:64
> +        view._parentView = this;

Correct me if I'm wrong, but this isn't a layering violation because we are dealing with two objects (this and view) which are both WebInspector.View objects?  This just looks weird to me...

> Source/WebInspectorUI/UserInterface/Views/View.js:73
> +        console.assert(view.element.parentNode === this.element, "Subview element should be a child of this view's element.");

Same as line 62.

> Source/WebInspectorUI/UserInterface/Views/View.js:76
> +        view._parentView = null;

Same as line 64.

> Source/WebInspectorUI/UserInterface/Views/View.js:111
> +        units = units || "%";

Couldn't you use a default parameter?  Also, why is it that you're defaulting to "%"?  We normally use "px" for our defaults (e.g. the computed panel).

[Blaze Burg]

Comment on attachment 263328 [details]
[Patch] Proposed Fix

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

Marking this patch r- to get it out of review queue. I know you are working on a new version, looking forward to seeing it.

>> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:37
>> +        this._contentScaleFactor = new WebInspector.Point(1, 1);
> 
> Calling this "_contentScaleFactor" is a bit misleading to me as it implies a scalar quantity (single value c) by which other points are scaled.  I think something like "_contentScaleOrigin" may make more sense.

I'm not sure what scale factor means here, or why we would need separate x and y scale factors. When I read 'scale', I'm imagining d3's scales (https://github.com/mbostock/d3/wiki/Scales) which are functions that map input domains to output ranges.

In any case, I wouldn't use a Point here. If you want to support multiple scales (linear, log, etc) then the change should accompany a consumer of the API.

>> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:38
>> +        this._inverseContentScaleFactor = new WebInspector.Point(1, 1);
> 
> "inverse" isn't really the right word to describe this (the inverse of a point (x, y) would be (y, x)).  Technically, this is the reciprocal.  I get why this is necessary, but I think it may be much easier and more readable to just compute the point reciprocal at the moment it is necessary.  See line 323.

I would make these getters, if it is really used in more than 1-2 places.

> Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:43
> +        this._zoomDirection = zoomDirection || WebInspector.ScrollableView.ZoomDirection.None;
> +        this._minimumZoomFactor = 0.1;

Not sure what zoomDirection is supposed to do. The only options are None and Horizontal. Maybe it should be a boolean?

[Matt Baker]

(In reply to comment #6)
> Comment on attachment 263328 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263328&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/Geometry.js:73
> >> +        return new WebInspector.Point(this.x * anotherPoint.x, this.y * anotherPoint.y);
> > 
> > this should be Point.scale(x, y), right? Points can't be multiplied.
> 
> While I agree that points can't be multiplied, Point.scale(anotherPoint) is
> just a shortcut of vector scaling.  By using anotherPoint instead of x, y,
> and z, this just skips the matrix transformation step when scaling a vector
> (e.g.
> https://en.wikipedia.org/wiki/Scaling_(geometry)#Matrix_representation). 
> The end result is the same, so I think it would actually be prudent to allow
> for all three options: Point.scale(c), Point.scale(x, y), and
> Point.scale(anotherPoint)

Point.scale is a much better name, and I agree an overload for passing a Point is useful. While it's true there isn't geometric interpretation of point multiplication, piecewise multiplication of vectors is a common operation in computer graphics (often using the notation p ⊗ q), especially in lighting and shading equations (see Real Time Rendering, by Adenine-Moller and Haines). That said the class is Point, not Vector, and it only makes sense to scale points on the plane.

> 
> >> Source/WebInspectorUI/UserInterface/Models/Geometry.js:76
> >> +    offset(anotherPoint)
> > 
> > These names are not standard. Point.moveTo(Point) or translate are used elsewhere in the codebase.
> 
> I agree.  Translate would to fit here.

Agreed.

> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:34
> > +        this._scrollDirection = scrollDirection;
> 
> Should this have a fallback (maybe
> WebInspector.ScrollableView.ScrollDirection.Vertical)?
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:37
> > +        this._contentScaleFactor = new WebInspector.Point(1, 1);
> 
> Calling this "_contentScaleFactor" is a bit misleading to me as it implies a
> scalar quantity (single value c) by which other points are scaled.  I think
> something like "_contentScaleOrigin" may make more sense.

Maybe viewTransform or viewScale would be better. This tuple defines the scale from world (content) space to view space.
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:38
> > +        this._inverseContentScaleFactor = new WebInspector.Point(1, 1);
> 
> "inverse" isn't really the right word to describe this (the inverse of a
> point (x, y) would be (y, x)).  Technically, this is the reciprocal.  I get
> why this is necessary, but I think it may be much easier and more readable
> to just compute the point reciprocal at the moment it is necessary.  See
> line 323.
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:45
> > +        this._zoomFactor = 1;
> 
> From what I can tell, this only ever applies to horizontal values (x, width,
> etc).  Is there a reason why, in a view that can scroll both ways it
> wouldn't also effect the vertical values (y, height, etc)?

It all depends on the application. For flame charts we intend to scroll in both directions, but only scale on the horizontal axis. We currently use zooming (horizontal) in the TimelineOverview, which only scrolls horizontally. There are no use cases yet for zooming on the y-axis.

> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:165
> > +        // clamp the zoom factor and force an update.
> 
> NIT: I think constrain is a better word instead of clamp.
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:185
> > +        // clamp the zoom factor and force an update.
> 
> NIT: I think constrain is a better word instead of clamp.
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:197
> > +        x = Math.max(this._minimumZoomFactor, Math.min(this._maximumZoomFactor, x));
> 
> I think Number.constrain(x, this._minimumZoomFactor,
> this._maximumZoomFactor) would work here.
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:207
> > +        if (x)
> 
> You could use this.element.classList.toggle("debug", x).  I would also
> probably rename "x" to "flag"
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:229
> > +        const maximumContentOffsetX = Math.max(0, this._contentSize.width - visibleContentSize.width);
> 
> I think this line is unnecessary.  You are already constraining the value to
> >0 on the next line, so you cloud just merge these two lines.
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:233
> > +        this._contentOffset.y = Math.min(maximumContentOffsetY, this._contentOffset.y);
> 
> Use Number.constrain() like you did above.
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:245
> > +        // Debug information: overlay scroll/zoom properties.
> 
> I think this could result in a lot of unnecessary DOM manipulation.  Instead
> of always having the debug element exist, you could save the debug info as
> this._debugInfo = {offset: ..., scale: ..., content: ..., visible: ...,
> view: ..., zoom: ...}
> and have two functions (this.showDebugInfo() and this.hideDebugInfo()) that
> add/remove the necessary DOM elements.
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:323
> > +        let mousePositionContentOffset = mousePositionView.piecewiseProduct(this._inverseContentScaleFactor);
> 
> If you do transition to Point.scale(x,y), I would remove
> this._inverseContentScaleFactor and just use mousePoisitionView.scale(1 /
> this._contentScaleFactor.x, 1 / this._contentScaleFactor.y)
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:328
> > +        const delta = event.deltaY * WebInspector.ScrollableView.ScrollDeltaScale * deviceDirection;
> 
> Is it necessary to create this variable even though it's only used once?
> 
> > Source/WebInspectorUI/UserInterface/Views/ScrollableView.js:333
> > +            const contentOffsetX = mousePositionContent.x - (mousePositionView.x / this._contentScaleFactor.x) / this._zoomFactor;
> 
> See line 328.
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:32
> > +        this._element = element || document.createElement("div");
> 
> Am I being too paranoid, or would it be safe to add a check to make sure the
> typeof element is HTMLElement?

We don't typically make these types of checks on elements.
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:34
> > +        this._element.__view = this;
> 
> I'm a bit confused as to why this is necessary.  Purely for convenience?

This is an addition Joe and I discussed, to aid in inspecting the Inspector. It allows, for example, quick access to the view object associated with a DOM element logged to the console:

$1.__view

> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:62
> > +        console.assert(!view.element.parentNode, "Subview element already exists in the DOM.");
> 
> It seems to me like if any of these assertions are triggered that we should
> have a return.  Is there a reason that we don't?

Our defensive programming style is a little murky. In some places we return if a sanity check fails, but recently I've seen a trend toward omitting the return.
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:64
> > +        view._parentView = this;
> 
> Correct me if I'm wrong, but this isn't a layering violation because we are
> dealing with two objects (this and view) which are both WebInspector.View
> objects?  This just looks weird to me...

I'm not sure what both objects being WebInspector.Views has to do with it. Do you mean that it's a layering violation because one is accessing (and mutating) private data of the other? Private data is accessible to objects of the same class in languages such as C++, so I figured it was acceptable. Let me know if this violates established JS best practices.

> > Source/WebInspectorUI/UserInterface/Views/View.js:73
> > +        console.assert(view.element.parentNode === this.element, "Subview element should be a child of this view's element.");
> 
> Same as line 62.
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:76
> > +        view._parentView = null;
> 
> Same as line 64.
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:111
> > +        units = units || "%";
> 
> Couldn't you use a default parameter?  Also, why is it that you're
> defaulting to "%"?  We normally use "px" for our defaults (e.g. the computed
> panel).

[Devin Rousso]

(In reply to comment #8)
> > > Source/WebInspectorUI/UserInterface/Views/View.js:34
> > > +        this._element.__view = this;
> > 
> > I'm a bit confused as to why this is necessary.  Purely for convenience?
> 
> This is an addition Joe and I discussed, to aid in inspecting the Inspector.
> It allows, for example, quick access to the view object associated with a
> DOM element logged to the console:
> 
> $1.__view

I like this idea.

> > > Source/WebInspectorUI/UserInterface/Views/View.js:64
> > > +        view._parentView = this;
> > 
> > Correct me if I'm wrong, but this isn't a layering violation because we are
> > dealing with two objects (this and view) which are both WebInspector.View
> > objects?  This just looks weird to me...
> 
> I'm not sure what both objects being WebInspector.Views has to do with it.
> Do you mean that it's a layering violation because one is accessing (and
> mutating) private data of the other? Private data is accessible to objects
> of the same class in languages such as C++, so I figured it was acceptable.
> Let me know if this violates established JS best practices.

Ah ok yeah if we are following C++ then its fine.  I was just unsure of how we deal with this in JS.  Just a thought, that's all.

[Matt Baker]

(In reply to comment #9)
> (In reply to comment #8)
> > > > Source/WebInspectorUI/UserInterface/Views/View.js:34
> > > > +        this._element.__view = this;
> > > 
> > > I'm a bit confused as to why this is necessary.  Purely for convenience?
> > 
> > This is an addition Joe and I discussed, to aid in inspecting the Inspector.
> > It allows, for example, quick access to the view object associated with a
> > DOM element logged to the console:
> > 
> > $1.__view
> 
> I like this idea.
> 
> > > > Source/WebInspectorUI/UserInterface/Views/View.js:64
> > > > +        view._parentView = this;
> > > 
> > > Correct me if I'm wrong, but this isn't a layering violation because we are
> > > dealing with two objects (this and view) which are both WebInspector.View
> > > objects?  This just looks weird to me...
> > 
> > I'm not sure what both objects being WebInspector.Views has to do with it.
> > Do you mean that it's a layering violation because one is accessing (and
> > mutating) private data of the other? Private data is accessible to objects
> > of the same class in languages such as C++, so I figured it was acceptable.
> > Let me know if this violates established JS best practices.
> 
> Ah ok yeah if we are following C++ then its fine.  I was just unsure of how
> we deal with this in JS.  Just a thought, that's all.

We aren't following C++, I just used the precedent in that venerable language to inform my choice. We access private data in a few places in the Inspector (although it's relatively uncommon), and even with objects of unrelated types.

[Matt Baker]

Created attachment 263649 [details]
[Patch] WIP - has issues

Almost there, a few issues with sidebar panels need to be resolved.

[Devin Rousso]

Comment on attachment 263649 [details]
[Patch] WIP - has issues

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

> Source/WebInspectorUI/UserInterface/Views/View.js:92
> +        units = units || "%";

You could make this a default parameter.  Also, why "%" as the fallback instead of "px" (we use "px" for the computed panel)?

Also, in Source/WebInspectorUI/UserInterface/Views/TabBar.js on lines 137, 148, 247, 278, 360, 508, 519, and 628, you could merge the "classList.add" and "classList.remove" calls into one single line (unless there is some reason not to, like a layout).  Lastly, there were a few places of this (pretty much every time you call "classList.contains" in WebInspector.NavigationBar and WebInspector.TabBar), but we are trying to move away from keeping the state of objects in the DOM (according to Brian), so the state could be saved in a variable.  This could be handled in a different patch, but I just wanted to bring it to your attention.

[Matt Baker]

(In reply to comment #12)
> Comment on attachment 263649 [details]
> [Patch] WIP - has issues
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263649&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:92
> > +        units = units || "%";
> 
> You could make this a default parameter.  Also, why "%" as the fallback
> instead of "px" (we use "px" for the computed panel)?

I'll probably remove this from the next version of this patch. It's not uncommon in the UI to specify the size and position of child elements as a percentage of their parent element's width/height. To reduce duplication of this pattern it would be good to break out these type of utility functions, but it can wait.

> Also, in Source/WebInspectorUI/UserInterface/Views/TabBar.js on lines 137,
> 148, 247, 278, 360, 508, 519, and 628, you could merge the "classList.add"
> and "classList.remove" calls into one single line (unless there is some
> reason not to, like a layout).  Lastly, there were a few places of this
> (pretty much every time you call "classList.contains" in
> WebInspector.NavigationBar and WebInspector.TabBar), but we are trying to
> move away from keeping the state of objects in the DOM (according to Brian),
> so the state could be saved in a variable.  This could be handled in a
> different patch, but I just wanted to bring it to your attention.

While I agree, it isn't relevant here. The scope of this patch is limited to just the refactoring needed to get these view classes using the new View base class.

[Timothy Hatcher]

Comment on attachment 263649 [details]
[Patch] WIP - has issues

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

This is looking really good.

> Source/WebInspectorUI/UserInterface/Views/ConsolePrompt.js:125
> +        super.updateLayout();

Should super.updateLayout() be called at the end? In general the parent should update before the subviews. In this case (and in most cases) order isn't critical. But I think we should flip all the super calls to be safe.

> Source/WebInspectorUI/UserInterface/Views/View.js:47
> +        return this._subviews.slice();

I don't think we need to make a copy here. We don't typically do that and we trust clients to not mutate the arrays from getters.

> Source/WebInspectorUI/UserInterface/Views/View.js:78
> +        // Implemented by subclasses.

This comment should be first based on the comment above that subclasses should call super.updateLayout() last.

> Source/WebInspectorUI/UserInterface/Views/View.js:90
> +    updateElementPosition(element, value, propertyName, units)

Who uses this — nothing in this patch. Is it really needed? I think view subclasses can do this themselves if needed and it isn't common enough for the base class.

[Matt Baker]

(In reply to comment #14)
> Comment on attachment 263649 [details]
> [Patch] WIP - has issues
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263649&action=review
> 
> This is looking really good.
> 
> > Source/WebInspectorUI/UserInterface/Views/ConsolePrompt.js:125
> > +        super.updateLayout();
> 
> Should super.updateLayout() be called at the end? In general the parent
> should update before the subviews. In this case (and in most cases) order
> isn't critical. But I think we should flip all the super calls to be safe.

In most cases order doesn't matter, and I'm not aware of any places where child views *must* be updated before the parent. However, there are places where the parent view needs to set state on its child views before they are updated (see TimelineOverview). So I agree that this should be switched.

> > Source/WebInspectorUI/UserInterface/Views/View.js:47
> > +        return this._subviews.slice();
> 
> I don't think we need to make a copy here. We don't typically do that and we
> trust clients to not mutate the arrays from getters.
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:78
> > +        // Implemented by subclasses.
> 
> This comment should be first based on the comment above that subclasses
> should call super.updateLayout() last.

Agreed.
 
> > Source/WebInspectorUI/UserInterface/Views/View.js:90
> > +    updateElementPosition(element, value, propertyName, units)
> 
> Who uses this — nothing in this patch. Is it really needed? I think view
> subclasses can do this themselves if needed and it isn't common enough for
> the base class.

I'm going to remove it.

[Matt Baker]

Created attachment 263869 [details]
[Patch] WIP - introduces perf issue

Patch creates a *very* noticable performance impact on UI updates during timeline recording. Currently investigating.

[Blaze Burg]

Comment on attachment 263869 [details]
[Patch] WIP - introduces perf issue

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

What is the status of the performance regressions? (Not specific to this patch, but it is worrying that we don't have any concrete data, even at the per-backend-message level.)

> Source/WebInspectorUI/UserInterface/Views/View.js:100
> +        if (this._scheduledLayoutUpdateIdentifier) {

I don't think this is doing what you want. It will try to cancelAnimationFrame even in cases where the animation frame has called back normally. Perhaps you need to put the synchronous update in a private method? It would be even better to split the self-layout from the recursion, i.e.:

render()
{
    // Implemented by subclasses.
    // Not responsible for recursing to child views.
    // Should not be called directly; use updateLayout() instead.

    // Return value signals whether child views should also re-render.
    return true;
}

_renderSubtree()
{
    let shouldRenderChildren = this.render();
    if (!shouldRenderChildren)
        return;

    this._subviews.forEach((view) => { view._renderSubtree(); });
}

updateLayout()
{
    if (this._scheduledLayoutUpdateIdentifier) {
        cancelAnimationFrame(this._scheduledLayoutUpdateIdentifier);
        this._scheduledLayoutUpdateIdentifier = undefined;
    }

    this._renderSubtree();
}

needsLayout()
{
    if (this._scheduledLayoutUpdateIdentifier)
        return;
    this._scheduledLayoutUpdateIdentifier = requestAnimationFrame(this._renderSubtree.bind(this));
}

[Matt Baker]

(In reply to comment #17)
> Comment on attachment 263869 [details]
> [Patch] WIP - introduces perf issue
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263869&action=review
> 
> What is the status of the performance regressions? (Not specific to this
> patch, but it is worrying that we don't have any concrete data, even at the
> per-backend-message level.)
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:100
> > +        if (this._scheduledLayoutUpdateIdentifier) {
> 
> I don't think this is doing what you want. It will try to
> cancelAnimationFrame even in cases where the animation frame has called back
> normally. Perhaps you need to put the synchronous update in a private
> method? It would be even better to split the self-layout from the recursion,
> i.e.:

I was just discussing this with Tim the other day. I'd already made the change for the next path. I also like the application of a template method pattern here (https://en.wikipedia.org/wiki/Template_method_pattern). I'll incorporate your suggestions below, nice!

> render()
> {
>     // Implemented by subclasses.
>     // Not responsible for recursing to child views.
>     // Should not be called directly; use updateLayout() instead.
> 
>     // Return value signals whether child views should also re-render.
>     return true;
> }
> 
> _renderSubtree()
> {
>     let shouldRenderChildren = this.render();
>     if (!shouldRenderChildren)
>         return;
> 
>     this._subviews.forEach((view) => { view._renderSubtree(); });
> }
> 
> updateLayout()
> {
>     if (this._scheduledLayoutUpdateIdentifier) {
>         cancelAnimationFrame(this._scheduledLayoutUpdateIdentifier);
>         this._scheduledLayoutUpdateIdentifier = undefined;
>     }
> 
>     this._renderSubtree();
> }
> 
> needsLayout()
> {
>     if (this._scheduledLayoutUpdateIdentifier)
>         return;
>     this._scheduledLayoutUpdateIdentifier =
> requestAnimationFrame(this._renderSubtree.bind(this));
> }

[Matt Baker]

Created attachment 264256 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 264256 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/View.js:119
> +        this._scheduledLayoutUpdateIdentifier = requestAnimationFrame(this._renderSubtree.bind(this));

When the rAF fires this this should probably clear the _scheduledLayoutUpdateIdentifier. Otherwise, repeated calls to needsLayout would only ever render once?

[Blaze Burg]

(In reply to comment #20)
> Comment on attachment 264256 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264256&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:119
> > +        this._scheduledLayoutUpdateIdentifier = requestAnimationFrame(this._renderSubtree.bind(this));
> 
> When the rAF fires this this should probably clear the
> _scheduledLayoutUpdateIdentifier. Otherwise, repeated calls to needsLayout
> would only ever render once?

It should set it back to undefined, but not cancelAnimationFrame() as it was doing in the previous patch.

[Matt Baker]

(In reply to comment #21)
> (In reply to comment #20)
> > Comment on attachment 264256 [details]
> > [Patch] Proposed Fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=264256&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Views/View.js:119
> > > +        this._scheduledLayoutUpdateIdentifier = requestAnimationFrame(this._renderSubtree.bind(this));
> > 
> > When the rAF fires this this should probably clear the
> > _scheduledLayoutUpdateIdentifier. Otherwise, repeated calls to needsLayout
> > would only ever render once?
> 
> It should set it back to undefined, but not cancelAnimationFrame() as it was
> doing in the previous patch.

_renderSubtree could set it to undefined before doing anything else.

[Matt Baker]

Created attachment 264261 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 264262 [details]
[Patch] Proposed Fix

[Blaze Burg]

Comment on attachment 264262 [details]
[Patch] Proposed Fix

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

Nearly there, just a few more questions.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:418
> +        var currentContentView = this.currentContentView;

let

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:420
> +            currentContentView.updateLayout();

This seems to ignore the subview/render traversal pattern. Do we want the current content view to be a subview? I don't see any code to add/remove subviews here.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:427
>      _addContentViewElement(contentView)

It seems that we should be attaching and detaching subviews here.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:556
>          // Do not attempt to use offsetes if we're not attached to the document tree yet.

drive-by typo: offsets

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:582
>          this.dispatchEventToListeners(WebInspector.DataGrid.Event.DidLayout);

Where is this event used? If it's code that shouldn't run in the render() run loop, maybe we can make it async.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:-605
> -        this.updateLayout();

Weird that this used to be synchronous!

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:175
> +            this._navigationItems[i].updateLayout(true);

I would prefer we rename NavigationItem.updateLayout to render(). Then the NavigationBar's render() method should return false, since it has custom management of its children (even though they themselves are not View instances- should they be?).

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:164
> +        var oldStartTime = this._timelineRuler.startTime;

let

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:179
> +                dataGridNode = dataGridNode.traverseNextNode(true, null, true);

This is a little awkward since we will recurse to DataGrid.render(), but I guess that data grid wouldn't know on its own whether to call refreshGraph().

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:183
> +        // FIXME: once TimelineRuler inherits from View this can be removed.

File a bug.

> Source/WebInspectorUI/UserInterface/Views/ScriptTimelineView.js:205
> +                    dataGridNode.refreshIfNeeded();

See above.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:397
> +        var firstNormalTabItem = null;

let

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:447
> +        var visibleWidth = this.element.clientWidth;

let, let, let

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:182
> +        if (!this.visible)

I feel like this could be rolled into View.prototype.needsLayout. Would there be any problems?

> Source/WebInspectorUI/UserInterface/Views/View.js:54
> +    insertSubviewBefore(view, referenceView)

I never saw this coming, but I miss ObjC method syntax in cases like this. =)

> Source/WebInspectorUI/UserInterface/Views/View.js:124
> +        // Implemented by subclasses.

'Overridden'

> Source/WebInspectorUI/UserInterface/Views/View.js:138
> +        let shouldRenderChildren = this.render();

What are the pros/cons of adding more visibility checks to the base class methods?

[Timothy Hatcher]

Comment on attachment 264262 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:175
>> +            this._navigationItems[i].updateLayout(true);
> 
> I would prefer we rename NavigationItem.updateLayout to render(). Then the NavigationBar's render() method should return false, since it has custom management of its children (even though they themselves are not View instances- should they be?).

layout(), see below.

> Source/WebInspectorUI/UserInterface/Views/View.js:122
> +    render()

I'm late on this, but I think "layout" is a better name since it connects it to "updateLayout", etc. "render" implies something is drawn, which is not the case and it is harder to associate with "updateLayout".

[Matt Baker]

I don't think we want views to short-circuit the layout of child views. There isn't a precedent for this (that I've seen), and having to return true from every derived view's implementation of layout() is pretty tedious. It also requires the implementer to look at View.js if they forget what the bool signifies.

I'm going to drop it for now. Layouts will always cascade to child views (which is currently the case anyway), and in the future we can explore a more sophisticated mechanism for marking views dirty and traversing the view hierarchy.

[Blaze Burg]

(In reply to comment #27)
> I don't think we want views to short-circuit the layout of child views.
> There isn't a precedent for this (that I've seen), and having to return true
> from every derived view's implementation of layout() is pretty tedious. It
> also requires the implementer to look at View.js if they forget what the
> bool signifies.

This can be fixed with better-named symbols on the constructor.

> 
> I'm going to drop it for now. Layouts will always cascade to child views
> (which is currently the case anyway), and in the future we can explore a
> more sophisticated mechanism for marking views dirty and traversing the view
> hierarchy.

Sounds good to me.

[Matt Baker]

Comment on attachment 264262 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:582
>>          this.dispatchEventToListeners(WebInspector.DataGrid.Event.DidLayout);
> 
> Where is this event used? If it's code that shouldn't run in the render() run loop, maybe we can make it async.

It isn't being used; will remove. If there is a future need we can add events at the View level (WillLayout/DidLayout) or protected overridable view methods (willLayout/didLayout). AppKit includes the latter.

>> Source/WebInspectorUI/UserInterface/Views/View.js:138
>> +        let shouldRenderChildren = this.render();
> 
> What are the pros/cons of adding more visibility checks to the base class methods?

We should push visibility checks (and the closely related shown() and hidden() methods) into the base class in a follow up patch. I see two approaches here:

1. Make `visible` a read-only property of View, which is set by calling shown()/hidden().
2. Make `visible` writable and shown()/hidden() protected methods which are called in response to the visible state changing.

Since visibility checks are implemented ad-hoc throughout the UI, I'm not sure which approach is more prevalent. At first glance I like the second method, since the first requires that derived classes call super.shown()/super.hidden() in order to update the visible flag.

[Matt Baker]

Comment on attachment 264262 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:182
>> +        if (!this.visible)
> 
> I feel like this could be rolled into View.prototype.needsLayout. Would there be any problems?

See https://bugs.webkit.org/show_bug.cgi?id=150741.

[Devin Rousso]

Comment on attachment 264262 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:174
> +        for (var i = 0; i < this._navigationItems.length; ++i)

You could rewrite this as (also on line 178 and 193):
for (let item of this._navigationItems) {

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:177
> +        var totalItemWidth = 0;

let instead of var

Also, I think that this isn't your fault at all, but there are many cases of using classList.contains as a check for functionality.  I think we may want to go back and add member vars to keep track of the state of the DOM object instead of using classList sometime in the future (right Brian?).  Makes things much nicer to debug too (no trawling of the DOM) :)

[Blaze Burg]

> Also, I think that this isn't your fault at all, but there are many cases of
> using classList.contains as a check for functionality.  I think we may want
> to go back and add member vars to keep track of the state of the DOM object
> instead of using classList sometime in the future (right Brian?).  Makes
> things much nicer to debug too (no trawling of the DOM) :)

Indeed, but this will be in follow-ups.

[Matt Baker]

(In reply to comment #32)
> > Also, I think that this isn't your fault at all, but there are many cases of
> > using classList.contains as a check for functionality.  I think we may want
> > to go back and add member vars to keep track of the state of the DOM object
> > instead of using classList sometime in the future (right Brian?).  Makes
> > things much nicer to debug too (no trawling of the DOM) :)
> 
> Indeed, but this will be in follow-ups.

Agreed. Initially I was fixing these as I found them, but given the scope of this patch I think t's best to limit the urge to refactor.

[Matt Baker]

Created attachment 264453 [details]
[Patch] Proposed Fix

[Matt Baker]

(In reply to comment #34)
> Created attachment 264453 [details]
> [Patch] Proposed Fix

I removed the changes to TimelineRuler, since it's getting updated in a follow up: https://bugs.webkit.org/show_bug.cgi?id=150703.

[Matt Baker]

Created attachment 264706 [details]
[Patch] Proposed Fix

Updated an incorrect FIXME url

[Timothy Hatcher]

Comment on attachment 264706 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/View.js:137
> +        this._subviews.forEach((view) => { view._layoutSubtree(); });

Does this need to be forEach? We prefer for (..of..) now, it avoids the extra work of arrow function.

[Matt Baker]

Created attachment 264715 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 264715 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 264715

Committed r191976: <http://trac.webkit.org/changeset/191976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.