1040771 - Allow about: pages to opt in to IndexedDB optionally specifying a desired origin.

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

(From Bug 1028187 comment #0)
> Talking to :tOkeshu he told me that about:loop* pages need IndexedDB storage
> and folks are currently experimenting using message passing for that. Seems
> like we should just follow bug 861302 with the exception that we make both
> about:loop* pages share an artificial origin.

Also Bug 1028187 added a method to nsIAboutModule.idl so we need to update our implementation.

See: https://hg.mozilla.org/mozilla-central/rev/acf444215f8d#l9.42

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

> +  getModule: function(aURI) {
getModule: or _getModule: or just do it as a function outside the About object?

> +  getIndexedDBOriginPostfix: function(aURI) {
> +    if (this.getURIFlags(aURI) & INDEXEDDB) {
> +      var postfix = this[this.getModule(aURI) + "Postfix"];
> +      return postfix ? postfix : null;
> +    }
> +    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

I hope I've understood the Firefox patch correctly since the Firefox nsAbout C++ module uses NS_ERROR_ILLEGAL_VALUE instead.

Comment 2

[[e-mail]]

Comment on attachment 8458684 [details] [diff] [review]
Patch v1.0 Proposed fix.

You would throw NS_ERROR_NOT_IMPLEMENTED if you weren't going to support indexeddb at all (which is still an option at this point, as we don't have any about: pages that need it). As it is, if you want to provide the option to support it in the future you should show something else, such as the other value you suggested.

Instead of testing for the flag in getIndexedDBOriginPostfix, you could just throw if the postfix is undefined.

Comment 3

[Philip Chee]

> You would throw NS_ERROR_NOT_IMPLEMENTED if you weren't going to support 
> indexeddb at all (which is still an option at this point, as we don't have 
> any about: pages that need it). As it is, if you want to provide the 
> option to support it in the future you should show something else, such as 
> the other value you suggested.

https://hg.mozilla.org/mozilla-central/rev/acf444215f8d
about:blank about:bloat about:cache all return NS_ERROR_NOT_IMPLEMENTED. Should they return NS_ERROR_ILLEGAL_VALUE instead?

> Instead of testing for the flag in getIndexedDBOriginPostfix, you could 
> just throw if the postfix is undefined.

I need to return a null if the postfix is undefined so I can't throw. Notice that in the Firefox patch, about:home has the ENABLE_INDEXED_DB flag but no postfix defined.

https://hg.mozilla.org/mozilla-central/rev/acf444215f8d#l9.42
The IDL seems to say that getIndexedDBOriginPostfix() returning null is acceptable and this signals the caller to construct a origin based on the about: URI's path

> +     * Returns the Indexed DB origin's postfix used for the given about: URI.
> +     * If the postfix returned is null then the URI's path (e.g. "home" for
> +     * about:home) will be used to construct the origin.

This is needed (in Firefox) because about:loopconversation and about:looppanel need to share the same origin for indexedDB.

Comment 4

[[e-mail]]

(In reply to Philip Chee from comment #3)
> > You would throw NS_ERROR_NOT_IMPLEMENTED if you weren't going to support 
> > indexeddb at all (which is still an option at this point, as we don't have 
> > any about: pages that need it). As it is, if you want to provide the 
> > option to support it in the future you should show something else, such as 
> > the other value you suggested.
> 
> https://hg.mozilla.org/mozilla-central/rev/acf444215f8d
> about:blank about:bloat about:cache all return NS_ERROR_NOT_IMPLEMENTED.
> Should they return NS_ERROR_ILLEGAL_VALUE instead?

Those ones always return NS_ERROR_NOT_IMPLEMENTED as they assume that they're not actually going to get called because they don't support the indexeddb value. The /browser/ redirector is expecting to support indexeddb but only for those three values, other ones would be illegal.

> Notice that in the Firefox patch, about:home has the ENABLE_INDEXED_DB flag
> but no postfix defined.

Well, I guess they could have written it so that about:home was explicitly defined to be null, which would have allowed them to return the value if it was explicitly defined otherwise throw.

Comment 5

[[e-mail]]

Ah, I think I get it now.

In firefox, about:home, about:loopconversation and about:looppanel need indexeddb. Additionally, about:looppanel needs to share its indexeddb with about:loopconversation. So, getIndexedDBOriginPostfix can either:
* Return an override postfix, i.e. looppanel returns "loopconverstaion"
* Return null, meaning use the default postfix e.g. about:home
* Throw for invalid pages (those not using indexeddb)

Comment 6

[[e-mail]]

Comment on attachment 8458684 [details] [diff] [review]
Patch v1.0 Proposed fix.

>+    if (this.getURIFlags(aURI) & INDEXEDDB) {
>+      var postfix = this[this.getModule(aURI) + "Postfix"];
>+      return postfix ? postfix : null;
Better to use || i.e.
if (this.getURIFlags(aURI) & INDEXEDDB)
  return this[this.getModule(aURI) + "Postfix"] || null;

>+    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
Use NS_ERROR_ILLEGAL_VALUE as previously mentioned.

Comment 7

[Philip Chee]

Attachment: Patch v2.0 fix nits.

>>+    if (this.getURIFlags(aURI) & INDEXEDDB) {
>>+      var postfix = this[this.getModule(aURI) + "Postfix"];
>>+      return postfix ? postfix : null;
> Better to use || i.e.
> if (this.getURIFlags(aURI) & INDEXEDDB)
>   return this[this.getModule(aURI) + "Postfix"] || null;
Done.

>>+    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> Use NS_ERROR_ILLEGAL_VALUE as previously mentioned.
Done.

Comment 8

[Philip Chee]

Comment on attachment 8463021 [details] [diff] [review]
Patch v2.0 fix nits.

[Approval Request Comment]
Regression caused by (bug #): Bug 1028187
User impact if declined: Probably nothing nothing uses this but the Firefox loop client.
Testing completed (on m-c, etc.): has baked on comm-central and has been live in Firefox
Risk to taking this patch (and alternatives if risky): can't think of anything risky.
String changes made by this patch: None.

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 8463021 [details] [diff] [review]
Patch v2.0 fix nits.

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

" Probably nothing nothing uses this but the Firefox loop client."

Since we have no UI for this, I don't see a benefit to taking right now, lets let it ride the trains.

Comment 10

[Philip Chee]

Pushed to comm-central on Wed Oct 01 17:08:14 2014
http://hg.mozilla.org/comm-central/rev/e09d309ad19a