Bug 132754 - Web Inspector: empty timeline should not use previous timeline's zoom interval
Summary: Web Inspector: empty timeline should not use previous timeline's zoom interval
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-05-09 15:51 PDT by Brian Burg
Modified: 2015-05-15 20:49 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.19 KB, patch)
2014-08-08 17:05 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (4.00 KB, patch)
2015-05-13 19:17 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (8.18 KB, patch)
2015-05-14 19:56 PDT, 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 Brian Burg 2014-05-09 15:51:11 PDT
As is, I feel this is really really confusing.

We could save timeline zoom intervals the same way we save other view state for back-forward navigations, but for unrelated page executions I don't see the use case.
Comment 1 Radar WebKit Bug Importer 2014-05-09 15:51:38 PDT
<rdar://problem/16872739>
Comment 2 Timothy Hatcher 2014-05-09 16:12:06 PDT
"unrelated page executions" is the key phase here. I find keeping the zoom level is useful for reload cases. Restoring a sane default for new / unrelated loads sounds fine.
Comment 3 Brian Burg 2014-08-08 17:03:04 PDT
This should be per-TimelineRecording, with a sane default.
Comment 4 Brian Burg 2014-08-08 17:05:14 PDT
Created attachment 236325 [details]
Patch
Comment 5 Timothy Hatcher 2014-08-09 20:59:55 PDT
Comment on attachment 236325 [details]
Patch

I think we should attempt to keep the state when reloading at minimum. If you are trying to see somthing happen on load it is annoying to keep fiddling with the zoom and selection. Maybe we need a hybrid approach here? First timeline uses saved settings, subsequent timelines get a sane default at the beginning of time? Maybe secondsPerPixel can always persist?
Comment 6 Brian Burg 2014-08-09 22:00:58 PDT
If the recording is explicitly started via the button, then any subsequent reload navigations do not appear at the start of the recording. So I think you are suggesting that we reuse the same zoom setting for auto-capturing sessions on the same page as the previous auto-capturing session.
Comment 7 Timothy Hatcher 2014-08-09 23:29:24 PDT
Correct. Only the auto-recorded timeline.
Comment 8 Timothy Hatcher 2015-02-10 09:46:00 PST
Comment on attachment 236325 [details]
Patch

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

Lets fix this up and make reload reuse the previous settings. Otherwise I am on board with doing this.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:71
> +    this.selectionDuration = 5;

5 seconds seems short now. 10 or 15?
Comment 9 Matt Baker 2015-05-13 19:17:40 PDT
Created attachment 253082 [details]
[Patch] Proposed Fix
Comment 10 Matt Baker 2015-05-13 19:52:58 PDT
Wrong patch, disregard.
Comment 11 Matt Baker 2015-05-14 19:56:11 PDT
Created attachment 253169 [details]
[Patch] Proposed Fix
Comment 12 Joseph Pecoraro 2015-05-15 19:46:24 PDT
Comment on attachment 253169 [details]
[Patch] Proposed Fix

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

r=me

> Source/WebInspectorUI/ChangeLog:23
> +        Increase default selection time to 15 seconds.

I'm surprised by this, but I'll trust your judgement!
Comment 13 Simon Fraser (smfr) 2015-05-15 20:04:51 PDT
Yay!
Comment 14 WebKit Commit Bot 2015-05-15 20:48:55 PDT
Comment on attachment 253169 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 253169

Committed r184429: <http://trac.webkit.org/changeset/184429>
Comment 15 WebKit Commit Bot 2015-05-15 20:49:01 PDT
All reviewed patches have been landed.  Closing bug.