ViewerRegistry should guard against runtime errors in entry filters

RESOLVED FIXED

Status

Other Applications
DOM Inspector
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: crussell, Assigned: crussell)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 450154 [details] [diff] [review]
ViewerRegistry.js cleanup

Right now, in ViewerRegistry.js, we guard against syntax errors in the filters, but not against errors that occur while executing those filters. Given the review process, which kind errors are most likely to sneak through?
Attachment #450154 - Flags: review?(sdwilsh)
Created attachment 450155 [details] [diff] [review]
Wrap filter execution in a try ... catch and report any errors
Assignee: nobody → Sevenspade
Status: NEW → ASSIGNED
Attachment #450155 - Flags: review?(sdwilsh)
Created attachment 451751 [details] [diff] [review]
ViewerRegistry.js cleanup mkII--fixed some mistakes in first attachment
Attachment #450154 - Attachment is obsolete: true
Attachment #451751 - Flags: review?(sdwilsh)
Attachment #450154 - Flags: review?(sdwilsh)
Created attachment 451753 [details] [diff] [review]
Wrap filter execution in a try ... catch and report any errors mkII--work off cleanup mkII and move return out of catch
Attachment #450155 - Attachment is obsolete: true
Attachment #451753 - Flags: review?(sdwilsh)
Attachment #450155 - Flags: review?(sdwilsh)
Attachment #451753 - Attachment description: Wrap filter execution in a try ... catch and report any errors mkII--work off cleanup mkII and return value out of catch → Wrap filter execution in a try ... catch and report any errors mkII--work off cleanup mkII and move return out of catch
Comment on attachment 451751 [details] [diff] [review]
ViewerRegistry.js cleanup mkII--fixed some mistakes in first attachment

>+  /**
>+   * Returns the absolute url where the xul file for a viewer can be found.
>+   *
>+   * @param aIndex
>+   *        The numerical index of the entry representing the viewer.
>+   * @return
>+   *        A string of the fully canonized url.
er, is that how we've been dong @return?  I thought we kept that on the same line...

r=sdwilsh
Attachment #451751 - Flags: review?(sdwilsh) → review+
Comment on attachment 451753 [details] [diff] [review]
Wrap filter execution in a try ... catch and report any errors mkII--work off cleanup mkII and move return out of catch

r=sdwilsh
Attachment #451753 - Flags: review?(sdwilsh) → review+
(In reply to comment #4)
> (From update of attachment 451751 [details] [diff] [review])
> >+  /**
> >+   * Returns the absolute url where the xul file for a viewer can be found.
> >+   *
> >+   * @param aIndex
> >+   *        The numerical index of the entry representing the viewer.
> >+   * @return
> >+   *        A string of the fully canonized url.
> er, is that how we've been dong @return?  I thought we kept that on the same
> line...

Well, I've written dom-inspector stuff like that plenty of times, but I'm not sure if any of those have gotten submitted for review/checked in yet. It looks like attachment 445241 [details] [diff] [review] for bug 179621 did that. It makes the description line up with the params' descriptions, which can't happen if the "@return" and its description are on the same line. (Notice that "return" is one char longer than "param".)
Attachment #451753 - Flags: superreview?(neil)
(In reply to comment #6)
> Well, I've written dom-inspector stuff like that plenty of times, but I'm not
> sure if any of those have gotten submitted for review/checked in yet. It looks
> like attachment 445241 [details] [diff] [review] for bug 179621 did that. It makes the description line
> up with the params' descriptions, which can't happen if the "@return" and its
> description are on the same line. (Notice that "return" is one char longer than
> "param".)
Hrm, I must have missed that.  Not sure that that is the style anywhere else in mozilla trees.
All right, I'll change it. Is it all right if I change the aforementioned line changed in attachment 445241 [details] [diff] [review], too?
OS: Linux → All
Hardware: x86 → All

Updated

8 years ago
Attachment #451753 - Flags: superreview?(neil) → superreview+
(In reply to comment #8)
> All right, I'll change it. Is it all right if I change the aforementioned line
> changed in attachment 445241 [details] [diff] [review], too?
yes
Pushed with those changes:
http://hg.mozilla.org/dom-inspector/rev/535e5fbda65c (cleanup)
http://hg.mozilla.org/dom-inspector/rev/03cf7e93828f (real changes)
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.