Bug 195317 - Web Inspector: recordsInTimeRange sometimes does not get the expected record when includeRecordBeforeStart
Summary: Web Inspector: recordsInTimeRange sometimes does not get the expected record ...
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: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-05 00:58 PST by Joseph Pecoraro
Modified: 2019-03-05 16:41 PST (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (1.87 KB, patch)
2019-03-05 00:59 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (3.13 MB, application/zip)
2019-03-05 03:09 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-03-05 00:58:35 PST
recordsInTimeRange sometimes does not get the expected record when includeRecordBeforeStart

For example, when the before record is a composite of a bunch of paints:

    - Composite:
        - Paint: ...
        - Paint: ...
        - Paint: ...
        - Paint: ...

Currently we get the inner Paint, but that isn't as useful as the Composite, which may be much longer in duration.

Steps to Reproduce:
1. Record CPU Timeline on maps.google.com zooming in/out
2. Select part of the large composite and view the CPU Timeline graph
  => Main Thread indicator doesn't see paints when it should
Comment 1 Joseph Pecoraro 2019-03-05 00:59:09 PST
Created attachment 363616 [details]
[PATCH] Proposed Fix
Comment 2 Devin Rousso 2019-03-05 02:13:37 PST
Comment on attachment 363616 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:116
> +                lowerIndex--;

You could make this into a do-while :)

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:117
> +                while (this._records[lowerIndex] !== recordBefore.parent)

Is it guaranteed that we'd eventually reach the parent record, or should we also add `&& this._records[lowerIndex].type === recordBefore.parent.type` so it's a bit more restrictive?
Comment 3 Build Bot 2019-03-05 03:09:07 PST
Comment on attachment 363616 [details]
[PATCH] Proposed Fix

Attachment 363616 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11375599

New failing tests:
fast/scrolling/ios/hit-testing-iframe-002.html
fast/images/imagemap-polygon-focus-ring.html
fast/scrolling/ios/hit-testing-iframe-003.html
editing/selection/thai-word-at-document-end.html
fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html
Comment 4 Build Bot 2019-03-05 03:09:09 PST
Created attachment 363629 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 5 Joseph Pecoraro 2019-03-05 16:14:39 PST
Comment on attachment 363616 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/Timeline.js:116
>> +                lowerIndex--;
> 
> You could make this into a do-while :)

True, I thought this was clearer.

>> Source/WebInspectorUI/UserInterface/Models/Timeline.js:117
>> +                while (this._records[lowerIndex] !== recordBefore.parent)
> 
> Is it guaranteed that we'd eventually reach the parent record, or should we also add `&& this._records[lowerIndex].type === recordBefore.parent.type` so it's a bit more restrictive?

We already did this check up above: `recordBefore.parent && recordBefore.parent.type === recordBefore.type`.

Once they are the same type, then yes it should be guaranteed that the parent is in the list (it should have started earlier then the child).
Comment 6 WebKit Commit Bot 2019-03-05 16:40:50 PST
Comment on attachment 363616 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 363616

Committed r242520: <https://trac.webkit.org/changeset/242520>
Comment 7 WebKit Commit Bot 2019-03-05 16:40:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-03-05 16:41:24 PST
<rdar://problem/48620478>