Closed Bug 1132556 Opened 10 years ago Closed 9 years ago

Replace runInContent() with ContentTask.spawn()

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: smacleod, Assigned: ajkerrigan, Mentored)

References

Details

(Whiteboard: [good first bug])

User Story

Session Restore tests have a helper function runInContent()[1] which is used to execute functions in the Content Process (There is documentation on MDN which is helpful for developing multi-process firefox[2][3]).

Recently Bug 1107609 added a more general ContentTask.spawn()[4] method for executing functions in the content process. We should replace all of the uses of runInContent()[5] with the new method, and then remove the implementation of runInContent()[1] so that no new tests use it.

The two methods take slightly different arguments, so it will be a little more complex than just replacing the method name, but should still be pretty simple.

The affected tests can be executed from the mozilla-central checkout by running:
 $ ./mach mochitest-browser browser/components/sessionstore/test/

Or, the tests that currently use runInContent can be run individually (and more quickly) by running these commands:
 $ ./mach mochitest-browser browser/components/sessionstore/test/browser_447951.js
 $ ./mach mochitest-browser browser/components/sessionstore/test/browser_500328.js

A successful patch for this bug will have 
- All uses of runInContent replaced and adapted to the new ContentTask.spawn method
- The definition of runInContent removed
- All the sessionstore mochitest-browser tests passing.

[1] https://dxr.mozilla.org/mozilla-central/search?q=%22function+runInContent%22+path%3Abrowser%2Fcomponents%2Fsessionstore%2Ftest%2F*&redirect=true
[2] https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox
[3] https://developer.mozilla.org/en-US/Add-ons/Working_with_multiprocess_Firefox

[4] https://hg.mozilla.org/integration/fx-team/file/2483a2d08384/testing/mochitest/BrowserTestUtils/ContentTask.jsm#l52
[5] https://dxr.mozilla.org/mozilla-central/search?q=%22runInContent%22&redirect=true

Attachments

(1 file, 3 obsolete files)

