Closed Bug 1530282 Opened 5 years ago Closed 5 years ago

Replace promiseWaitForEvent in browser_visibleFindSelection.js with BrowserTestUtils.waitForEvent

Categories

(Firefox :: General, task, P5)

Product:
Firefox
Component:
General
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: johannh, Assigned: arunmohandm, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

Bug 1530282 - Replace promiseWaitForEvent with BrowserTestUtils.waitForEvent
47 bytes, text/x-phabricator-request
Details | Review

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.

Hey, please assign this to me!

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

Assignee: nobody → jenaaashna
Status: NEW → ASSIGNED

(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!

Flags: needinfo?(jenaaashna)

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

Flags: needinfo?(jenaaashna)

Sounds good!

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

Thanks!

(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.

Flags: needinfo?(jenaaashna)

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

Flags: needinfo?(jenaaashna)

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

(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.

Flags: needinfo?(jenaaashna)

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

Flags: needinfo?(jenaaashna)

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

Thanks!

Flags: needinfo?(jenaaashna)
Assignee: jenaaashna → nobody
Status: ASSIGNED → NEW
Type: enhancement → task
Flags: needinfo?(jenaaashna)

(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.

Flags: needinfo?(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

Flags: needinfo?(lloanalas)

(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.

Hey Johann!

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

Thanks!

Flags: needinfo?(jhofmann)

Sure!

Assignee: nobody → arunmohandm
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)

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

Keywords: checkin-needed

@Johann Awesome! That was quick. Thanks again!

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

Flags: needinfo?(arunmohandm)
Keywords: checkin-needed

(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.

Flags: needinfo?(arunmohandm)

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

Flags: needinfo?(arunmohandm)

(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?

Flags: needinfo?(arunmohandm) → needinfo?(csabou)

Did you upload the latest revision to Phabricator?

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();
 });
-
Flags: needinfo?(csabou) → needinfo?(arunmohandm)

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

Did you upload the latest revision to Phabricator?

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

Flags: needinfo?(arunmohandm)

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
Flags: needinfo?(arunmohandm)

Depends on D35979

Attachment #9077238 - Attachment is obsolete: true
Attachment #9074093 - Attachment is obsolete: true

(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?

Flags: needinfo?(jhofmann)
Flags: needinfo?(csabou)
Flags: needinfo?(arunmohandm)
Flags: needinfo?(csabou)

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

Flags: needinfo?(jhofmann)

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

Flags: needinfo?(csabou)
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/157079d7e7cc
Replace promiseWaitForEvent with BrowserTestUtils.waitForEvent r=johannh

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Thanks Raul for landing this.

Flags: needinfo?(csabou)

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

Looking forward to more :)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: