[webkit-reviews] review granted: [Bug 189773] Web Inspector: create special Network waterfall for media events : [Attachment 351856] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 10 15:51:56 PDT 2018


Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 189773: Web Inspector: create special Network waterfall for media events
https://bugs.webkit.org/show_bug.cgi?id=189773

Attachment 351856: Patch

https://bugs.webkit.org/attachment.cgi?id=351856&action=review




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 351856
  --> https://bugs.webkit.org/attachment.cgi?id=351856
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:467
> +Object.defineProperty(Array.prototype, "adjacentForEach",

I still don't see value in this. If I read this I'd think it would run callback
with both the adjacent elements, something like (before, current, next)

    ( undefi, arr[0], arr[1] )
    ( arr[0], arr[1], arr[2] )
    ( arr[1], arr[2], arr[3] )
    ( arr[2], arr[3], undefi )

The fact that it doesn't do what I expect is reason to not make it a top level
function like this. Another reason is that it is a single for loop.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1845
> +	       if (!entry.resource)
> +		   return
WI.Rect.rectFromClientRect(targetElement.getBoundingClientRect());

I still feel like this will fall over in some way but I'm not exactly sure how
to test. Maybe some kind of table update that happens while the popover is
showing. (An incomplete request becoming complete?)

> LayoutTests/http/tests/inspector/dom/didFireEvent.html:18
> +    let videoNode = null;

Style: I normally drop the assignment on these "values to be populated"

    let videoNode;


More information about the webkit-reviews mailing list