Closed Bug 944073 Opened 6 years ago Closed 6 years ago

[Session Restore] Use Components.Exceptions instead of Components.returnCode

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Yoric, Assigned: lpy)

Details

(Whiteboard: [Async][mentor=Yoric][lang=js][qa-])

Attachments

(1 file, 3 obsolete files)

In a number of places of SesionStore.jsm, we use Components.returnCode to throw errors. That's rather ugly, we could do much nicer with Components.Exceptions.
I pick this bug.
Assignee: nobody → pylaurent1314
Attached patch bug944073.patch (obsolete) — Splinter Review
Attachment #8339902 - Flags: review?(dteller)
Comment on attachment 8339902 [details] [diff] [review]
bug944073.patch

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

It's the right idea. Could you apply the changes below?
I haven't reviewed the end of the patch, so I suspect that you'll have to make the same changes a few more times.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1443,5 @@
>      }
>      catch (ex) { /* invalid state object - don't restore anything */ }
>      if (!state || !state.windows)
> +      throw Components.Exception("Invalid state object: not JSON or no windows restored",
> +                                 Cr.NS_ERROR_INVALID_ARG);

Could you take the opportunity to split this in two?
i.e.
if (!state) {
  throw ...
}
if (!state.windows) {
  throw ...
}

@@ +1444,5 @@
>      catch (ex) { /* invalid state object - don't restore anything */ }
>      if (!state || !state.windows)
> +      throw Components.Exception("Invalid state object: not JSON or no windows restored",
> +                                 Cr.NS_ERROR_INVALID_ARG);
> +    

Nit: Please don't add whitespaces here.

