1530282 - Replace promiseWaitForEvent in browser_visibleFindSelection.js with BrowserTestUtils.waitForEvent

Status: RESOLVED FIXED

(Firefox :: General, task, P5)

Description

[Johann Hofmann [:johannh]]

This is a good first bug for newcomers to Firefox development.

promiseWaitForEvent in the browser_visibleFindSelection.js test file can be replaced by the BrowserTestUtils.waitForEvent utility function.

The code in question is here: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/base/content/test/general/browser_visibleFindSelection.js#13

For instructions on how to get your local build of Firefox up and running and submit your patch, see https://developer.mozilla.org/en-US/docs/Introduction.

You can run this test with the ./mach mochitest command:

./mach mochitest browser/base/content/test/general/browser_visibleFindSelection.js

Please leave a comment if you would like to be assigned to this bug and feel free to ask questions here or via IRC if you're stuck.

Comment 1

[Aashna]

Hey, please assign this to me!

Comment 2

[Johann Hofmann [:johannh]]

Hi Aashna, this bug is yours. Let me know how it goes.

Comment 3

[lloan alas:[lloanalas]]

(In reply to Aashna from comment #1)

Hey, please assign this to me!

Hey Aashna, are you still interested in this bug? If not, I'd be interested in getting it done. Thanks!

Comment 4

[Aashna]

I'm in contact with Johann and I'm working on it

Comment 5

[lloan alas:[lloanalas]]

Sounds good!

Let me know if you need any help then, I'd be happy to help :)

Thanks!

Comment 6

[lloan alas:[lloanalas]]

(In reply to Aashna from comment #4)

I'm in contact with Johann and I'm working on it

Hey Aashna,

Did you need help submitting your patch? I could walk you through the process, more than happy to help you with your first bug.

Comment 7

[Aashna]

Hey, could you please help me? I'm unsure about how to make a new patch and submit it

Comment 8

[Aashna]

I made the changes and ran the test, it shows passed. Please review the patch and let me know.

Comment 9

[lloan alas:[lloanalas]]

(In reply to Aashna from comment #8)

I made the changes and ran the test, it shows passed. Please review the patch and let me know.

@Aashna I'll see if I can find you on slack to walk you through how to make a patch for submission. Doesn't look like it got attached to the bug.

Comment 10

[Aashna]

Sure, how can I reach you? Do I need to join a slack channel?

Comment 11

[Johann Hofmann [:johannh]]

Hi Aashna, are you still working on this? Do you need any more help? :)

Thanks!

Comment 12

[Arun Kumar Mohan]

(In reply to Johann Hofmann [:johannh] (Away until July 3rd) from comment #0)

This is a good first bug for newcomers to Firefox development.

promiseWaitForEvent in the browser_visibleFindSelection.js test file can be replaced by the BrowserTestUtils.waitForEvent utility function.

The code in question is here: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/base/content/test/general/browser_visibleFindSelection.js#13

For instructions on how to get your local build of Firefox up and running and submit your patch, see https://developer.mozilla.org/en-US/docs/Introduction.

You can run this test with the ./mach mochitest command:

./mach mochitest browser/base/content/test/general/browser_visibleFindSelection.js

Please leave a comment if you would like to be assigned to this bug and feel free to ask questions here or via IRC if you're stuck.

I would like to take a stab at this issue.

Comment 13

[lloan alas:[lloanalas]]

(In reply to Arun Kumar Mohan from comment #12)

(In reply to Johann Hofmann [:johannh] (Away until July 3rd) from comment #0)

This is a good first bug for newcomers to Firefox development.

promiseWaitForEvent in the browser_visibleFindSelection.js test file can be replaced by the BrowserTestUtils.waitForEvent utility function.

The code in question is here: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/base/content/test/general/browser_visibleFindSelection.js#13

For instructions on how to get your local build of Firefox up and running and submit your patch, see https://developer.mozilla.org/en-US/docs/Introduction.

You can run this test with the ./mach mochitest command:

./mach mochitest browser/base/content/test/general/browser_visibleFindSelection.js

Please leave a comment if you would like to be assigned to this bug and feel free to ask questions here or via IRC if you're stuck.

I would like to take a stab at this issue.

Hi Arun, thanks for taking a look at this! I can't assign things myself and :johann is out until July 3rd. I would just submit a patch and have it ready for when he comes back. Let me know if you need help, :).

Thanks!

clearing needinfo

Comment 14

[Arun Kumar Mohan]

Attachment: Bug 1530282 - Replace promiseWaitForEvent with BrowserTestUtils.waitForEvent

Comment 15

[Arun Kumar Mohan]

(In reply to lloan alas:[lloanalas] from comment #13)

(In reply to Arun Kumar Mohan from comment #12)

(In reply to Johann Hofmann [:johannh] (Away until July 3rd) from comment #0)

This is a good first bug for newcomers to Firefox development.

promiseWaitForEvent in the browser_visibleFindSelection.js test file can be replaced by the BrowserTestUtils.waitForEvent utility function.

The code in question is here: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/base/content/test/general/browser_visibleFindSelection.js#13

For instructions on how to get your local build of Firefox up and running and submit your patch, see https://developer.mozilla.org/en-US/docs/Introduction.

You can run this test with the ./mach mochitest command:

./mach mochitest browser/base/content/test/general/browser_visibleFindSelection.js

Please leave a comment if you would like to be assigned to this bug and feel free to ask questions here or via IRC if you're stuck.

I would like to take a stab at this issue.

Hi Arun, thanks for taking a look at this! I can't assign things myself and :johann is out until July 3rd. I would just submit a patch and have it ready for when he comes back. Let me know if you need help, :).

Thanks!

clearing needinfo

Ah, I see. I am new to Mercurial and Phabricator but I have managed to get my changes on Phabricator.

https://phabricator.services.mozilla.com/D35979

Let's wait for Johann to respond, I guess.

Comment 16

[Arun Kumar Mohan]

Hey Johann!

Do you mind reviewing my patch when you get a chance?

Thanks!

Comment 17

[Johann Hofmann [:johannh]]

Sure!

Comment 18

[Johann Hofmann [:johannh]]

Thanks for the contribution! I'm setting the checkin-needed flag to get someone to land this patch for us.

Comment 19

[Arun Kumar Mohan]

@Johann Awesome! That was quick. Thanks again!

Comment 20

[Cosmin Sabou [:CosminS]]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpfNoJEb browser/base/content/test/general/browser_visibleFindSelection.js Hunk #1 FAILED at 10. Hunk #2 FAILED at 53. 2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/test/general/browser_visibleFindSelection.js.rej abort: patch command failed: exited with status 256

Comment 21

[Arun Kumar Mohan]

(In reply to Cosmin Sabou [:CosminS] from comment #20)

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpfNoJEb browser/base/content/test/general/browser_visibleFindSelection.js Hunk #1 FAILED at 10. Hunk #2 FAILED at 53. 2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/test/general/browser_visibleFindSelection.js.rej abort: patch command failed: exited with status 256

I have updated the revision. Do let me know if I missed anything.

Thanks.

Comment 22

[Cosmin Sabou [:CosminS]]

Arun, this still needs to be looked at.
Please manually rebase your commits and try again. applying /tmp/tmp77uaDL browser/base/content/test/general/browser_visibleFindSelection.js Hunk #1 FAILED at 10. Hunk #2 FAILED at 53. 2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/test/general/browser_visibleFindSelection.js.rej abort: patch command failed: exited with status 256

Comment 23

[Arun Kumar Mohan]

(In reply to Cosmin Sabou [:CosminS] from comment #22)

Arun, this still needs to be looked at.
Please manually rebase your commits and try again. applying /tmp/tmp77uaDL browser/base/content/test/general/browser_visibleFindSelection.js Hunk #1 FAILED at 10. Hunk #2 FAILED at 53. 2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/test/general/browser_visibleFindSelection.js.rej abort: patch command failed: exited with status 256

I tried running hg pull --rebase but the output says there are no new changes. I am probably missing something. Do you mind helping me get unstuck?

Comment 24

[Johann Hofmann [:johannh]]

Did you upload the latest revision to Phabricator?

Comment 25

[Cosmin Sabou [:CosminS]]

We use moz-phab to get our patches up to phabricator
moz-phab patch <patch-id> --apply-to . (to apply patch on local repository)
make changes to the file(s) we want
hg commit
and then upload it with |moz-phab| command. Hope this helps.
moz-phab documentation here: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Arun, this is the output of the .rej file if it helps:

+++ browser_visibleFindSelection.js
@@ -10,7 +10,7 @@
 
   let remote = gBrowser.selectedBrowser.isRemoteBrowser;
 
-  let findBarOpenPromise = promiseWaitForEvent(gBrowser, "findbaropen");
+  let findBarOpenPromise = BrowserTestUtils.waitForEvent(gBrowser, "findbaropen");
   EventUtils.synthesizeKey("f", {accelKey: true});
   await findBarOpenPromise;
 
@@ -53,4 +53,3 @@
   gFindBar.close();
   gBrowser.removeCurrentTab();
 });
-

Comment 26

[Arun Kumar Mohan]

(In reply to Johann Hofmann [:johannh] from comment #24)

Did you upload the latest revision to Phabricator?

Done! https://phabricator.services.mozilla.com/D35979

Comment 27

[Cosmin Sabou [:CosminS]]

Arun, this still failed when trying to land:

browser/base/content/test/general/browser_visibleFindSelection.js
Hunk #1 FAILED at 10.
Hunk #2 FAILED at 53.
2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/test/general/browser_visibleFindSelection.js.rej
abort: patch command failed: exited with status 256

Comment 28

[Arun Kumar Mohan]

Attachment: Bug 1530282 - Replace promiseWaitForEvent with BrowserTestUtils.waitForEvent

Depends on D35979

Comment 29

[Arun Kumar Mohan]

Attachment: Bug 1530282 - Replace promiseWaitForEvent with BrowserTestUtils.waitForEvent

Comment 30

[Arun Kumar Mohan]

(In reply to Cosmin Sabou [:CosminS] from comment #27)

Arun, this still failed when trying to land:

browser/base/content/test/general/browser_visibleFindSelection.js
Hunk #1 FAILED at 10.
Hunk #2 FAILED at 53.
2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/test/general/browser_visibleFindSelection.js.rej
abort: patch command failed: exited with status 256

It was my bad. I then tried to rebase the changes but my branch was lost (I probably messed it up because of poor understanding of Mercurial). I ended up creating a new revision. Hopefully, things go fine this time.

@Johann Sorry to bother you. Do you mind reviewing https://phabricator.services.mozilla.com/D37663?

Comment 31

[Johann Hofmann [:johannh]]

You still need to run ./mach eslint browser/ --fix :)

Comment 32

[Arun Kumar Mohan]

Thanks for accepting the revision, Johann. Any updates on this, Cosmin?

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/157079d7e7cc
Replace promiseWaitForEvent with BrowserTestUtils.waitForEvent r=johannh

Comment 34

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/157079d7e7cc

Comment 35

[Cosmin Sabou [:CosminS]]

Thanks Raul for landing this.

Comment 36

[Arun Kumar Mohan]

Woohoo! Thank you Johann, Cosmin, and Raul. Working on this was really fun!

Looking forward to more :)