527444 - Stop using tail_ files in xpcshell: use do_register_cleanup() instead, in Core and Toolkit

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

[
/modules/libjar/zipwriter/test/unit/tail_zipwriter.js
/uriloader/exthandler/tests/unit/tail_handlerService.js

/toolkit/components/contentprefs/tests/unit/tail_contentPrefs.js
/toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js
/toolkit/mozapps/extensions/test/unit/tail_extensionmanager.js
]

All these are (near) one-liners.
So it should be trivial to move this call to the corresponding head files.

Comment 1

[Martin Cerdeira (mRt)]

(In reply to comment #0)
> [
> /modules/libjar/zipwriter/test/unit/tail_zipwriter.js
> /uriloader/exthandler/tests/unit/tail_handlerService.js
> 
> /toolkit/components/contentprefs/tests/unit/tail_contentPrefs.js
> /toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js
> /toolkit/mozapps/extensions/test/unit/tail_extensionmanager.js
> ]
> 
> All these are (near) one-liners.
> So it should be trivial to move this call to the corresponding head files.

Do you mean, for instance, move the contents of:

/toolkit/components/contentprefs/tests/unit/tail_contentPrefs.js

to

/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js

?

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> Do you mean, for instance, move the contents of:
> /toolkit/components/contentprefs/tests/unit/tail_contentPrefs.js
> to
> /toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
> ?

Yes.
In addition, the moved code should be put in a function, which should be do_register_cleanup()'ed.

Comment 3

[Martin Cerdeira (mRt)]

Attachment: zipwriter diff file

Comment 4

[Martin Cerdeira (mRt)]

/toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js
contains this code:
cleanUp();

And, /toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
includes a call to cleanUp() at the bottom (line 307) 

This need a refactor too?

Comment 5

[Martin Cerdeira (mRt)]

Attachment: Multiple files diff

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 499950 [details] [diff] [review]
Multiple files diff

>+function do_register_cleanup(){

You probably rather need:

+function do_register_cleanup() {
+  function () {

>+    try {
>+      zipW.close();
>+    }
>+    catch (e) {
>+      // Just ignore a failure here and attempt to delete the file anyway.
>+    }
>+
>+    if (tmpFile.exists())
>+      tmpFile.remove(true);    

+  }

>+}

And you need to remove the code from where it currently is too.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> /toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js
> 
> This need a refactor too?

Yes, one call is executed before the test, the other one after the test.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> You probably rather need:
> 
> +function do_register_cleanup() {
> +  function () {

+do_register_cleanup(
+  function () {

> >+    try {
> >+      zipW.close();
> >+    }
> >+    catch (e) {
> >+      // Just ignore a failure here and attempt to delete the file anyway.
> >+    }
> >+
> >+    if (tmpFile.exists())
> >+      tmpFile.remove(true);    
> 
> +  }
> 
> >+}

+);

actually ;->

Comment 9

[Serge Gautherie (:sgautherie)]

Martin, ping for update.

Comment 10

[Josh Matthews [:jdm]]

Martin, could you confirm that you're still working on this bug?

Comment 11

[Martin Cerdeira (mRt)]

(In reply to Josh Matthews [:jdm] from comment #10)
> Martin, could you confirm that you're still working on this bug?

Josh, Im not working on this.

Comment 12

[Rahid Hasan [:recursive]]

Hi, can I work on this bug?

Comment 13

[Rahid Hasan [:recursive]]

Attachment: stop_using_tail_files.patch (v1)

Comment 14

[Rahid Hasan [:recursive]]

Attachment: stop_using_tail_files.patch (v2)

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 8360833 [details] [diff] [review]
stop_using_tail_files.patch (v2)

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

This looks really good Rahid! Sorry about the long delay. I have two concerns - have you the download manager tests and the app tests? Those are the changes that look slightly more concerning to me, and I want to ensure that the tests still pass correctly.

::: modules/libjar/zipwriter/test/unit/head_zipwriter.js
@@ +45,5 @@
>  
>  var zipW = new ZipWriter();
> +
> +do_register_cleanup(
> +	function () {

Move this onto the previous line.

@@ +55,5 @@
> +		}
> +
> +		if (tmpFile.exists())
> +		  tmpFile.remove(true);
> +	}

This is using tabs; please use spaces instead.

::: modules/libjar/zipwriter/test/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_zipwriter.js
> +tail = 

Remove the trailing space.

::: toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
@@ +164,5 @@
>    prefBranch.setBoolPref("browser.preferences.content.log", true);
>  }
>  
> +do_register_cleanup(
> +  function () {

Previous line.

::: toolkit/components/contentprefs/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_contentPrefs.js
> +tail = 

Trailing space.

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +232,5 @@
>    return false;
> +}
> +
> +do_register_cleanup(
> +  function test_common_terminate()

Previous line.

@@ +233,5 @@
> +}
> +
> +do_register_cleanup(
> +  function test_common_terminate()
> +  {

Move this on the same line as the function name.

::: toolkit/components/downloads/test/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_download_manager.js
> +tail = 

Trailing space.

::: toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
@@ +359,5 @@
>  };
>  
>  cleanUp();
> +
> +do_register_cleanup(

This can just be |do_register_cleanup(cleanUp);|

::: toolkit/components/url-classifier/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_urlclassifier.js
> +tail = 

Trailing space.

::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +123,5 @@
>    Services.dirsvc.QueryInterface(Ci.nsIDirectoryService).registerProvider(provider);
>  }
>  
> +do_register_cleanup(
> +  function () {

Previous line.

::: toolkit/devtools/apps/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_apps.js
> +tail = 

Trailing space.

::: toolkit/identity/tests/unit/head_identity.js
@@ +212,5 @@
>  // Switch debug messages on by default
>  Services.prefs.setBoolPref("toolkit.identity.debug", true);
> +
> +do_register_cleanup(
> +  function () {

Previous line.

::: toolkit/identity/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_identity.js
> +tail = 

Trailing space.

::: uriloader/exthandler/tests/unit/head_handlerService.js
@@ +162,5 @@
>  
>  HandlerServiceTest.init();
> +
> +do_register_cleanup(
> +  function () {

Previous line.

::: uriloader/exthandler/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_handlerService.js
> +tail = 

Trailing space.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 8360833 [details] [diff] [review]
stop_using_tail_files.patch (v2)

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

(Sorry for the 2,5 month delay.)
Patch looks mainly good :-)

::: modules/libjar/zipwriter/test/unit/head_zipwriter.js
@@ +45,5 @@
>  
>  var zipW = new ZipWriter();
> +
> +do_register_cleanup(
> +	function () {

In addition to comment 15, my preference would be to name the added function(s), like "function tail_zipwriter()", or any better name you can think of.

::: modules/libjar/zipwriter/test/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_zipwriter.js
> +tail = 

Guenuine question:
Is it even needed/wanted to keep an empty "tail ="? Or could it be removed?

Comment 17

[Rahid Hasan [:recursive]]

Attachment: stop_using_tail_files.patch (v3)

This is updated patch based on previous review.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 8416058 [details] [diff] [review]
stop_using_tail_files.patch (v3)

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

In this patch, you missed to remove the tail files themselves: see your previous patch :-(
I found some (style) consistency nits...

::: modules/libjar/zipwriter/test/unit/head_zipwriter.js
@@ +42,5 @@
>  tmpFile.append("zipwriter-test.zip");
>  if (tmpFile.exists())
>    tmpFile.remove(true);
>  
>  var zipW = new ZipWriter();

Add a blank line before the added "do_register_cleanup(...". (Here and elsewhere.))

@@ +43,5 @@
>  if (tmpFile.exists())
>    tmpFile.remove(true);
>  
>  var zipW = new ZipWriter();
> +do_register_cleanup(function tail_zipwriter () {

No space before "()". (Here and elsewhere.)

@@ +44,5 @@
>    tmpFile.remove(true);
>  
>  var zipW = new ZipWriter();
> +do_register_cleanup(function tail_zipwriter () {
> +    try {

2-space indentations are enough. (Here and elsewhere.)
NB: I might not have minded the initial 4-space, but 2-space there too seems more consistent with (some) other files...

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +230,5 @@
>      return true;
>    }
>    return false;
> +}
> +do_register_cleanup(function test_common_terminate() {

"tail_download_manager()", to be consistent?

::: toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
@@ +361,5 @@
>      return (val >>> (32 - bits));
>    },
>  };
>  
> +do_register_cleanup(cleanUp);

No, this should be:

cleanUp(); // The existing one.

do_register_cleanup(cleanUp); // The one you move.

::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +119,5 @@
>    Services.dirsvc.QueryInterface(Ci.nsIDirectoryService).registerProvider(provider);
>  }
> +do_register_cleanup(function tail_apps () {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test

While here, add a ".". (Maybe elsewhere too.)

@@ +120,5 @@
>  }
> +do_register_cleanup(function tail_apps () {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test
> +    gClient.close(function () {

(Add a dumb function name here too.)

::: toolkit/identity/tests/unit/head_identity.js
@@ +246,5 @@
>  } catch(noPref) {}
>  Services.prefs.setBoolPref("identity.fxaccounts.enabled", true);
>  
>  // after execution, restore prefs
>  do_register_cleanup(function() {

"tail_identity()", to be consistent?

Comment 19

[Mike Hoye [:mhoye]]

Hi, Rahid - Are you still working on this?

Comment 20

[Akshendra Pratap Singh]

Can I work on this?

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to Mike Hoye [:mhoye] from comment #19)
> Hi, Rahid - Are you still working on this?

(No answer for 5 months.)


(In reply to Akshendra Pratap Singh from comment #20)
> Can I work on this?

Sure.

Comment 22

[Akshendra Pratap Singh]

Attachment: 527444_StopUsingTailFiles.patch (v1)

Comment 23

[Akshendra Pratap Singh]

review ping

Comment 24

[Josh Matthews [:jdm]]

Comment on attachment 8515565 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v1)

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

I'll look at this.

Comment 25

[Josh Matthews [:jdm]]

Comment on attachment 8515565 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v1)

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

This looks fine to me. I'm going to throw it at try and see if anything breaks!

Comment 26

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=5195390ecf75

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 8515565 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v1)

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

(Sorry for the one month delay.)

Good patch :-)
A few nits below.
Also, try and fix "\ No newline at end of file" in the modified files.

::: toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
@@ +368,5 @@
>  
>  cleanUp();
> +
> +do_register_cleanup(function tail_ulrclassifier() {
> +  cleanUp();

(In reply to Josh Matthews [:jdm] from comment #15)
> This can just be |do_register_cleanup(cleanUp);|

Agreed.
Yet, fwiw, my preference would be to have this as in this patch, for "clarity" (as in not to confuse the 2 cleanUp() calls).

::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +130,5 @@
>  }
>  
> +do_register_cleanup(function tail_apps() {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> While here, add a ".". (Maybe elsewhere too.)

(At the end of the sentence.)

@@ +131,5 @@
>  
> +do_register_cleanup(function tail_apps() {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test
> +    gClient.close(function () {

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> (Add a dumb function name here too.)

(Like "tail_apps_close" for example.)

Comment 28

[Akshendra Pratap Singh]

Attachment: 527444_StopUsingTailFiles.patch (v2)

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 8530879 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v2)

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

Good, though still a few nits.

::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +128,5 @@
>    };
>    Services.dirsvc.QueryInterface(Ci.nsIDirectoryService).registerProvider(provider);
>  }
>  
> +do_register_cleanup(function tail_apps_close() {

No, 'tail_apps' was fine here.

@@ +131,5 @@
>  
> +do_register_cleanup(function tail_apps_close() {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test.
> +    gClient.close(function () {

Add 'tail_apps_close' here.

::: toolkit/devtools/apps/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_apps.js
> +tail =

Why are you readding this "empty" line?

::: toolkit/identity/tests/unit/head_identity.js
@@ +253,5 @@
>    Services.prefs.setBoolPref("identity.fxaccounts.enabled", initialPrefFXAValue);
>  });
>  
> +do_register_cleanup(function tail_identity() {
> +  // pre-emptively shut down to clear resources.

Nit: "Pre-emptively", while there.

Comment 30

[Akshendra Pratap Singh]

Attachment: 527444_StopUsingTailFiles.patch (v3)

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 8531217 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v3)

Looks good :-)

>Bug 527444: Stopped using tail_files in Core and Toolkit; r=sgautherie

Nit: more like
Bug 527444: use "do_register_cleanup(...)" calls instead of "tail_*.js" files in xpcshell, in Core and Toolkit; f=sgautherie r=josh.

>--- a/toolkit/components/downloads/test/unit/tail_download_manager.js
>+++ /dev/null
>@@ -1,24 +0,0 @@
>-/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>-/* vim: set ts=2 et sw=2 tw=80: */
>-/* This Source Code Form is subject to the terms of the Mozilla Public
>- * License, v. 2.0. If a copy of the MPL was not distributed with this
>- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>-
>-/**
>- * Provides infrastructure for automated download components tests.
>- */
>-
>-"use strict";
>-
>-////////////////////////////////////////////////////////////////////////////////
>-//// Termination functions common to all tests

josh, can you (double) check whether any of these lines should be copied to the head file?
(Especially the "use strict" line.)

Comment 32

[Josh Matthews [:jdm]]

Comment on attachment 8531217 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v3)

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

The head file doesn't contain `use strict`, but I don't think it's worth introducing at this point.

Comment 33

[Akshendra Pratap Singh]

Attachment: 527444_StopUsingTailFiles.patch (v4)

Comment 34

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=6d99aaa360fa

Comment 35

[Josh Matthews [:jdm]]

Try run was green. Let's merge this!

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96a5b2854978

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4c12f753a004 for android xpcshell bustage: 

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4598518&repo=mozilla-inbound

Comment 38

[Serge Gautherie (:sgautherie)]

(In reply to Wes Kocher (:KWierso) from comment #37)
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=4598518&repo=mozilla-inbound

{
12. '/tools/buildbot/bin/python scripts/scripts/android_emulator_unittest.py ...' warnings 41m 18s 24ms
2901 12:31:45 INFO - KeyError: 'tail'
2910 12:31:45 INFO - KeyError: 'tail'
2911 12:31:45 ERROR - No tests run or test summary not found
}

Is this because remotexpcshelltests.py would not support having no tail line in the manifest?
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/remotexpcshelltests.py#81
99         return (list(sanitize_list(test['head'], 'head')),
100                 list(sanitize_list(test['tail'], 'tail')))

Unlike runxpcshelltests.py:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#379
401         headlist = test_object['head'] if 'head' in test_object else ''
402         taillist = test_object['tail'] if 'tail' in test_object else ''
403         return (list(sanitize_list(headlist, 'head')),
404                 list(sanitize_list(taillist, 'tail')))

If so, we could leave dumb "tail=" lines, while the harness issue is fixed in a separate bug.

Comment 39

[:Paolo Amadini]

Comment on attachment 8535621 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v4)

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

Hi, thanks for the work to help keeping our code base in good shape!

I've noticed an issue with the patch for the old "downloads" folder, that I described below.

Note that tests in this folder may be removed in the next few months, or maybe earlier, but in the meantime we should make sure we do the right thing.

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +232,5 @@
>    return false;
>  }
> +
> +do_register_cleanup(function tail_download_manager() {
> +  add_task(function test_common_terminate() {

"add_task" has no effect during cleanup functions - in fact, the existing code was broken, as "add_task" does not work properly inside "tail.js" files as well.

The cleanup function, instead, should just return the promise. This ensures that we wait until the HTTP server has shut down before continuing.

Comment 40

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98467cc30a33

Comment 41

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61cea12f5829

Comment 42

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9d73facd881

Comment 43

[Josh Matthews [:jdm]]

Attachment: use "do_register_cleanup(...)" calls instead of "tail_*.js" files in xpcshell, in Core and Toolkit; f=sgautherie

This addresses your previous feedback on the patch.

Comment 44

[:Paolo Amadini]

Comment on attachment 8704833 [details] [diff] [review]
use "do_register_cleanup(...)" calls instead of "tail_*.js" files in xpcshell, in Core and Toolkit; f=sgautherie

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

Looks good to me with the changes below assuming a tryserver build succeeds. Thanks!

::: devtools/shared/apps/tests/unit/head_apps.js
@@ +134,5 @@
> +do_register_cleanup(function tail_apps() {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test.
> +    gClient.close(function tail_apps_close() {
> +      run_next_test();

Again, it seems like you want to return a Promise here. I'm not even sure that "run_next_test" did what it was supposed to in the tail file.

Note that if you use both do_register_cleanup and do_get_profile, the relative order of those calls is important for network-related operations. See <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Environment> that I recently updated.

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +177,5 @@
> +    // Stop the HTTP server.  We must do this inside a task in "tail.js" until the
> +    // xpcshell testing framework supports asynchronous termination functions.
> +    let deferred = Promise.defer();
> +    gHttpServer.stop(deferred.resolve);
> +    return deferred.promise;

Bonus points for using the "new Promise()" syntax, despite we're removing this code completely at some point :-)

::: uriloader/exthandler/tests/unit/head_handlerService.js
@@ +161,5 @@
>  };
>  
>  HandlerServiceTest.init();
> +
> +do_register_cleanup(function tail_handleService() {

nit: tail_handlerService

Comment 45

[Josh Matthews [:jdm]]

(In reply to :Paolo Amadini from comment #44)
> ::: devtools/shared/apps/tests/unit/head_apps.js
> @@ +134,5 @@
> > +do_register_cleanup(function tail_apps() {
> > +  if (gClient) {
> > +    // Close the test remote connection before leaving this test.
> > +    gClient.close(function tail_apps_close() {
> > +      run_next_test();
> 
> Again, it seems like you want to return a Promise here. I'm not even sure
> that "run_next_test" did what it was supposed to in the tail file.
> 
> Note that if you use both do_register_cleanup and do_get_profile, the
> relative order of those calls is important for network-related operations.
> See
> <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-
> based_unit_tests#Environment> that I recently updated.

I originally attempted this change when updating this patch and encountered test failures.

Comment 46

[:Paolo Amadini]

(In reply to Josh Matthews [:jdm] from comment #45)
> I originally attempted this change when updating this patch and encountered
> test failures.

It may well be caused by the ordering issue, so it may be easy to fix. If it's more complicated and you want to handle this test only in a different bug, feel free to.

Comment 47

[Mark Banner (:standard8)]

Josh, I'd like to get this done as makes parts of bug 1311312 simpler. Would you be happy to update the patch, or can I take it from you?

Looks like there's now only four instances of tail files left:

https://dxr.mozilla.org/mozilla-central/search?q=file%3Atail_*.js&redirect=false

Comment 48

[Mark Banner (:standard8)]

Ok, maybe two more as well:

https://dxr.mozilla.org/mozilla-central/search?q=file%3Atail.js&redirect=false

Comment 49

[Josh Matthews [:jdm]]

Please do!

Comment 50

[Mark Banner (:standard8)]

Attachment: Bug 527444 - use do_register_cleanup calls instead of tail_*.js files in xpcshell, in Core and Toolkit. .

Patch originally by Rahid Hasan. Updated by Akshendra Pratap Singh, jdm and Standard8.

Review commit: https://reviewboard.mozilla.org/r/104198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104198/

Comment 51

[Mark Banner (:standard8)]

Comment on attachment 8704833 [details] [diff] [review]
use "do_register_cleanup(...)" calls instead of "tail_*.js" files in xpcshell, in Core and Toolkit; f=sgautherie

Ok, patch updated. Hopefully this will make it this time :-)

Comment 52

[:Paolo Amadini]

Comment on attachment 8826208 [details]
Bug 527444 - use do_register_cleanup calls instead of tail_*.js files in xpcshell, in Core and Toolkit. .

https://reviewboard.mozilla.org/r/104198/#review105648

Looks good to me. Nit: I've seen at least one xpcshell.ini without the "tail =" line, I believe it might be optional nowadays.

Comment 53

[Mark Banner (:standard8)]

Comment on attachment 8826208 [details]
Bug 527444 - use do_register_cleanup calls instead of tail_*.js files in xpcshell, in Core and Toolkit. .

https://reviewboard.mozilla.org/r/104198/#review105648

I'm going to remove the tail functionality in bug 503613, so I'll clean those up there.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/446a150332c3
use do_register_cleanup calls instead of tail_*.js files in xpcshell, in Core and Toolkit. r=Paolo.

Comment 55

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/446a150332c3