989307 - Make FUEL warn deprecation to the console on first usage

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

We all know FUEL is deprecated, but there's nothing notifying consumers of that. We should start notifying that outside the team, with a nice deprecation warning.

Comment 1

[:Gijs (he/him)]

We should doublecheck that all the relevant MDN pages have this info, too.

Comment 2

[Marco Bonardo [:mak]]

We should use Deprecated.jsm, we can probably issue the deprecation warning in the Application() constructor in http://mxr.mozilla.org/mozilla-central/source/browser/fuel/fuelApplication.js since it's managed as a singleton that should be enough to avoid double warnings.

Gavin, is it ok to move on with this? Do you want AMO team to collect statistical data first?

Comment 3

[Marco Bonardo [:mak]]

hm the fact Application is preloaded in XUL scripts may cause us to warn too early, so maybe we'll need a different hack.

Comment 4

[Marco Bonardo [:mak]]

so looks like the Application global is lazy, so we can indeed use the constructor, provided we fix instances in browser that are mistakenly using FUEL.

For example
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3228
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-addons.js#173
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-addons.js#284

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Yes, sounds fine to proceed with this.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #4)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3228

http://hg.mozilla.org/mozilla-central/annotate/d92db71d4e67/browser/base/content/tabbrowser.xml#l3236

> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> addons.js#173

http://hg.mozilla.org/mozilla-central/annotate/d92db71d4e67/browser/base/content/browser-addons.js#l162

> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> addons.js#284

http://hg.mozilla.org/mozilla-central/annotate/d92db71d4e67/browser/base/content/browser-addons.js#l273

Comment 7

[shreyas]

Hi

I would like to work on this bug. How should I go about it?

Thanks in advance
shreyas

Comment 8

[Marco Bonardo [:mak]]

Hi, do you already have a working firefox build? https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

you can create a patch based on the User Story (at the top of this bug report).

If you need immediate help you can ask questions on irc.mozilla.org, #introduction channel, or set a needinfo on the mentor (jared filled up most of the info here, I'm happy if he wants to mentor this).

Comment 9

[Sushrut Girdhari (sg345)]

Hi, I'd like to work on this bug, I have a working Firefox build.
"There are four steps to this bug:" - does this mean that they are the steps for reproducing the bug behaviour on the mozilla system or are they the steps for fixing the bug?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Sushrut Girdhari (sg345) from comment #9)
> Hi, I'd like to work on this bug, I have a working Firefox build.
> "There are four steps to this bug:" - does this mean that they are the steps
> for reproducing the bug behaviour on the mozilla system or are they the
> steps for fixing the bug?

Those steps are for fixing the bug.

Comment 11

[Sushrut Girdhari (sg345)]

Hi,

For reducing duplication between (2) and (3), can I create a new JS function at the top:
 
function fuelDeprecatedWarning(){
let appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"].
                            getService(Components.interfaces.nsIAppStartup);
 
// If already in safe mode restart in safe mode (bug 327119)
if (Services.appinfo.inSafeMode) {
  appStartup.restartInSafeMode(Components.interfaces.nsIAppStartup.eAttemptQuit);
  return;
}
 
appStartup.quit(Components.interfaces.nsIAppStartup.eAttemptQuit |
                Components.interfaces.nsIAppStartup.eRestart);

return null;
}

and then I can place two calls to fuelDeprecatedWarning() in place of Application.restart()?
Is there a standard convention for naming the function? If so, what should it be named?

Comment 12

[Marco Bonardo [:mak]]

browser-addons.js is imported in browser.js, and by making a js function at the top level into it, you'd basically add it to the global object in browser.js.

I see 2 possibilities here:
1. add a goRestartApplication() to toolkit globalOverlay.js
2. Add a restart() method to BrowserUtils.jsm

