Bug 150703 - Web Inspector: Convert TimelineRuler to View base class
Summary: Web Inspector: Convert TimelineRuler to 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: 149186
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-29 18:59 PDT by Matt Baker
Modified: 2015-11-05 13:18 PST (History)
8 users (show)

See Also:


Attachments
[Patch] Proposed Fix (29.74 KB, patch)
2015-11-03 14:16 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (29.74 KB, patch)
2015-11-03 15:33 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (29.72 KB, patch)
2015-11-03 16:07 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (29.98 KB, patch)
2015-11-05 12:30 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 2015-10-29 18:59:46 PDT
* SUMMARY
Convert TimelineRuler to View base class. TimelineRuler can update markers and selection separately from the rest of its content, which may require additional layout logic in View.js.
Comment 1 Radar WebKit Bug Importer 2015-10-29 19:00:08 PDT
<rdar://problem/23326721>
Comment 2 Matt Baker 2015-11-03 14:16:55 PST
Created attachment 264722 [details]
[Patch] Proposed Fix
Comment 3 Timothy Hatcher 2015-11-03 14:56:30 PST
Comment on attachment 264722 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:340
> -        var visibleWidth = this._recalculate();
> +        let visibleWidth = this.element.clientWidth;

Why this change?

> Source/WebInspectorUI/UserInterface/Views/View.js:44
> +    get isLayoutPending()

This should be a function if it is prefixed with "is".
Comment 4 Matt Baker 2015-11-03 15:28:38 PST
(In reply to comment #3)
> Comment on attachment 264722 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264722&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:340
> > -        var visibleWidth = this._recalculate();
> > +        let visibleWidth = this.element.clientWidth;
> 
> Why this change?

I think this was an error.

> > Source/WebInspectorUI/UserInterface/Views/View.js:44
> > +    get isLayoutPending()
> 
> This should be a function if it is prefixed with "is".

What about changing to "has"? We prefix getters with both "is" and "has", the latter being slightly more common.
Comment 5 Matt Baker 2015-11-03 15:33:57 PST
Created attachment 264740 [details]
[Patch] Proposed Fix
Comment 6 Timothy Hatcher 2015-11-03 16:00:04 PST
Comment on attachment 264740 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/View.js:44
> +    get hasPendingLayout()

Getters should have no verb prefix, including "has". Properties should be about the state of the object, not a question being asked. A better property name would be "layoutScheduled" or "layoutPending". Otherwise this should be a function with an "is" or "has" prefix.
Comment 7 Matt Baker 2015-11-03 16:07:51 PST
Created attachment 264747 [details]
[Patch] Proposed Fix
Comment 8 BJ Burg 2015-11-05 11:55:33 PST
Comment on attachment 264747 [details]
[Patch] Proposed Fix

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

r=me, nice work

> Source/WebInspectorUI/ChangeLog:7
> +

Please add a summary (from the bug is fine).
Comment 9 Matt Baker 2015-11-05 12:30:13 PST
Created attachment 264878 [details]
[Patch] Proposed Fix
Comment 10 WebKit Commit Bot 2015-11-05 13:18:36 PST
Comment on attachment 264878 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 264878

Committed r192070: <http://trac.webkit.org/changeset/192070>
Comment 11 WebKit Commit Bot 2015-11-05 13:18:40 PST
All reviewed patches have been landed.  Closing bug.