Bug 153731 – Web Inspector: add a LayoutReason enum to the View base class

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
Add an updateLayoutForResize method to the View base class. A handful of classes cache the size/position of DOM elements to avoid unnecessary reflows during layout. It would be useful to have a generic way to inform a view that it's size has changed, and to propagate this to the view's children.

[Radar WebKit Bug Importer]

<rdar://problem/24430938>

[Matt Baker]

Created attachment 270355 [details]
[Patch] Proposed Fix

[Blaze Burg]

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

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

> Source/WebInspectorUI/ChangeLog:42
> +        Overridden by views that need to perform resize logic, such as

If this is always going to happen in the same event loop as the actual layout, what's the benefit of splitting this from updateLayoutForResize implementations?

[Blaze Burg]

(In reply to comment #3)
> Comment on attachment 270355 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270355&action=review
> 
> > Source/WebInspectorUI/ChangeLog:42
> > +        Overridden by views that need to perform resize logic, such as
> 
> If this is always going to happen in the same event loop as the actual
> layout, what's the benefit of splitting this from updateLayoutForResize
> implementations?

Another way of thinking about this might be: can we pass a LayoutUpdateReason enum {Resize, Dirty} as an argument to updateLayout(), and would this be clearer than two separate methods with an implicit contract about the calling order? My hunch is yes, but maybe I'm missing something.

[Matt Baker]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 270355 [details]
> > [Patch] Proposed Fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=270355&action=review
> > 
> > > Source/WebInspectorUI/ChangeLog:42
> > > +        Overridden by views that need to perform resize logic, such as
> > 
> > If this is always going to happen in the same event loop as the actual
> > layout, what's the benefit of splitting this from updateLayoutForResize
> > implementations?
> 
> Another way of thinking about this might be: can we pass a
> LayoutUpdateReason enum {Resize, Dirty} as an argument to updateLayout(),
> and would this be clearer than two separate methods with an implicit
> contract about the calling order? My hunch is yes, but maybe I'm missing
> something.

I like this suggestion. The optional argument can be passed to needsLayout as well as updateLayout, making it more flexible than this approach. There used to exist an updateLayoutForResize method prior to all the View refactoring, which is what I initially based this on.

[Matt Baker]

Created attachment 270444 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 270446 [details]
[Patch] Proposed Fix

[Blaze Burg]

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

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

r=me, but please check that TimelineRuler has not turned into a performance problem.

> Source/WebInspectorUI/ChangeLog:41
> +        update cached client width if resized.

Nit: 'Update'

> Source/WebInspectorUI/UserInterface/Views/View.js:304
> +    Dirty: Symbol("layout-layoutReason-dirty"),

Nit: view-layout-reason-dirty

[Matt Baker]

(In reply to comment #8)
> Comment on attachment 270446 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270446&action=review
> 
> r=me, but please check that TimelineRuler has not turned into a performance
> problem.

Did ad hoc testing, looks okay!

> 
> > Source/WebInspectorUI/ChangeLog:41
> > +        update cached client width if resized.
> 
> Nit: 'Update'
> 
> > Source/WebInspectorUI/UserInterface/Views/View.js:304
> > +    Dirty: Symbol("layout-layoutReason-dirty"),
> 
> Nit: view-layout-reason-dirty

Find-replace error. Ugh.

[Matt Baker]

Created attachment 270451 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 270451

Committed r195995: <http://trac.webkit.org/changeset/195995>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.