Closed Bug 394686 Opened 17 years ago Closed 9 years ago

Throw Component.exceptions instead of strings

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: Dolske, Assigned: shreyaslakhe, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

Gavin mentioned in bug 387617 that we should do this, as it would be more useful to external callers via XPCOM.
Product: Firefox → Toolkit
Mentor: dteller
Depends on: 1059706
Whiteboard: [lang=js][good first bug]
The objective of this bug is to fix many of the throwing statements of directory toolkit/components/passwordmgr (see http://dxr.mozilla.org/mozilla-central/search?q=throw+path%3Atoolkit%2Fcomponents%2Fpasswordmgr&case=true ).

Generally, in JavaScript, you can call `throw foo` to throw an error, but if `foo` is not an instance of `Error`, this complicates debugging a lot. So, we want to look at all the calls to `throw foo` in that directory and make sure that we always throw instances of `Error`.

- wherever there is `throw "Some text"`, replace it with `throw new Error("Some text")`;
- wherever there is `throw Components.results.SOME_CONSTANT` or equivalently `throw Cr.SOME_CONSTANT`, replace it with `throw new Components.Exception(some message, Components.results.SOME_CONSTANT)`.

Is this information sufficient to get you started, Rajat? If you have any question, don't hesitate to ask.
Flags: needinfo?(rajat503)
Is this open? Interested in taking it.
Flags: needinfo?(dteller)
Hi Jeffrey. Rajat mentioned a few days ago that he was interested in this bug, but I just filed bug 1061521 which is a very similar bug. Would you be interested in working on bug 1061521 instead?
Flags: needinfo?(dteller)
Yes I'm interested in working on bug 1061521. Can you please  assign that to me?
I haven't been able to clone mozilla-central due to its size. Any alternatives?
Flags: needinfo?(rajat503)
Hey! This is my first time fixing a bug. How do i start?
Mentor: dteller
Whiteboard: [lang=js][good first bug]
(In reply to Medhini from comment #6)
> Hey! This is my first time fixing a bug. How do i start?

See comment 1 and https://developer.mozilla.org/en-US/docs/Introduction
Flags: needinfo?(medhini95)
Mentor: MattN+bmo
Whiteboard: [lang=js][good first bug]
Hi

I would like to work on this bug. How do I proceed?

Thanks in advance
shreyas
Flags: needinfo?(MattN+bmo)
(In reply to shreyas from comment #8)
> I would like to work on this bug. How do I proceed?

Hello shreyas, please see comment 1 and https://developer.mozilla.org/en-US/docs/Introduction. You can also join #introduction on irc.mozilla.org to get live help getting a development environment setup.
Flags: needinfo?(medhini95)
Flags: needinfo?(MattN+bmo)
Attached patch bug394686.diff (obsolete) — Splinter Review
Attachment #8565825 - Flags: review?(MattN+bmo)
Assignee: nobody → shreyaslakhe
Comment on attachment 8565825 [details] [diff] [review]
bug394686.diff

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

Thanks for the patch. There are just some small things to fix then you can request review again.

Your commit message should follow the rules at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions (you can include r=MattN in advance to make it easier for checkin later).

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +74,4 @@
>        return this;
>      }
>  
> +    throw new Components.Exception("interface not found", Components.results.NS_ERROR_NO_INTERFACE);

Good catch that Cr wasn't defined :)

Can you add the alias for `Cr` to this file like so:

  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;

(replacing the existing Cc, Ci, Cu lines)

Also, I think "Interface not available" is a bit clearer.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +231,4 @@
>        this.__strBundle = bunService.createBundle(
>                    "chrome://passwordmgr/locale/passwordmgr.properties");
>        if (!this.__strBundle)
> +        throw new error("String bundle for Login Manager not present!");

This should be `Error` (uppercase e).

@@ +294,4 @@
>    prompt : function (aDialogTitle, aText, aPasswordRealm,
>                       aSavePassword, aDefaultText, aResult) {
>      if (aSavePassword != Ci.nsIAuthPrompt.SAVE_PASSWORD_NEVER)
> +      throw new Components.Exception("password not saaved", Components.results.NS_ERROR_NOT_IMPLEMENTED);

How about "prompt only supports SAVE_PASSWORD_NEVER"?

@@ +318,4 @@
>      this.log("===== promptUsernameAndPassword() called =====");
>  
>      if (aSavePassword == Ci.nsIAuthPrompt.SAVE_PASSWORD_FOR_SESSION)
> +        throw new Components.Exception("password saved", Components.results.NS_ERROR_NOT_IMPLEMENTED);

How about "promptUsernameAndPassword doesn't support SAVE_PASSWORD_FOR_SESSION"?

@@ +420,4 @@
>      this.log("===== promptPassword called() =====");
>  
>      if (aSavePassword == Ci.nsIAuthPrompt.SAVE_PASSWORD_FOR_SESSION)
> +        throw new Components.Exception("password saved", Components.results.NS_ERROR_NOT_IMPLEMENTED);

How about "promptPassword doesn't support SAVE_PASSWORD_FOR_SESSION"?

::: toolkit/components/passwordmgr/storage-mozStorage.js
@@ +66,4 @@
>        return this._dbConnection;
>      }
>  
> +    throw new Components.Exception("interface not found", Cr.NS_ERROR_NO_INTERFACE);

"Interface not available"
Attachment #8565825 - Flags: review?(MattN+bmo) → feedback+
Attached patch bug_394686.diffSplinter Review
Updated the patch
Attachment #8565825 - Attachment is obsolete: true
Attachment #8568339 - Flags: review?(MattN+bmo)
https://hg.mozilla.org/integration/fx-team/rev/4c55a705e629
Flags: in-testsuite-
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
Comment on attachment 8568339 [details] [diff] [review]
bug_394686.diff

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

I pushed this after fixing a few small issues and making the commit message more descriptive. Good job.

You can find some more mentored bugs at http://www.joshmatthews.net/bugsahoy/?internals=1&ff=1
Attachment #8568339 - Flags: review?(MattN+bmo) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/4c55a705e629
https://hg.mozilla.org/mozilla-central/rev/4e79d19ab9f2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.