Make FUEL warn deprecation to the console on first usage

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mak, Assigned: developer, Mentored)

Tracking

({addon-compat, dev-doc-complete})

unspecified
Firefox 40
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [good first bug][lang=js])

User Story

There are four steps to fixing this bug:

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

Change the call from Application.console.log to Services.console.logStringMessage

2) At http://hg.mozilla.org/mozilla-central/annotate/d92db71d4e67/browser/base/content/browser-addons.js#l162 and at http://hg.mozilla.org/mozilla-central/annotate/d92db71d4e67/browser/base/content/browser-addons.js#l273

Change the call from Application.restart() to the following:

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);

3) Find a way to reduce the duplication between (2) and (3)

4) Add a call to Deprecated.warning() from the FUEL Application constructor. You may need to import the Deprecated javascript module. To do that, you can add

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

to the top of the http://mxr.mozilla.org/mozilla-central/source/browser/fuel/fuelApplication.js file.

Attachments

(1 attachment, 3 obsolete attachments)

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.
We should doublecheck that all the relevant MDN pages have this info, too.
Keywords: dev-doc-needed
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=1
Points: --- → 1
Flags: qe-verify?
Whiteboard: p=1
Blocks: 1090880
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?
Mentor: mak77
Flags: needinfo?(gavin.sharp)
Whiteboard: [good first bug][lang=js]
hm the fact Application is preloaded in XUL scripts may cause us to warn too early, so maybe we'll need a different hack.
Yes, sounds fine to proceed with this.
Flags: needinfo?(gavin.sharp)
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][could use better steps for new contributor]
Mentor: jaws
User Story: (updated)
Whiteboard: [good first bug][lang=js][could use better steps for new contributor] → [good first bug][lang=js]
Hi

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

Thanks in advance
shreyas
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).
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?
(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.
User Story: (updated)
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?
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.
or .restartApplication() if we want to be more specific (so there's not doubt we are not just "restarting" BrowserUtils)
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?
Should I go ahead with comment #14?
It should work yes. pay attention to the indentation, we'll look at remaining coding style issues later.
Made changes as per the description given in the user story.
Flags: needinfo?(jaws)
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.
Attachment #8569361 - Flags: feedback+
Flags: needinfo?(jaws)
Posted patch updated patch (obsolete) — Splinter Review
Attachment #8569611 - Flags: review?(mak77)
Attachment #8569611 - Flags: review?(jaws)
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.
Attachment #8569611 - Flags: review?(mak77)
Attachment #8569611 - Flags: review?(jaws)
Jared, I'm leaving this mentorship to you, I have enough requests to follow atm :/
Regardless, I think I don't have further comments.
Mentor: mak77
Posted patch updated patch (obsolete) — Splinter Review
Hi, please ignore the previous patch just updated, I need to fix the indentation. I'll upload another patch soon.
Posted patch updated patchSplinter Review
Hi,

I have made the required changes. Please check the attachment from comment #24. Do let me know if any additional changes are required.
Flags: needinfo?(jaws)
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)
Flags: needinfo?(jaws)
Attachment #8569874 - Flags: review?(jaws)
Attachment #8569361 - Attachment is obsolete: true
Attachment #8569611 - Attachment is obsolete: true
Attachment #8569854 - Attachment is obsolete: true
I think the code seems to work now. Please review the code.
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.
Attachment #8569874 - Flags: review?(jaws) → feedback+
(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
Blocks: 1094832
Sushrut, have you had a chance to review comment #28 and comment #29?
Assignee: nobody → developer
Status: NEW → ASSIGNED
Flags: needinfo?(developer)
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.
Flags: needinfo?(developer)
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
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
Hi Sushrut, any update here?
Flags: needinfo?(developer)
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!
Flags: needinfo?(developer)
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
Flags: qe-verify? → qe-verify-
https://hg.mozilla.org/mozilla-central/rev/bc45ac6b031e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Iteration: --- → 40.1 - 13 Apr
Blocks: 1168663
I've added a deprecation notice in FUEL pages on MDN.
Depends on: 1185294
Blocks: 1278074
You need to log in before you can comment on or make changes to this bug.