The former solution seems to make sense since we already have a goQuitApplication, BUT this method doesn't depend on a document or a window, so it's instead the perfect fit for a module (I think also goQuitApplication is not in the best place, but it's there for compat reasons).

So in the end, I think the best solution is a BrowserUtils.restart() method.

Comment 13

[Marco Bonardo [:mak]]

or .restartApplication() if we want to be more specific (so there's not doubt we are not just "restarting" BrowserUtils)

Comment 14

[Sushrut Girdhari (sg345)]

Okay, just to confirm this is what I'm trying to do right now:

1. Add the following snippet of code in BrowserUtils.jsm

restartApplication: function() {

let appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"].
                            getService(Components.interfaces.nsIAppStartup);
 
// If already in safe mode restart in safe mode (bug 327119)
if (Services.appinfo.inSafeMode) {
  appStartup.restartInSafeMode(Components.interfaces.nsIAppStartup.eAttemptQuit);
  return;
}
 
appStartup.quit(Components.interfaces.nsIAppStartup.eAttemptQuit |
                Components.interfaces.nsIAppStartup.eRestart);

return null;
}


And then simple replace Application.restart() with BrowserUtils.restartApplication() in (2).

Is it right?

Comment 15

[Sushrut Girdhari (sg345)]

Should I go ahead with comment #14?

Comment 16

[Marco Bonardo [:mak]]

It should work yes. pay attention to the indentation, we'll look at remaining coding style issues later.

Comment 17

[Sushrut Girdhari (sg345)]

Attachment: Made changes as per the description.

Made changes as per the description given in the user story.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8569361 [details] [diff] [review]
Made changes as per the description.

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

These changes look good. Some simple style issues to clean up, and then this should be ready.

After you make the changes and re-upload, I'll push the patch to tryserver for you where it will run the patch against all of our automated tests on every platform that we support.

::: browser/base/content/tabbrowser.xml
@@ +3350,1 @@
>                                    "will be removed in a future release.");

nit, please line up this string with the one above it,
> Services.console.logStringMessage("enterTabbedMode is an obsolete method and " +
>                                   "will be removed in a future release.");

::: browser/fuel/fuelApplication.js
@@ -8,5 @@
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  const APPLICATION_CID = Components.ID("fe74cf80-aa2d-11db-abbd-0800200c9a66");
>  const APPLICATION_CONTRACTID = "@mozilla.org/fuel/application;1";
> -

This line can be added back.

::: toolkit/modules/BrowserUtils.jsm
@@ +24,5 @@
> +  
> +  /**
> +   *Bug 989307 - Make FUEL warn deprecation to the console on first usage. This
> +   *method is used to restart the application after displaying the warning
> +   */

The function below is pretty self explanatory so this comment is pretty redundant. We can use `hg annotate` to see which patch/bug was the source for a line of code.

Comment 19

[Sushrut Girdhari (sg345)]

Attachment: updated patch

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8569611 [details] [diff] [review]
updated patch

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

You can run the FUEL tests by using

./mach mochitest-browser browser/fuel/test

please ensure they pass.

::: browser/base/content/tabbrowser.xml
@@ +3345,5 @@
>  
>        <!-- Deprecated stuff, implemented for backwards compatibility. -->
>        <method name="enterTabbedMode">
>          <body>Application.console.log("enterTabbedMode is an obsolete method and " +
> +          "will be removed in a future release.");

You should still use Services.console here... not Application.
you should also properly indent here so the strings are at the same indentation level.

::: browser/fuel/fuelApplication.js
@@ +1,5 @@
>  /* 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/. */
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Deprecated", "resource://gre/modules/Deprecated.jsm");

we have an old habit to wrap files at 80 chars, this should be reindented as

XPCOMUtils.defineLazyModuleGetter(this, "Deprecated",
                                  "resource://gre/modules/Deprecated.jsm");

@@ +734,5 @@
>  
>  //=================================================
>  // Application constructor
>  function Application() {
> +  Deprecated.warning();

You must provide arguments to this call

I'd suggest something along the lines of

"FUEL is deprecated, you should use the add-on SDK instead.",
"https://developer.mozilla.org/Add-ons/SDK/"

::: toolkit/modules/BrowserUtils.jsm
@@ +20,5 @@
>      for (let a of args)
>        dump(a + " ");
>      dump("\n");
>    },
> +  restartApplication: function() {

this needs a javadoc.

Plus an empty newline above and below it to visually differentiate from the other methods, in the end it should look like this:
\n
/**
 * javadoc
 */
method() {
  ...
}
\n

@@ +21,5 @@
>        dump(a + " ");
>      dump("\n");
>    },
> +  restartApplication: function() {
> +    let appStartup=Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(Components.interfaces.nsIAppStartup);

add spaces around "="
use Cc instead of Components.classes and Ci instead of Components.interfaces
align dot with [ as
Cc[...]
  .getService(Ci...)

@@ +23,5 @@
>    },
> +  restartApplication: function() {
> +    let appStartup=Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(Components.interfaces.nsIAppStartup);
> +    if (Services.appinfo.inSafeMode) {
> +      appStartup.restartInSafeMode(Components.interfaces.nsIAppStartup.eAttemptQuit);

use Ci

@@ +26,5 @@
> +    if (Services.appinfo.inSafeMode) {
> +      appStartup.restartInSafeMode(Components.interfaces.nsIAppStartup.eAttemptQuit);
> +      return;
> +    }
> +    appStartup.quit(Components.interfaces.nsIAppStartup.eAttemptQuit | Components.interfaces.nsIAppStartup.eRestart);

use Ci

@@ +27,5 @@
> +      appStartup.restartInSafeMode(Components.interfaces.nsIAppStartup.eAttemptQuit);
> +      return;
> +    }
> +    appStartup.quit(Components.interfaces.nsIAppStartup.eAttemptQuit | Components.interfaces.nsIAppStartup.eRestart);
> +    return null;

I think this return null can be removed. this method returns a "void", so it should return nothing.

Comment 21

[Marco Bonardo [:mak]]

Jared, I'm leaving this mentorship to you, I have enough requests to follow atm :/
Regardless, I think I don't have further comments.

Comment 22

[Sushrut Girdhari (sg345)]

Attachment: updated patch

Comment 23

[Sushrut Girdhari (sg345)]

Hi, please ignore the previous patch just updated, I need to fix the indentation. I'll upload another patch soon.

Comment 24

[Sushrut Girdhari (sg345)]

Attachment: updated patch

Comment 25

[Sushrut Girdhari (sg345)]

Hi,

I have made the required changes. Please check the attachment from comment #24. Do let me know if any additional changes are required.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8569874 [details] [diff] [review]
updated patch

(when uploading a patch, you can flag me for review by setting the "review" flag to "?" and then putting in ":jaws" in the text box that appears)

Comment 27

[Sushrut Girdhari (sg345)]

I think the code seems to work now. Please review the code.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8569874 [details] [diff] [review]
updated patch

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

::: browser/fuel/fuelApplication.js
@@ +736,5 @@
>  //=================================================
>  // Application constructor
>  function Application() {
> +  Deprecated.warning("FUEL is deprecated, you should use the add-on SDK instead.");
> +  Deprecated.warning("https://developer.mozilla.org/Add-ons/SDK/");

This can be combined to one string so that there isn't two deprecated warning for this.

Deprecated.warning("FUEL is deprecated, you should use the add-on SDK instead. " +
                   "See https://developer.mozilla.org/Add-ons/SDK/");

::: toolkit/modules/BrowserUtils.jsm
@@ +22,5 @@
>      dump("\n");
>    },
>  
> +/**
> + * javadoc

The "javadoc" that comment #20 was referencing means to add some documentation here that includes any parameters that it accepts, what it does, and if it returns anything.

You can replace "javadoc" with:

* restartApplication: Restarts the application, keeping it in
* safe mode if it is already in safe mode.

@@ +32,5 @@
> +    if (Services.appinfo.inSafeMode) {
> +      appStartup.restartInSafeMode(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart);
> +    }
> +    appStartup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart);
> +    }

Did you build these changes and test them? I can't see how this would work. Please build your changes, test them, and see if you can fix this. You can ask for help with getting your build up and running in #introduction on irc.mozilla.org.

Comment 29

[Marco Bonardo [:mak]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28)
> > +  Deprecated.warning("FUEL is deprecated, you should use the add-on SDK instead.");
> > +  Deprecated.warning("https://developer.mozilla.org/Add-ons/SDK/");
> 
> This can be combined to one string so that there isn't two deprecated
> warning for this.

Deprecated.warning takes 2 arguments, a description and an href:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Deprecated.jsm?force=1#60

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Sushrut, have you had a chance to review comment #28 and comment #29?

Comment 31

[Sushrut Girdhari (sg345)]

Hi Jared,

Yes, I have reviewed the comments above and I'm working on the patch. I'll upload the updated patch after implementing the suggested changes.

Comment 32

[Sushrut Girdhari (sg345)]

Results of mochi test: The following tests failed-

178 INFO TEST-UNEXPECTED-FAIL | browser/fuel/test/browser_Browser.js | Exception thrown - [Exception... "[JavaScript Error: "this._tabbrowser is undefined" {file: "file:///home/firefox-dev/mozilla-central/objdir-ff-release/dist/bin/browser/components/fuelApplication.js" line: 106}]'[JavaScript Error: "this._tabbrowser is undefined" {file: "file:///home/firefox-dev/mozilla-central/objdir-ff-release/dist/bin/browser/components/fuelApplication.js" line: 106}]' when calling method: [fuelIApplication::windows]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://mochitests/content/browser/browser/fuel/test/browser_Browser.js :: test :: line 18"  data: yes]
TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - ReferenceError: gBrowser is not defined at chrome://mochikit/content/browser-test.js:523
SUITE-END | took 336s

Comment 33

[:Gijs (he/him)]

(In reply to Sushrut Girdhari (sg345) from comment #32)
> Results of mochi test: The following tests failed-
> 
> 178 INFO TEST-UNEXPECTED-FAIL | browser/fuel/test/browser_Browser.js |
> Exception thrown - [Exception... "[JavaScript Error: "this._tabbrowser is
> undefined" {file:
> "file:///home/firefox-dev/mozilla-central/objdir-ff-release/dist/bin/browser/
> components/fuelApplication.js" line: 106}]'[JavaScript Error:
> "this._tabbrowser is undefined" {file:
> "file:///home/firefox-dev/mozilla-central/objdir-ff-release/dist/bin/browser/
> components/fuelApplication.js" line: 106}]' when calling method:
> [fuelIApplication::windows]"  nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame ::
> chrome://mochitests/content/browser/browser/fuel/test/browser_Browser.js ::
> test :: line 18"  data: yes]
> TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception -
> ReferenceError: gBrowser is not defined at
> chrome://mochikit/content/browser-test.js:523
> SUITE-END | took 336s

Please upload the patch you're using that gives you this error. It looks like you've introduced a syntax error somewhere.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Hi Sushrut, any update here?

Comment 35

[Sushrut Girdhari (sg345)]

Hi,
I'm currently busy with college coursework and exams and wouldn't be able to work on this for the coming weeks. If anyone else would like to work on this bug feel free to re-assign it. 
Thanks!

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for getting back to me. I finished up your patch and pushed it to fx-team,
https://hg.mozilla.org/integration/fx-team/rev/bc45ac6b031e

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bc45ac6b031e

Comment 38

[Jean-Yves Perrier [:teoli]]

I've added a deprecation notice in FUEL pages on MDN.