Closed Bug 1443470 Opened 5 years ago Closed 5 years ago

Convert devtools/client/netmonitor from Task.jsm/yield to async/await

Categories

(DevTools :: Netmonitor, defect)

Product:
DevTools
Component:
Netmonitor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(3 files, 3 obsolete files)

Bug 1443470 - Convert Task.jsm to async/await in devtools/client/netmonitor.
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1443470 - Fix wrong assertion in browser_net_simple-request-data.js.
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1443470 - Fix race when hovering status column in browser_net_filter-flags.js.
59 bytes, text/x-review-board-request
Honza
: review+
Details 
In order to help bug 1439888, I'll try to land Task.jsm refactoring for the netmonitor first as getting everything right for the whole client folder is challenging.
Assignee: nobody → poirot.alex
Green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=43a2e93e5a35131597f340a76bc59a35fc8cfe15
Looks like we can land netmonitor refactoring independently.
Quick comment about all these patches:
* the first patch is informational, I won't land it. It contains the xpcshell script used to convert the codebase automatically.
  The important parts are the whitelists, listing all files that shouldn't be modified.
  For the netmonitor, the only whitelist is related to one test, to be addressed as a followup in bug 1441792.
* the two next patches are fully automated. You can see the command line I used in changeset description.
  the first converts Task.async to async, while the second converts all the generators to async functions.
* "Put spaces before parentheses in 'async function()'" is here to fix eslint error as the xpcshell script doesn't rewrite with the right eslint rules.
* the two last ones addressed test failures introduced by the switch to async/await. TBH I haven't tried to understand why it started failing and just fixed them.
Comment on attachment 8956412 [details]
Bug 1443470 - Convert Task.jsm to async/await in devtools/client/netmonitor.

https://reviewboard.mozilla.org/r/225306/#review231570

Looks good to me
R+ assuming try is green
Honza
Attachment #8956412 - Flags: review?(odvarko) → review+
Comment on attachment 8956413 [details]
Bug 1443470 - Convert generators to async functions in devtools/client/netmonitor

https://reviewboard.mozilla.org/r/225308/#review231572
Attachment #8956413 - Flags: review?(odvarko) → review+
Comment on attachment 8956414 [details]
Bug 1443470 - Put spaces before parentheses in "async function()" pattern in devtools/client/netmonitor.

https://reviewboard.mozilla.org/r/225310/#review231574
Attachment #8956414 - Flags: review?(odvarko) → review+
Comment on attachment 8956415 [details]
Bug 1443470 - Fix wrong assertion in browser_net_simple-request-data.js.

https://reviewboard.mozilla.org/r/225312/#review231576

Passes when running localy
R+
Honza

::: devtools/client/netmonitor/test/browser_net_simple-request-data.js:280
(Diff revision 1)
>          "GET",
>          SIMPLE_SJS,
>          {
>            type: "plain",
>            fullMimeType: "text/plain; charset=utf-8",
> -          transferred: L10N.getFormatStrWithNumbers("networkMenu.sizeB", 342),
> +          transferred: L10N.getFormatStrWithNumbers("networkMenu.sizeB", 347),

How come this worked before?
Attachment #8956415 - Flags: review?(odvarko) → review+
Comment on attachment 8956416 [details]
Bug 1443470 - Fix race when hovering status column in browser_net_filter-flags.js.

https://reviewboard.mozilla.org/r/225314/#review231578

5089 checks green in this test ;-)

R+

Thanks for working on this Alex!
Honza
Attachment #8956416 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> ::: devtools/client/netmonitor/test/browser_net_simple-request-data.js:280
> (Diff revision 1)
> >          "GET",
> >          SIMPLE_SJS,
> >          {
> >            type: "plain",
> >            fullMimeType: "text/plain; charset=utf-8",
> > -          transferred: L10N.getFormatStrWithNumbers("networkMenu.sizeB", 342),
> > +          transferred: L10N.getFormatStrWithNumbers("networkMenu.sizeB", 347),
> 
> How come this worked before?

Yes, this one is really, really surprising!
Attachment #8956411 - Attachment is obsolete: true
Attachment #8956413 - Attachment is obsolete: true
Attachment #8956414 - Attachment is obsolete: true
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c105a8573f4d0e06d368b595524f06f37c56c393 -d 87a34b1b1ad7: rebasing 451044:c105a8573f4d "Bug 1443470 - Convert Task.jsm to async/await in devtools/client/netmonitor. r=Honza"
merging devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js
merging devtools/client/netmonitor/test/browser_net_filter-01.js
merging devtools/client/netmonitor/test/browser_net_filter-02.js
merging devtools/client/netmonitor/test/browser_net_filter-03.js
merging devtools/client/netmonitor/test/browser_net_filter-04.js
merging devtools/client/netmonitor/test/browser_net_filter-flags.js
merging devtools/client/netmonitor/test/browser_net_sort-01.js
merging devtools/client/netmonitor/test/browser_net_sort-02.js
merging devtools/client/netmonitor/test/head.js
warning: conflicts while merging devtools/client/netmonitor/test/browser_net_filter-01.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/netmonitor/test/browser_net_filter-02.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/netmonitor/test/browser_net_filter-04.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/netmonitor/test/browser_net_filter-flags.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18d68a0c96e6
Convert Task.jsm to async/await in devtools/client/netmonitor. r=Honza
https://hg.mozilla.org/integration/autoland/rev/c1bfcc7294c5
Fix wrong assertion in browser_net_simple-request-data.js. r=Honza
https://hg.mozilla.org/integration/autoland/rev/1040f6405dd9
Fix race when hovering status column in browser_net_filter-flags.js. r=Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> ::: devtools/client/netmonitor/test/browser_net_simple-request-data.js:280
> (Diff revision 1)
> >          "GET",
> >          SIMPLE_SJS,
> >          {
> >            type: "plain",
> >            fullMimeType: "text/plain; charset=utf-8",
> > -          transferred: L10N.getFormatStrWithNumbers("networkMenu.sizeB", 342),
> > +          transferred: L10N.getFormatStrWithNumbers("networkMenu.sizeB", 347),
> 
> How come this worked before?

Ok, so I took a minute to see what was going on.
Before this patch we were not asserting the tooltip, nor tons of other assertions.
We go from 50 up to 254 assertions with the async refactoring.

That's because we don't yield on a generator function:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_simple-request-data.js#271-284
      verifyRequestItemTarget(
        document,
        getDisplayedRequests(store.getState()),
        requestItem,
        "GET",
        SIMPLE_SJS,
        {
          type: "plain",
          fullMimeType: "text/plain; charset=utf-8",
          transferred: L10N.getFormatStrWithNumbers("networkMenu.sizeB", 342),
          size: L10N.getFormatStrWithNumbers("networkMenu.sizeB", 12),
        }
      );
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/head.js#398
function* verifyRequestItemTarget(document, requestList, requestItem, method,
                                 url, data = {}) {
  info("> Verifying: " + method + " " + url + " " + data.toSource());

And so, verifyRequestItemTarget wasn't called :/
Whereas the async refactoring converted verifyRequestItemTarget to a regular function as it wasn't yielding anything.
This is a regression from bug 1418927.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.