Closed
Bug 1132556
Opened 10 years ago
Closed 9 years ago
Replace runInContent() with ContentTask.spawn()
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
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)
6.25 KB,
patch
|
smacleod
:
review+
|
Details | Diff | Splinter Review |
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+
Reporter | ||
Updated•10 years ago
|
User Story: (updated)
Reporter | ||
Updated•10 years ago
|
Flags: qe-verify-
Comment 2•10 years ago
|
||
I have my environment set up and I am working on this bug. Is that alright?
Reporter | ||
Comment 3•10 years ago
|
||
(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
Comment 4•10 years ago
|
||
Awesome. Thank you
Reporter | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
(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)
Reporter | ||
Comment 7•10 years ago
|
||
(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
Comment 8•10 years ago
|
||
Perfect. Thank you.
Comment 9•10 years ago
|
||
I am hoping I did this right. I attached the patch with the diff file in it.
Flags: needinfo?(smacleod)
Reporter | ||
Comment 10•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
(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)
Reporter | ||
Comment 12•10 years ago
|
||
(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.
Comment 13•10 years ago
|
||
(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?
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(smacleod)
Reporter | ||
Comment 14•10 years ago
|
||
(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)
Comment 15•10 years ago
|
||
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?
Updated•10 years ago
|
Flags: needinfo?(smacleod)
Reporter | ||
Comment 16•10 years ago
|
||
(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)
Comment 18•10 years ago
|
||
I hope this patch turned out right this time.
Attachment #8569437 -
Attachment is obsolete: true
Flags: needinfo?(smacleod)
Reporter | ||
Comment 19•10 years ago
|
||
(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)
Comment 20•9 years ago
|
||
Has this bug been fixed ?
Reporter | ||
Comment 21•9 years ago
|
||
(In reply to faris18omar from comment #20)
> Has this bug been fixed ?
It has not.
Assignee: cbazoian12 → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 22•9 years ago
|
||
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!
Reporter | ||
Comment 24•9 years ago
|
||
(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 :)
Assignee | ||
Comment 25•9 years ago
|
||
Assignee | ||
Comment 26•9 years ago
|
||
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
Reporter | ||
Comment 27•9 years ago
|
||
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)
Assignee | ||
Comment 28•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8705277 -
Attachment is obsolete: true
Attachment #8705277 -
Flags: review?(smacleod)
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8705288 -
Flags: review?(smacleod)
Reporter | ||
Comment 30•9 years ago
|
||
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+
Assignee | ||
Comment 31•9 years ago
|
||
(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.
Reporter | ||
Comment 32•9 years ago
|
||
The try run looks good, marking checkin-needed so someone will land this. Thanks!
Keywords: checkin-needed
Reporter | ||
Updated•9 years ago
|
Attachment #8577444 -
Attachment is obsolete: true
Comment 33•9 years ago
|
||
Keywords: checkin-needed
Comment 34•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 35•9 years ago
|
||
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.
Assignee | ||
Comment 36•9 years ago
|
||
(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)?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mhoye)
Comment 38•9 years ago
|
||
[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.
Description
•