Bug 162099 - Web Inspector: adopt Object.awaitEvent in LayoutTests/inspector/network
Summary: Web Inspector: adopt Object.awaitEvent in LayoutTests/inspector/network
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
: 162360 (view as bug list)
Depends on:
Blocks: 161634
  Show dependency treegraph
 
Reported: 2016-09-16 16:41 PDT by Devin Rousso
Modified: 2016-12-13 15:37 PST (History)
5 users (show)

See Also:


Attachments
Patch (7.44 KB, patch)
2016-09-16 16:45 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2016-09-16 16:41:05 PDT
.
Comment 1 Radar WebKit Bug Importer 2016-09-16 16:41:53 PDT
<rdar://problem/28346596>
Comment 2 Devin Rousso 2016-09-16 16:45:17 PDT
Created attachment 289137 [details]
Patch
Comment 3 Brian Burg 2016-09-19 07:56:18 PDT
Comment on attachment 289137 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2016-09-19 13:47:29 PDT
Comment on attachment 289137 [details]
Patch

Clearing flags on attachment: 289137

Committed r206112: <http://trac.webkit.org/changeset/206112>
Comment 5 WebKit Commit Bot 2016-09-19 13:47:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Ryan Haddad 2016-09-22 11:22:06 PDT
Reverted r206112 for reason:

This change made inspector/network/xhr-json-blob-has-content.html very flaky.

Committed r206263: <http://trac.webkit.org/changeset/206263>
Comment 7 Ryan Haddad 2016-09-22 11:23:02 PDT
*** Bug 162360 has been marked as a duplicate of this bug. ***
Comment 8 Joseph Pecoraro 2016-09-22 12:07:02 PDT
Comment on attachment 289137 [details]
Patch

I really find these changes much harder to read then the original test. The original test read:

  "do this, cause and do this"

Now they read:

  "prepare for some stuff, do this"

Am I alone in this?
Comment 9 Brian Burg 2016-09-22 14:06:01 PDT
(In reply to comment #8)
> Comment on attachment 289137 [details]
> Patch
> 
> I really find these changes much harder to read then the original test. The
> original test read:
> 
>   "do this, cause and do this"
> 
> Now they read:
> 
>   "prepare for some stuff, do this"
> 
> Am I alone in this?

I think so? It's not always possible to make the test linear like that, then the promise chain kicks off at some random awkward part of the test. I prefer making the test explicitly event-based with the trigger at the end rather than implicitly depending on evaluating script being asynchronous. That way, the test is structured the same way (prep + trigger) whether the initial part of the promise chain is triggered by synchronous events from WI.Object or from async backend commands that have responses or cause events.