Bug 153731 - Web Inspector: add a LayoutReason enum to the View base class
Summary: Web Inspector: add a LayoutReason enum to the View base class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: 153032
  Show dependency treegraph
 
Reported: 2016-01-31 16:41 PST by Matt Baker
Modified: 2016-02-01 17:27 PST (History)
8 users (show)

See Also:


Attachments
[Patch] Proposed Fix (9.40 KB, patch)
2016-01-31 17:15 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (12.75 KB, patch)
2016-02-01 16:06 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (12.78 KB, patch)
2016-02-01 16:10 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (12.77 KB, patch)
2016-02-01 16:37 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2016-01-31 16:41:36 PST
* 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.
Comment 1 Radar WebKit Bug Importer 2016-01-31 16:42:11 PST
<rdar://problem/24430938>
Comment 2 Matt Baker 2016-01-31 17:15:22 PST
Created attachment 270355 [details]
[Patch] Proposed Fix
Comment 3 Brian Burg 2016-02-01 06:00:57 PST
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?
Comment 4 Brian Burg 2016-02-01 09:40:18 PST
(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.
Comment 5 Matt Baker 2016-02-01 14:45:15 PST
(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.
Comment 6 Matt Baker 2016-02-01 16:06:15 PST
Created attachment 270444 [details]
[Patch] Proposed Fix
Comment 7 Matt Baker 2016-02-01 16:10:32 PST
Created attachment 270446 [details]
[Patch] Proposed Fix
Comment 8 Brian Burg 2016-02-01 16:32:37 PST
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
Comment 9 Matt Baker 2016-02-01 16:33:43 PST
(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.
Comment 10 Matt Baker 2016-02-01 16:37:45 PST
Created attachment 270451 [details]
[Patch] Proposed Fix
Comment 11 WebKit Commit Bot 2016-02-01 17:27:30 PST
Comment on attachment 270451 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 270451

Committed r195995: <http://trac.webkit.org/changeset/195995>
Comment 12 WebKit Commit Bot 2016-02-01 17:27:34 PST
All reviewed patches have been landed.  Closing bug.