Now that Bug 1107609 has landed we should replace runInContent() uses with ContentTask.spawn() and then remove it from head.js
Flags: needinfo?(mhoye)
Flags: firefox-backlog+
User Story: (updated)
Flags: qe-verify-
Nice.
Flags: needinfo?(mhoye)
Whiteboard: [good first bug]
I have my environment set up and I am working on this bug. Is that alright?
(In reply to cbazoian12 from comment #2) > I have my environment set up and I am working on this bug. Is that alright? Yes, that's great! I've assigned the bug to you. Please let me know when you have any questions or would like help.
Assignee: nobody → cbazoian12
Status: NEW → ASSIGNED
Awesome. Thank you
Hey cbazoian12, I wanted to check in and see how you're progressing with this bug? Is there anything I can do to help you moving forward?
Flags: needinfo?(cbazoian12)
(In reply to Steven MacLeod [:smacleod] from comment #5) > Hey cbazoian12, > > I wanted to check in and see how you're progressing with this bug? Is there > anything I can do to help you moving forward? Just to make sure since I've never submitted a patch before, do I submit the diff file as the patch? And once I have the diff file, how do you want me to submit it?
Flags: needinfo?(cbazoian12)
(In reply to cbazoian12 from comment #6) > Just to make sure since I've never submitted a patch before, do I submit the > diff file as the patch? And once I have the diff file, how do you want me to > submit it? Yes you'll submit the diff file as the patch. You submit it by uploading it to this bug. For more specifics about how to do this we have some great documentation[1]. Let me know if you have any more questions! [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
Perfect. Thank you.
Attached patch Patch for bug 1132556 (obsolete) — Splinter Review
I am hoping I did this right. I attached the patch with the diff file in it.
Flags: needinfo?(smacleod)
The patch that was attached doesn't appear to be your changes, it looks like a diff introducing a bunch of the files that already exist in the repository. (In the future you'll also want to mark review?smacleod@mozilla.com rather than needinfo?smacleod@mozilla.com). What command are you using to generate this patch for upload?
Flags: needinfo?(smacleod) → needinfo?(cbazoian12)
(In reply to Steven MacLeod [:smacleod] from comment #10) > The patch that was attached doesn't appear to be your changes, it looks like > a diff introducing a bunch of the files that already exist in the > repository. (In the future you'll also want to mark > review?smacleod@mozilla.com rather than needinfo?smacleod@mozilla.com). > > What command are you using to generate this patch for upload? I was using the link http://jungels.net/articles/diff-patch-ten-minutes.html. But I believe I followed the instructions wrong. I'll go back and try again.
Flags: needinfo?(cbazoian12)
(In reply to cbazoian12 from comment #11) > I was using the link > http://jungels.net/articles/diff-patch-ten-minutes.html. But I believe I > followed the instructions wrong. I'll go back and try again. Since firefox development uses mercurial, you'll want to generate your patches using mercurial itself (rather than using the diff and patch commands). This can be done using the "hg export" command. In order to ensure mercurial is configured properly though you'll want to first run "./mach mercurial-setup" inside the repository clone.
(In reply to Steven MacLeod [:smacleod] from comment #12) > (In reply to cbazoian12 from comment #11) > > I was using the link > > http://jungels.net/articles/diff-patch-ten-minutes.html. But I believe I > > followed the instructions wrong. I'll go back and try again. > > Since firefox development uses mercurial, you'll want to generate your > patches using mercurial itself (rather than using the diff and patch > commands). This can be done using the "hg export" command. > > In order to ensure mercurial is configured properly though you'll want to > first run "./mach mercurial-setup" inside the repository clone. So the problem I ran into is that I already made the changes to the files before starting the process of making a diff file. How do I roll back those changes so that I can make a diff file?
Flags: needinfo?(smacleod)
(In reply to cbazoian12 from comment #13) > So the problem I ran into is that I already made the changes to the files > before starting the process of making a diff file. How do I roll back those > changes so that I can make a diff file? You actually won't need to rollback your changes in order to create the diff file. Assuming you've made your changes inside of your clone of the repository, you should be able to run "hg diff" to show you what has changed. You can then use "hg commit" to save this changes as a commit, and then export the whole commit to a patch file using "hg export", or upload it to bugzilla using bzexport (which should have been installed by ./mach mercurial-steup). It is probably worth familiarizing yourself with mercurial. The mercurial project's website[1] is a great resource for learning the basics. [1] http://mercurial.selenic.com/guide
Flags: needinfo?(smacleod)
This is my first time making a diff file. I have pulled changes since I made my own changes to the code. When I run the "hg diff" command, the terminal spits out a bunch of code I don't recognize. As I understand, I am supposed to just see the changes I made. Is that because I pulled changes since I made my own changes?
Flags: needinfo?(smacleod)
(In reply to cbazoian12 from comment #15) > This is my first time making a diff file. I have pulled changes since I made > my own changes to the code. When I run the "hg diff" command, the terminal > spits out a bunch of code I don't recognize. As I understand, I am supposed > to just see the changes I made. Is that because I pulled changes since I > made my own changes? hg diff should only be displaying the changes to your working directory and wouldn't include changes that have been pulled from the server (they will be present in commits) For example, you were based on commit 'A' from the server > ... <- A <- Your work After pulling the result would be > ... <- A <- Your work > \ > \- Commits pulled <- New default head from server If you run 'hg diff' without arguments and you are seeing the changes from the commits that came from the server something strange has happened. So a few questions for you: - How exactly did you pull the new changes? - Has your work been committed or is it still just part of the working directory? In the case where you pull to get new changes and things work as expected, you'd then want to "rebase" your work onto the new changes. This is done with the rebase extension[1] It would be really helpful if we could chat synchronously so I can help you through these mercurial issues more quickly. When you have some time could you join the mozilla irc server[2] and message me (I'm smacleod on irc)? [1] http://mercurial.selenic.com/wiki/RebaseExtension [2] https://wiki.mozilla.org/IRC
Flags: needinfo?(smacleod) → needinfo?(cbazoian12)
Thank you. I am working on the patch.
Flags: needinfo?(cbazoian12)
Attached patch bug1132556.diff (obsolete) — Splinter Review
I hope this patch turned out right this time.
Attachment #8569437 - Attachment is obsolete: true
Flags: needinfo?(smacleod)
(In reply to cbazoian12 from comment #18) > Created attachment 8577444 [details] [diff] [review] > bug1132556.diff > > I hope this patch turned out right this time. Hey sorry for the delay in the response, I had written a reply but I guess I didn't submit it. Unfortunately it looks like you have many modifications to unrelated files included in the patch. Something you can do before submitting is to open up the diff file in a text editor and check that it contains what you expect. I'm unsure how exactly the unrelated changes were included into your working directory and then committed. We'll need to create a new commit though which doesn't include the unrelated changes. It would be really helpful if you could contact me on irc and we could work through this together synchronously.
Flags: needinfo?(smacleod)
Has this bug been fixed ?
(In reply to faris18omar from comment #20) > Has this bug been fixed ? It has not.
Assignee: cbazoian12 → nobody
Status: ASSIGNED → NEW
Steven, since this bug appears to be unclaimed would you mind assigning it to me? I took a look to check it out and believe I have the necessary changes made. Don't want to submit a patch out of turn though!
AJ: Go for it.
Assignee: nobody → ajkerrigan
(In reply to AJ Kerrigan from comment #22) > Steven, since this bug appears to be unclaimed would you mind assigning it > to me? I took a look to check it out and believe I have the necessary > changes made. Don't want to submit a patch out of turn though! As :mhoye indicated, please submit your patch, it would be most welcome :)
This is my first attempt at submitting a patch, so please don't be shy if I'm doing something messed up :). As supporting documentation, here is the before/after mochitest output for the sessionrestore component. Before: Browser Chrome Test Summary Passed: 1029 Failed: 0 Todo: 0 *** End BrowserChrome Test Results *** SUITE-END | took 207s After: Browser Chrome Test Summary Passed: 1029 Failed: 0 Todo: 0 *** End BrowserChrome Test Results *** SUITE-END | took 205s
Comment on attachment 8705277 [details] [diff] [review] Replace runInContent() with ContentTask.spawn() In the future when posting patches you'll want to set a review? flag on who you would like to review the patch. When attempting to do that their is a suggested reviewers feature in bugzilla that can provide options for you if you don't know who should review the patch (if it picks the wrong person they'll be able to help decide on someone better). I'm going to set the flag on myself and I should have a review for you sometime soon. Thanks for the patch! :)
Attachment #8705277 - Flags: review?(smacleod)
Apologies for the double-patch. I initially missed that the "ss-test:run" message was only coming from the now-removed runInContent(). To remove runInContent() more cleanly, I've taken out the "ss-test:run" listener. Test results are unchanged.
Attachment #8705277 - Attachment is obsolete: true
Attachment #8705277 - Flags: review?(smacleod)
(In reply to Steven MacLeod [:smacleod] from comment #27) > Comment on attachment 8705277 [details] [diff] [review] > Replace runInContent() with ContentTask.spawn() > > In the future when posting patches you'll want to set a review? flag on who > you would like to review the patch. When attempting to do that their is a > suggested reviewers feature in bugzilla that can provide options for you if > you don't know who should review the patch (if it picks the wrong person > they'll be able to help decide on someone better). > > I'm going to set the flag on myself and I should have a review for you > sometime soon. Thanks for the patch! :) Oh thanks for the advice! I'll keep that in mind going forward.
Attachment #8705288 - Flags: review?(smacleod)
Comment on attachment 8705288 [details] [diff] [review] Replace runInContent() with ContentTask.spawn() Review of attachment 8705288 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, nice first patch :D I've pushed this to try, which will run the tests and show the results here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c5b469bdd43 If all looks good we should be able to land this. You can read about the try server if you'd like: https://wiki.mozilla.org/ReleaseEngineering/TryServer
Attachment #8705288 - Flags: review?(smacleod) → review+
(In reply to Steven MacLeod [:smacleod] from comment #30) > Comment on attachment 8705288 [details] [diff] [review] > Replace runInContent() with ContentTask.spawn() > > Review of attachment 8705288 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks great, nice first patch :D > > I've pushed this to try, which will run the tests and show the results here: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c5b469bdd43 > > If all looks good we should be able to land this. You can read about the try > server if you'd like: https://wiki.mozilla.org/ReleaseEngineering/TryServer Nice! Thanks for the review, guidance and links.
The try run looks good, marking checkin-needed so someone will land this. Thanks!
Keywords: checkin-needed
Attachment #8577444 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Wow, that was fast. AJ, we don't see a lot of easy slam dunks like this from new contributors, and it's a joy to watch this bug happen. Do you have something you'd like to work on next? We have a few of this grade that could use your attention, but we'd be pretty happy to work with you on something a few rungs further up the challenge ladder if you're so inclined.
(In reply to Mike Hoye [:mhoye] from comment #35) > Wow, that was fast. > > AJ, we don't see a lot of easy slam dunks like this from new contributors, > and it's a joy to watch this bug happen. Do you have something you'd like to > work on next? We have a few of this grade that could use your attention, but > we'd be pretty happy to work with you on something a few rungs further up > the challenge ladder if you're so inclined. Thanks Mike, very kind of you to say and I'd love to work with one of you. I'm happy to try something a bit more challenging, though I don't want to overreach and end up wasting people's time either. Would it be useful to chat a bit either via email or IRC (ajkerrigan)?
Flags: needinfo?(mhoye)
I'll email you directly!
Flags: needinfo?(mhoye)
[bugday-20160323] Status: RESOLVED,FIXED -> VERIFIED Comments: STR: Developer specific testing. Component: Name Firefox Version 46.0b4 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: