1439888 - Define new test helper method - performRequests

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This is a follow up for bug 1419350
https://bugzilla.mozilla.org/show_bug.cgi?id=1419350#c3

Net monitor tests are often using the following construct:

let wait = waitForNetworkEvents(monitor, 2);
await ContentTask.spawn(tab.linkedBrowser, {}, () => {
  content.wrappedJSObject.performRequests(2);
});
await wait;

It would be great if we have a helper in head.js

async function performRequests(monitor, tab, count) {
  let wait = waitForNetworkEvents(monitor, count);
  await ContentTask.spawn(tab.linkedBrowser, count, requestCount => {
    content.wrappedJSObject.performRequests(requestCount);
  });
  await wait;
}

... and use it everywhere in tests that are using the construct.

Honza

Comment 1

[Shivansh]

Hey, may I work on this bug?
just to clear things: we have to change the code for all the files in 
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test
having the given construct. is that right?
Shivansh

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #1)
> Hey, may I work on this bug?
Sure!

> just to clear things: we have to change the code for all the files in 
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test
> having the given construct. is that right?
Correct

All the tests should use simply call:
`await performRequests(monitor, tab, <actual count>)`

Honza

Comment 3

[Shivansh]

Hey, looking into the files I found that in some of the cases 'yield' has been used instead of 'await'. So do I also have to change the construct in which 'yield' has been used?(like this file: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_accessibility-01.js#33-37)
Secondly, some files already have 'performRequests' defined locally, so I believe I have to remove those local function also and call the function defined in the head.js file.
Thirdly, since I have to make changes in multiple files, how exactly do I have to submit these files(or patch)?

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #3)
> Hey, looking into the files I found that in some of the cases 'yield' has
> been used instead of 'await'. So do I also have to change the construct in
> which 'yield' has been used?(like this file:
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/
> browser_net_accessibility-01.js#33-37)
Keep the yield in place for now. We are replacing yield by await step by step
and it can be done as part of another report.

> Secondly, some files already have 'performRequests' defined locally, so I
> believe I have to remove those local function also and call the function
> defined in the head.js file.
Yes

> Thirdly, since I have to make changes in multiple files, how exactly do I
> have to submit these files(or patch)?
Just create a patch using:
hg diff > mypatch.patch

and attach the file here in the bug.

If you use HG queues you can:
hg push -f review
...to review board.

---

Some links

Read more about review board:
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

How to submit a patch
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Submitting review requests:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

Honza

Comment 5

[Shivansh]

Attachment: mypatch.patch

Helper function added in head.js, and added function call in the files that were using that function.

Comment 6

[Shivansh]

Hey, I have attached the patch here. I was not able to push review as I am not able to add my IRC nick.

I have edited the 'head.js' file, and I found the constructor in only one file 'browser_net_view-source-debugger.js', so made the required changes in it.

Let me know if I am missing something or need to make some more changes in the patch.

Comment 7

[Shivansh]

Hey, can you please update me with progress of the bug.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #5)
> Created attachment 8953114 [details] [diff] [review]
> mypatch.patch
> 
> Helper function added in head.js, and added function call in the files that
> were using that function.

You need to set the review to `?` to ask for a review.
The reviewer is consequently using `+` to indicate that the patch is ok or `-` if not

Honza

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8953114 [details] [diff] [review]
mypatch.patch

Review of attachment 8953114 [details] [diff] [review]:
-----------------------------------------------------------------

Thank for working on this!

Please see my inline comments.

Also, we want to use the new  helper in tests that are implementing own version of the function.

You can search the codebase (devtools/client/netmonitor/test) for `performRequests`
to find all such tests. And consequently manually check (in case when the test would
use different name)

Honza

::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js
@@ +16,4 @@
>    let { document, store, windowRequire } = monitor.panelWin;
>    let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>    store.dispatch(Actions.batchEnable(false));
> +  

Remove whitespace characters on this line

@@ +16,5 @@
>    let { document, store, windowRequire } = monitor.panelWin;
>    let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>    store.dispatch(Actions.batchEnable(false));
> +  
> +  //helper function moved to head.js

There must be a space between `//` and the first letter
The first letter should be capitalized.

@@ +22,1 @@
>  

Remove whitespaces on this line.

::: devtools/client/netmonitor/test/head.js
@@ +758,5 @@
>    await onResponseContent;
>  }
> +
> +
> +async function performRequests(monitor, tab, count) {

A comment in front of the function would be helpful. Something like:


/**
 * Helper function for executing XHRs on a test page.
 * 
 * @param {Number} count Number of requests to be executed.
 */

Comment 10

[Shivansh]

Hey,

> Also, we want to use the new  helper in tests that are implementing own
> version of the function.
> 
> You can search the codebase (devtools/client/netmonitor/test) for
> `performRequests`
> to find all such tests. And consequently manually check (in case when the
> test would
> use different name)

I am unable to find files other than the file "browser_net_view-source-debugger.js" having the said construct.
Even after using several keywords to search(using grep) I get the same file only.

Can you please help me to find other files?

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

When searching for `performRequests` in C:\src\mozilla.org\mozilla-central\devtools\client\netmonitor\test\

I am seeing usage in:
* browser_net_accessibility-01.js
* browser_net_accessibility-02.js
* browser_net_api-calls.js
* browser_net_api-calls.js (already has local helper)
* browser_net_brotli.js
* browser_net_cached-status.js (already has local helper)
* browser_net_cause_redirect.js (already has local helper)
* browser_net_complex-params.js:
* browser_net_content-type.js:
* browser_net_copy_image_as_data_uri.js:
* browser_net_copy_params.js:
* ... and many others

See my full search results here:
https://pastebin.com/gAJB40J0

Do you see it now?

Honza

Comment 12

[Shivansh]

Hey, surely these files use performRequest but these don't use the construct given in the description.
Mostly, they are using the 'yield' instead of 'await', rest all other lines are same as that of the given construct.
So should I make a different helper for the construct which are using 'yield'?


I am not sure whether the HTML files having the performRequest function are concerned here or not, 
as they are not using the given construct.


And lastly, I believe that the helper 'performRequest' and "content.wrappedJSObject.'performRequests()'" 
are both different things, and we are only concerned about the helper function. Right?

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #12)
> Hey, surely these files use performRequest but these don't use the construct
> given in the description.
> Mostly, they are using the 'yield' instead of 'await', rest all other lines
> are same as that of the given construct.
> So should I make a different helper for the construct which are using
> 'yield'?
There should be just one helper and tests using yield should be adopted to use await.

An existing test:

add_task(function* () {
  // ...
  let wait = waitForNetworkEvents(monitor, 2);
  yield ContentTask.spawn(tab.linkedBrowser, {}, function* () {
    content.wrappedJSObject.performRequests(2);
  });
  yield wait;
  // ...
});

Should be converted into:

add_task(async () => {
  // ...
  await performRequests(monitor, 2);
  // ...
});

Btw. it's also preferred to use arrow-function

> I am not sure whether the HTML files having the performRequest function are
> concerned here or not, as they are not using the given construct.
Correct, the helper function isn't related to HTML files.

> And lastly, I believe that the helper 'performRequest' and
> "content.wrappedJSObject.'performRequests()'" 
> are both different things, and we are only concerned about the helper
> function. Right?
Yes

Honza

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #12)
> Mostly, they are using the 'yield' instead of 'await', rest all other lines
> are same as that of the given construct.

Good news is that bug 1440321 is replacing `yield` by `await`, so it'll make
work on this bug easier. Please wait for the patch/patches in bug 1440321
before finishing this bug.

Honza

Comment 15

[Shivansh]

(In reply to Jan Honza Odvarko [:Honza] from comment #14)
 
> Good news is that bug 1440321 is replacing `yield` by `await`, 

Finally, it is happening :P.
I guess we will have to wait now before we wrap this up.

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #15)
> I guess we will have to wait now before we wrap this up.
Bug 1443470 is fixed, so we can move forward with this one.
Honza

Comment 17

[Shivansh]

Attachment: mypatch2.patch

Hey, I have fixed most of the files. Some of the files were calling content.wrappedJSObject.performRequests(args, url), i.e. with multiple arguments.
So if I could get some definition for the above function I can try to fix the remaining files also(if its needed).

Please let me know if the patch is missing something, other than the above mentioned.

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Great!

I tried to apply the patch, but I am seeing a lot of conflicts. Are you sure you are working with the latest m-c?


See some of them:
$ hg qpush
applying perform-requests.patch
patching file devtools/client/netmonitor/test/browser_net_accessibility-01.js
Hunk #1 FAILED at 29
Hunk #2 FAILED at 52
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_accessibility-01.js.rej
patching file devtools/client/netmonitor/test/browser_net_accessibility-02.js
Hunk #1 FAILED at 29
Hunk #2 FAILED at 54
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_accessibility-02.js.rej
patching file devtools/client/netmonitor/test/browser_net_api-calls.js
Hunk #1 FAILED at 28
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_api-calls.js.rej
patching file devtools/client/netmonitor/test/browser_net_brotli.js
Hunk #1 FAILED at 24
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_brotli.js.rej
patching file devtools/client/netmonitor/test/browser_net_cause_redirect.js


I was quickly looking at the new `performRequests` function. What is the `obj` argument for? It's not being used.

There are two patches attached at the moment. Should you mark the first one as obsolete?

Honza


Honza

Comment 19

[Shivansh]

Attachment: mypatch3.patch

Comment 20

[Shivansh]

(In reply to Jan Honza Odvarko [:Honza] from comment #18)

> I tried to apply the patch, but I am seeing a lot of conflicts. Are you sure
> you are working with the latest m-c?
Apparently, I wasn't doing 'hg update' after 'hg pull'.


> I was quickly looking at the new `performRequests` function. What is the
> `obj` argument for? It's not being used.
I used another argument so that 'performRequest' can accept arguments for which, in place of, '{}' has an url,
or some other count. But for now, I have ignored these constructs.
 
> There are two patches attached at the moment. Should you mark the first one
> as obsolete?
Done!.


I have submitted another patch considering all the above things. Please let me know if I am still missing something.

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8958149 [details] [diff] [review]
mypatch3.patch

Review of attachment 8958149 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks fro the patch, looks great!

Just a few comments inline.

Honza

::: devtools/client/netmonitor/test/browser_net_accessibility-01.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +50,5 @@
>    store.dispatch(Actions.selectDelta(-10));
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_accessibility-02.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +52,5 @@
>    EventUtils.sendKey("HOME", window);
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_api-calls.js
@@ +29,5 @@
>      "http://example.com/api/search/?q=search%E2%98%A2"
>    ];
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 5)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_resend.js
@@ +25,5 @@
>  
>    store.dispatch(Actions.batchEnable(false));
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2);

This test is still using generator, so you need to use yield.

::: devtools/client/netmonitor/test/head.js
@@ +749,5 @@
> + * Helper function for executing XHRs on a test page.
> + *
> + * @param {Number} count Number of requests to be executed.
> + */
> +async function performRequests(monitor, tab, count) {

Please put the name of the function in the list of exported globals at the top of this file.

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8958149 [details] [diff] [review]
mypatch3.patch

Review of attachment 8958149 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks fro the patch, looks great!

Just a few comments inline.

Honza

::: devtools/client/netmonitor/test/browser_net_accessibility-01.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +50,5 @@
>    store.dispatch(Actions.selectDelta(-10));
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_accessibility-02.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +52,5 @@
>    EventUtils.sendKey("HOME", window);
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_api-calls.js
@@ +29,5 @@
>      "http://example.com/api/search/?q=search%E2%98%A2"
>    ];
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 5)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_image-tooltip.js
@@ +35,5 @@
>    info("Reloading the debuggee and performing all requests again...");
>    await triggerActivity(ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED);
> +  // Execute requests.
> +  // +1 extra document reload
> +  await performRequests(monitor, tab, IMAGE_TOOLTIP_REQUESTS + 1);

