Bug 110819 - Web Inspector: Save/load timeline should preserve DOMContentLoaded and Load event markers
Summary: Web Inspector: Save/load timeline should preserve DOMContentLoaded and Load e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eugene Klyuchnikov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-25 16:12 PST by Tony Gentilcore
Modified: 2013-03-01 07:00 PST (History)
12 users (show)

See Also:


Attachments
Patch (4.95 KB, patch)
2013-02-26 09:44 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (6.08 KB, patch)
2013-02-26 21:38 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (6.22 KB, patch)
2013-02-26 22:39 PST, Eugene Klyuchnikov
pfeldman: review+
Details | Formatted Diff | Diff
Patch (6.22 KB, patch)
2013-03-01 04:02 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2013-02-25 16:12:35 PST
1. Record a timeline that includes a page load. Notice the presence of DOMContentLoaded and Load event markers.
2. Right click and "Save Timeline data..."
3. Close inspector, open a new inspector, right click on timeline and "Load Timeline data..."
4. Notice DOMContentLoaded and Load event markers are gone.

I expect DOMContentLoaded and Load event markers should survive saving and loading the timeline.
Comment 1 Eugene Klyuchnikov 2013-02-26 09:44:24 PST
Created attachment 190315 [details]
Patch
Comment 2 Build Bot 2013-02-26 12:31:34 PST
Comment on attachment 190315 [details]
Patch

Attachment 190315 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16781302

New failing tests:
inspector/timeline/timeline-load-event.html
inspector/timeline/timeline-dom-content-loaded-event.html
Comment 3 WebKit Review Bot 2013-02-26 13:39:05 PST
Comment on attachment 190315 [details]
Patch

Attachment 190315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16782211

New failing tests:
inspector/timeline/timeline-load-event.html
inspector/timeline/timeline-dom-content-loaded-event.html
Comment 4 Build Bot 2013-02-26 16:39:03 PST
Comment on attachment 190315 [details]
Patch

Attachment 190315 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16769558

New failing tests:
inspector/timeline/timeline-dom-content-loaded-event.html
inspector/timeline/timeline-load-event.html
Comment 5 Eugene Klyuchnikov 2013-02-26 21:38:40 PST
Created attachment 190438 [details]
Patch
Comment 6 Ilya Tikhonovsky 2013-02-26 21:45:38 PST
Comment on attachment 190438 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Save/load timeline should preserve DOMContentLoaded and Load event markers

Please provide Web Inspector prefix.

> Source/WebCore/ChangeLog:8
> +        Store "isMainFrame" flag in MarkLoad and MarkDOMConatent records data.

It is not clear from the description what was the root of problem and how it was fixed.
Comment 7 Eugene Klyuchnikov 2013-02-26 22:35:09 PST
Analysis:
 TimelinePresentationModel creates event dividers for MarkLoad and MarkDOMContent only if they are fired for main frame.
 It uses resource resourceTreeModel to check if given frame id is main frame id.
 This is not good because at time check is performed, main frame could have changed (for example when we load saved timeline data).
 With this patch check is performed at time record is created and result is saved as a part of record.
Comment 8 Eugene Klyuchnikov 2013-02-26 22:38:44 PST
Comment on attachment 190438 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Save/load timeline should preserve DOMContentLoaded and Load event markers
> 
> Please provide Web Inspector prefix.

Fixed.

>> Source/WebCore/ChangeLog:8
>> +        Store "isMainFrame" flag in MarkLoad and MarkDOMConatent records data.
> 
> It is not clear from the description what was the root of problem and how it was fixed.

Fixed.
Comment 9 Eugene Klyuchnikov 2013-02-26 22:39:41 PST
Created attachment 190447 [details]
Patch
Comment 10 Pavel Feldman 2013-03-01 01:16:18 PST
Comment on attachment 190447 [details]
Patch

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

> Source/WebCore/inspector/TimelineRecordFactory.cpp:190
> +    data->setBoolean("mainFrame", mainFrame);

isMainFrame
Comment 11 Eugene Klyuchnikov 2013-03-01 04:02:57 PST
Created attachment 190929 [details]
Patch
Comment 12 Eugene Klyuchnikov 2013-03-01 04:04:10 PST
Comment on attachment 190447 [details]
Patch

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

>> Source/WebCore/inspector/TimelineRecordFactory.cpp:190
>> +    data->setBoolean("mainFrame", mainFrame);
> 
> isMainFrame

Done
Comment 13 Eugene Klyuchnikov 2013-03-01 04:06:41 PST
Committed r144439: <http://trac.webkit.org/changeset/144439>