@@ +1489,5 @@
>        let data = DyingWindowCache.get(aWindow);
>        return this._toJSONString({ windows: [data] });
>      }
>  
> +    throw Components.Exception("Invalid state string: window is not being tracked",

The problem is not in the state string, it's in the window, so just "Window is not tracked" should be sufficient.

@@ +1495,4 @@
>    },
>  
>    setWindowState: function ssi_setWindowState(aWindow, aState, aOverwrite) {
>      if (!aWindow.__SSi)

Could you take the opportunity to add braces for all the |if| statements that you're touching?
i.e.
  if (!aWindow.__SSi) {
    ...
  }

@@ +1495,5 @@
>    },
>  
>    setWindowState: function ssi_setWindowState(aWindow, aState, aOverwrite) {
>      if (!aWindow.__SSi)
> +      throw Components.Exception("Invalid state string: window is not being tracked",

The problem is not in the state string, it's in the window.

@@ +1497,5 @@
>    setWindowState: function ssi_setWindowState(aWindow, aState, aOverwrite) {
>      if (!aWindow.__SSi)
> +      throw Components.Exception("Invalid state string: window is not being tracked",
> +                                 Cr.NS_ERROR_INVALID_ARG);
> +    

Nit: whitespace

@@ +1504,5 @@
>  
>    getTabState: function ssi_getTabState(aTab) {
>      if (!aTab.ownerDocument || !aTab.ownerDocument.defaultView.__SSi)
> +      throw Components.Exception("Invalid tab object: no ownerDocument",
> +                                 Cr.NS_ERROR_INVALID_ARG);

Here, too, could you make it two distinct errors?

@@ +1505,5 @@
>    getTabState: function ssi_getTabState(aTab) {
>      if (!aTab.ownerDocument || !aTab.ownerDocument.defaultView.__SSi)
> +      throw Components.Exception("Invalid tab object: no ownerDocument",
> +                                 Cr.NS_ERROR_INVALID_ARG);
> +    

Nit: whitespace.

@@ +1518,5 @@
>      // as |aState| can be an incomplete state that will be completed
>      // by |restoreTabs|.
>      let tabState = JSON.parse(aState);
>      if (!tabState) {
>        debug("Empty state argument");

In this method, let's get rid of these calls to debug(), your exceptions are more useful.

@@ +1525,4 @@
>      }
>      if (typeof tabState != "object") {
>        debug("State argument does not represent an object");
> +      throw Components.Exception("Invalid state object: not object",

Nit: "not an object"

@@ +1557,5 @@
>  
>    duplicateTab: function ssi_duplicateTab(aWindow, aTab, aDelta = 0) {
>      if (!aTab.ownerDocument || !aTab.ownerDocument.defaultView.__SSi ||
>          !aWindow.getBrowser)
> +      throw Components.Exception("Invalid tab object", Cr.NS_ERROR_INVALID_ARG);

Could you take the opportunity to throw several distinct errors?

@@ +1558,5 @@
>    duplicateTab: function ssi_duplicateTab(aWindow, aTab, aDelta = 0) {
>      if (!aTab.ownerDocument || !aTab.ownerDocument.defaultView.__SSi ||
>          !aWindow.getBrowser)
> +      throw Components.Exception("Invalid tab object", Cr.NS_ERROR_INVALID_ARG);
> +    

Nit: whitespace.

@@ +1589,5 @@
>      if ("__SSi" in aWindow) {
>        return NumberOfTabsClosedLastPerWindow.set(aWindow, aNumber);
>      }
>  
> +    throw Components.Exception("Invalid state string: window is not being tracked",

It's not a state string.

@@ +1607,5 @@
>        return Math.min(NumberOfTabsClosedLastPerWindow.get(aWindow) || 1,
>                        this.getClosedTabCount(aWindow));
>      }
>  
> +    throw Components.Exception("Invalid state string: window is not being tracked",

Not the state string.

@@ +1620,5 @@
>      if (DyingWindowCache.has(aWindow)) {
>        return DyingWindowCache.get(aWindow)._closedTabs.length;
>      }
>  
> +    throw Components.Exception("Invalid state string: window is not being tracked",

Not the state string.

@@ +1634,5 @@
>        let data = DyingWindowCache.get(aWindow);
>        return this._toJSONString(data._closedTabs);
>      }
>  
> +    throw Components.Exception("Invalid state string: window is not being tracked",

Not the state string.

@@ +1641,5 @@
>  
>    undoCloseTab: function ssi_undoCloseTab(aWindow, aIndex) {
>      if (!aWindow.__SSi)
> +      throw Components.Exception("Invalid state string: window is not being tracked",
> +                                 Cr.NS_ERROR_INVALID_ARG);

Not the state string.

@@ +1642,5 @@
>    undoCloseTab: function ssi_undoCloseTab(aWindow, aIndex) {
>      if (!aWindow.__SSi)
> +      throw Components.Exception("Invalid state string: window is not being tracked",
> +                                 Cr.NS_ERROR_INVALID_ARG);
> +    

Nit: whitespace.

@@ +1649,5 @@
>      // default to the most-recently closed tab
>      aIndex = aIndex || 0;
>      if (!(aIndex in closedTabs))
> +      throw Components.Exception("Invalid index: not in the closed tabs",
> +                                 Cr.NS_ERROR_INVALID_ARG);

Not the state string.

@@ +1650,5 @@
>      aIndex = aIndex || 0;
>      if (!(aIndex in closedTabs))
> +      throw Components.Exception("Invalid index: not in the closed tabs",
> +                                 Cr.NS_ERROR_INVALID_ARG);
> +    

Nit: whitespace.
Attachment #8339902 - Flags: review?(dteller) → feedback+
Attached patch bug944073.patch (obsolete) — Splinter Review
Thank you very much.
Attachment #8339902 - Attachment is obsolete: true
Attachment #8339935 - Flags: review?(dteller)
Comment on attachment 8339935 [details] [diff] [review]
bug944073.patch

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1442,5 @@
>        var state = JSON.parse(aState);
>      }
>      catch (ex) { /* invalid state object - don't restore anything */ }
> +    if (!state) {
> +      throw Components.Exception("Invalid state object: not JSON", Cr.NS_ERROR_INVALID_ARG);

Here, it's a state string.

@@ +1447,5 @@
> +    }
> +    if (!state.windows) {
> +      throw Components.Exception("Invalid state object: no windows restored", Cr.NS_ERROR_INVALID_ARG);
> +    }
> +    

Nit: Please don't add these whitespaces.

@@ +1499,5 @@
>    setWindowState: function ssi_setWindowState(aWindow, aState, aOverwrite) {
> +    if (!aWindow.__SSi) {
> +      throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);
> +    }
> +    

Please remove all these whitespaces. Click on the "review" link, you will see them highlighted in red.

@@ +1523,5 @@
>      // as |aState| can be an incomplete state that will be completed
>      // by |restoreTabs|.
>      let tabState = JSON.parse(aState);
>      if (!tabState) {
> +      throw Components.Exception("Invalid state object: not JSON", Cr.NS_ERROR_INVALID_ARG);

Here, it's a state string.

@@ +1591,5 @@
>      if ("__SSi" in aWindow) {
>        return NumberOfTabsClosedLastPerWindow.set(aWindow, aNumber);
>      }
>  
> +    throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);

Could you take the opportunity to reverse the flow of check?
i.e.

if (!("__SSi" in aWindow)) {
  throw Components.Exception(...);
}
return NumberOfTabsClosedLastPerWindow.set(...)

@@ +1609,5 @@
>        return Math.min(NumberOfTabsClosedLastPerWindow.get(aWindow) || 1,
>                        this.getClosedTabCount(aWindow));
>      }
>  
> +    throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);

Could you take the opportunity to reverse here, too?

@@ +1621,5 @@
>      if (DyingWindowCache.has(aWindow)) {
>        return DyingWindowCache.get(aWindow)._closedTabs.length;
>      }
>  
> +    throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);

Could you take the opportunity to reverse here, too?

@@ +1634,5 @@
>        let data = DyingWindowCache.get(aWindow);
>        return this._toJSONString(data._closedTabs);
>      }
>  
> +    throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);

Could you take the opportunity to reverse here, too?

@@ +1746,5 @@
>        this._windows[aWindow.__SSi].extData[aKey] = aStringValue;
>        this.saveStateDelayed(aWindow);
>      }
>      else {
> +      throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);

Could you take the opportunity to reverse here, too?
Attachment #8339935 - Flags: review?(dteller) → feedback+
Attached patch bug944073.patch (obsolete) — Splinter Review
Thanks!
Attachment #8339935 - Attachment is obsolete: true
Attachment #8340467 - Flags: review?(dteller)
Comment on attachment 8340467 [details] [diff] [review]
bug944073.patch

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

r+, with a few trivial changes
Note: in the future, could you number the successive versions of your patches?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1445,5 @@
> +    if (!state) {
> +      throw Components.Exception("Invalid state string: not JSON", Cr.NS_ERROR_INVALID_ARG);
> +    }
> +    if (!state.windows) {
> +      throw Components.Exception("Invalid state object: no windows restored", Cr.NS_ERROR_INVALID_ARG);

Just "no windows" will do.

@@ +1844,5 @@
>     */
>    restoreLastSession: function ssi_restoreLastSession() {
>      // Use the public getter since it also checks PB mode
> +    if (!this.canRestoreLastSession) {
> +      throw Components.Exception("Last session can not be restored");

You forgot Cr.NS_ERROR_FAILURE
Attachment #8340467 - Flags: review?(dteller) → review+
Final Version.
Thank you Yoric.
Attachment #8340467 - Attachment is obsolete: true
Attachment #8341043 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/080eb0c6441b
Keywords: checkin-needed
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/080eb0c6441b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Async][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async][mentor=Yoric][lang=js]
Target Milestone: --- → Firefox 28
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.