The refactoring isn't right, the test now fails on time out (never finishes).

You can run the test locally by:

./mach test devtools/client/netmonitor/test/browser_net_image-tooltip.js

::: devtools/client/netmonitor/test/browser_net_resend.js
@@ +25,5 @@
>  
>    store.dispatch(Actions.batchEnable(false));
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2);

This test is still using generator, so you need to use yield.

::: devtools/client/netmonitor/test/browser_net_send-beacon.js
@@ -19,5 @@
>    is(store.getState().requests.requests.size, 0, "The requests menu should be empty.");
>  
> -  let wait = waitForNetworkEvents(monitor, 1);
> -  await ContentTask.spawn(tab.linkedBrowser, {}, async function () {
> -    content.wrappedJSObject.performRequest();

The original name of the content method is 'performRequest'. The method is defined in html_send-beacon.html

It needs to be renamed to 'performRequests'

::: devtools/client/netmonitor/test/head.js
@@ +749,5 @@
> + * Helper function for executing XHRs on a test page.
> + *
> + * @param {Number} count Number of requests to be executed.
> + */
> +async function performRequests(monitor, tab, count) {

Please put the name of the function in the list of exported globals at the top of this file.

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8958149 [details] [diff] [review]
mypatch3.patch

Review of attachment 8958149 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks fro the patch, looks great!

Just a few comments inline.

Honza

::: devtools/client/netmonitor/test/browser_net_accessibility-01.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +50,5 @@
>    store.dispatch(Actions.selectDelta(-10));
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_accessibility-02.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +52,5 @@
>    EventUtils.sendKey("HOME", window);
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_api-calls.js
@@ +29,5 @@
>      "http://example.com/api/search/?q=search%E2%98%A2"
>    ];
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 5)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_image-tooltip.js
@@ +35,5 @@
>    info("Reloading the debuggee and performing all requests again...");
>    await triggerActivity(ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED);
> +  // Execute requests.
> +  // +1 extra document reload
> +  await performRequests(monitor, tab, IMAGE_TOOLTIP_REQUESTS + 1);

The refactoring isn't right, the test now fails on time out (never finishes).

You can run the test locally by:

./mach test devtools/client/netmonitor/test/browser_net_image-tooltip.js

::: devtools/client/netmonitor/test/browser_net_resend.js
@@ +25,5 @@
>  
>    store.dispatch(Actions.batchEnable(false));
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2);

