489869 - Create a new nsISessionStore interface for 1.9.1 branch

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Bug 394759 added to the session store IDL but didn't rev the uuid (on branch only), so binary extensions can't use the methods added.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

The patch (doing it how I think Gavin said I was supposed to)

Comment 2

[Simon Bünzli]

Comment on attachment 375209 [details] [diff] [review]
Patch v0.1

>+interface nsISessionStore_MOZILLA_1_9_1 : nsISupports

Any reason for not inheriting from nsISessionStore so that 1.9.1 extensions can just QI for this interface and get everything?

>   QueryInterface: XPCOMUtils.generateQI([Ci.nsISessionStore,
>                                          Ci.nsIDOMEventListener,
>                                          Ci.nsIObserver,
>-                                         Ci.nsISupportsWeakReference]),
>+                                         Ci.nsISupportsWeakReference,
>+                                         Ci.nsISessionStore_MOZILLA_1_9_1,
>+                                         Ci.nsIClassInfo]),
>+
>+  getInterfaces: function(countRef) {
>+    let interfaces = [Ci.nsISessionStore, Ci.nsIDOMEventListener,
>+                      Ci.nsIObserver, Ci.nsISupportsWeakReference,
>+                      Ci.nsISessionStore_MOZILLA_1_9_1, Ci.nsIClassInfo];
>+    countRef.value = interfaces.length;
>+    return interfaces;
>+  },

Is this redundancy really necessary?

>+  getHelperForLanguage: function(language) null,

Nit: Function arguments always have an a prefix in this file: aLanguage.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> Any reason for not inheriting from nsISessionStore so that 1.9.1 extensions can
> just QI for this interface and get everything?

nsIClassInfo should remove the need for people to worry about the branch interface entirely, right? I think that makes inheritance here unnecessary.

> Is this redundancy really necessary?

getInterfaces usually omits nsISupports, but including it probably doesn't hurt, so I suppose you could use a member variable to store the array of interfaces.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> (In reply to comment #2)
> > Any reason for not inheriting from nsISessionStore so that 1.9.1 extensions can
> > just QI for this interface and get everything?
> 
> nsIClassInfo should remove the need for people to worry about the branch
> interface entirely, right? I think that makes inheritance here unnecessary.

I'm new to this and not so familiar with C++/XPCOM/whatever, but doesn't it need to inherit for binary consumers to be able to use the nsISessionStore methods?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> I'm new to this and not so familiar with C++/XPCOM/whatever, but doesn't it
> need to inherit for binary consumers to be able to use the nsISessionStore
> methods?

No, they can just QI to either of the interfaces. However, inheritance would avoid the need for that, and I can't really think of a reason not to do it, so feel free I guess.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Put the interfaces in a constant, added the other things needed for nsIClassInfo. It's on tryserver making sure it doesn't break anything

Comment 7

[Simon Bünzli]

Comment on attachment 375363 [details] [diff] [review]
Patch v0.2

Nit: s/countRef/aCountRef/.

BTW: Do we also have to revert nsISessionStore's IID?

r+=me with that fixed.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #7)
> Nit: s/countRef/aCountRef/.

I meant to ask about that... since countRef is actually an out param, does it still get the "a" prefix?

> BTW: Do we also have to revert nsISessionStore's IID?

It got reverted before beta 4 freeze: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b057539eee94

Comment 9

[Simon Bünzli]

(In reply to comment #8)
> since countRef is actually an out param, does it still get the "a" prefix?

It does: the "a" just stands for Argument (as opposed to "g" for Global and "m" for Module). For reference, see <https://developer.mozilla.org/en/JavaScript_style_guide#Function_and_variable_naming>

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v1.0

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 375427 [details] [diff] [review]
Patch v1.0

a191=beltzner

Comment 12

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bca816f58a7f