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)

Reporter

Description

5 years ago
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.
Reporter

Updated

5 years ago

Comment 1

5 years ago
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
Reporter

Updated

5 years ago
Blocks: 1090880
Reporter

Comment 2

5 years ago
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]
Reporter

Comment 3

5 years ago
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]

Comment 7

4 years ago
Hi

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

Thanks in advance
shreyas
Reporter

Comment 8

4 years ago
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).
Assignee

Comment 9

4 years ago
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)
Assignee

Comment 11

4 years ago
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?
Reporter

Comment 12

4 years ago
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.
Reporter

Comment 13

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

Comment 14

4 years ago
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?
Assignee

Comment 15

4 years ago
Should I go ahead with comment #14?
Reporter

Comment 16

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

Comment 17

4 years ago
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+
Assignee

Comment 19

4 years ago
Posted patch updated patch (obsolete) — Splinter Review
Reporter

Comment 20

4 years ago
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)
Reporter

Comment 21

4 years ago
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
Assignee

Comment 22

4 years ago
Posted patch updated patch (obsolete) — Splinter Review
Assignee

Comment 23

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

Comment 24

4 years ago
Posted patch updated patchSplinter Review
Assignee

Comment 25

4 years ago
Hi,

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

Updated

4 years ago
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)
Assignee

Comment 27

4 years ago
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+
Reporter

Comment 29

4 years ago
(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
Reporter

Updated

4 years ago
Blocks: 1094832
Sushrut, have you had a chance to review comment #28 and comment #29?
Assignee: nobody → developer
Status: NEW → ASSIGNED
Flags: needinfo?(developer)
Assignee

Comment 31

4 years ago
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)
Assignee

Comment 32

4 years ago
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
Assignee

Updated

4 years ago
Flags: needinfo?(jaws)

Comment 33

4 years ago
(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)
Assignee

Comment 35

4 years ago
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
Last Resolved: 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.

Updated

4 years ago
Depends on: 1185294

Updated

3 years ago
Blocks: 1278074
You need to log in before you can comment on or make changes to this bug.