This test is still using generator, so you need to use yield.

::: devtools/client/netmonitor/test/browser_net_send-beacon.js
@@ -19,5 @@
>    is(store.getState().requests.requests.size, 0, "The requests menu should be empty.");
>  
> -  let wait = waitForNetworkEvents(monitor, 1);
> -  await ContentTask.spawn(tab.linkedBrowser, {}, async function () {
> -    content.wrappedJSObject.performRequest();

The original name of the content method is 'performRequest'. The method is defined in html_send-beacon.html

It needs to be renamed to 'performRequests'

::: devtools/client/netmonitor/test/head.js
@@ +749,5 @@
> + * Helper function for executing XHRs on a test page.
> + *
> + * @param {Number} count Number of requests to be executed.
> + */
> +async function performRequests(monitor, tab, count) {

Please put the name of the function in the list of exported globals at the top of this file.

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Sorry, I published the comments more times, pleas read the last one.

Honza

Comment 25

[Shivansh]

Attachment: mypatch4.patch

Comment 26

[Shivansh]

Hey, I've added all the required changes in the new patch.

I am not able to test the file. The error is as follows: 
"Exception: Binary expected at /home/shivansh/src/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox does not exist."
Even after several 'hg pull' and 'hg update', I am not able to get the binary.

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #26)
> Hey, I've added all the required changes in the new patch.
> 
> I am not able to test the file. The error is as follows: 
> "Exception: Binary expected at
> /home/shivansh/src/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox
> does not exist."
> Even after several 'hg pull' and 'hg update', I am not able to get the
> binary.
You need to build Firefox using:

./mach build


You can put:

ac_add_options --enable-artifact-builds

...into your .mozconfig file to make the build a lot faster.

Honza

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #26)
> Even after several 'hg pull' and 'hg update', I am not able to get the
> binary.

Btw. I am usually using the following command to update my source working directory:

hg pull -u

... which executes both 'hg pull' and 'hg update'


Honza

Comment 29

[Shivansh]

(In reply to Jan Honza Odvarko [:Honza] from comment #27)
> You need to build Firefox using:
> 
> ./mach build
Thanks, I am now able to test the files.

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> ::: devtools/client/netmonitor/test/browser_net_image-tooltip.js
> @@ +35,5 @@
> >    info("Reloading the debuggee and performing all requests again...");
> >    await triggerActivity(ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED);
> > +  // Execute requests.
> > +  // +1 extra document reload
> > +  await performRequests(monitor, tab, IMAGE_TOOLTIP_REQUESTS + 1);
> 
> The refactoring isn't right, the test now fails on time out (never finishes).
> 
> You can run the test locally by:
> 
> ./mach test devtools/client/netmonitor/test/browser_net_image-tooltip.js
I am attaching a working patch for this file. Please review it.

Comment 30

[Shivansh]

Attachment: br_net_img_ttip.patch

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Here is try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6874ae7cf779413adad19df38895afe9c2f2872a&selectedJob=168150118

I am still seeing some tests to fail.
There are also some eslint errors that needs to be fixed.

Mainly: "Unexpected space before function parentheses." (it's new rule introduced recently, so that's why it appeared now)

Also, please merge both patches together, thanks!

Honza

Comment 32

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8958511 [details] [diff] [review]
mypatch4.patch

Review of attachment 8958511 [details] [diff] [review]:
-----------------------------------------------------------------

Unassigning myself from the review for now till mentioned issues are fixed.

Comment 33

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8958902 [details] [diff] [review]
br_net_img_ttip.patch

Review of attachment 8958902 [details] [diff] [review]:
-----------------------------------------------------------------

Unassigning myself from the review for now till mentioned issues are fixed.

Honza

Comment 34

[Shivansh]

Attachment: mypatch5.patch

Comment 35

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8959774 [details] [diff] [review]
mypatch5.patch

Review of attachment 8959774 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, looks good, thanks for working on this!

R+

Try push looks ok
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9dc2373389f11bf66ec4bff8d007f5b2e816fe5

Honza

Comment 36

[Shivansh]

Hey, happy to know that my patch is acceptable. So, is there something still left to be done? or the bug has been resolved?

Comment 37

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Shivansh from comment #36)
> Hey, happy to know that my patch is acceptable. So, is there something still
> left to be done? or the bug has been resolved?

The next step is to set a flag checkin-needed into the Keyword field (it's the one with good-first-bug flag, you need to separate them by a comma). This way, sheriffs are informed about pending patches and can commit them. 

Contributors with commit access can commit patches with R+ manually.

So, please set the flag and wait till somebody lands this into m-c repo.

Thanks for the help!

Honza

Comment 38

[Cosmin Sabou [:CosminS]]

This patch failed to apply. Please take a look.

applying mypatch5.patch
patching file devtools/client/netmonitor/test/browser_net_params_sorted.js
Hunk #1 FAILED at 15
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_params_sorted.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh mypatch5.patch

Comment 39

[Shivansh]

Attachment: mypatch6.patch

Hey, this should solve the above problem.

Comment 40

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8961348 [details] [diff] [review]
mypatch6.patch

Review of attachment 8961348 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me.

R+

Thanks!
Honza

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2247b1e7324f
Define new performRequests test helper method. r=Honza

Comment 42

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/2247b1e7324f