Johann Hofmann [:johannh] Assignee
	Description • 6 years ago

Usage numbers for storage: persistent have returned to what seems like the more likely reflection of reality: https://mzl.la/2GVyQ7g

In preparation of writing a blog post declaring the deprecation, we want to add a console warning for developers.

	Johann Hofmann [:johannh] Assignee
	Comment 2 • 6 years ago

Intent to unship will follow tomorrow, I wanted to get the deprecation note out for review since I had the patch already. Thanks!

	Andrew Sutherland [:asuth] (he/him)
	Comment 3 • 6 years ago
mozreview-review

Comment on attachment 8956212 [details]
Bug 1442560 - Add a note for deprecating passing a storage option to indexedDB.open().

https://reviewboard.mozilla.org/r/225122/#review231126

Restating: For window globals, we will warn once per non-SystemPrincipal'ed document if IDBFactory.open()'s second-arg-is-dictionary variant is invoked and "storage" is passed, regardless of the value.  With the change below, we will also warn on workers.  We will not, however, warn for JSM/Sandbox IndexedDB instances.  (If we lost the main thread guard in the else case I propose below, we'd get coverage there too.  I'm fine with that if you think that makes sense.)

::: dom/indexedDB/IDBFactory.cpp:459
(Diff revision 1)
>    if (!IsChrome() &&
>        aOptions.mStorage.WasPassed()) {
>  
> +    if (mWindow && mWindow->GetExtantDoc()) {
> +      mWindow->GetExtantDoc()->WarnOnceAbout(nsIDocument::eIDBOpenDBOptions_StorageType);
> +    }

It seems likely that any code fancy enough to use persistent storage may also be using Workers.  We don't have suppression logic in-place, but I think it can be argued that this is a situation where spamming the console is okay.  The logic would go something like this:

```
} else if (!NS_IsMainThread()) {
  // (The method below reports on the main thread too, so we need to make sure we're on a worker.)
  // Workers don't have a WarnOnceAbout mechanism, so this will be reported every time.
  WorkerPrivate::ReportErrorToConsole("IDBOpenDBOptions_StorageType");
}
```

	Johann Hofmann [:johannh] Assignee
	Comment 5 • 6 years ago

Thank you for the suggestion! I think we should indeed warn on workers but I think internal usage is limited enough that we can deal with it without spamming the browser console. I made your suggested changes and I'll land this now given that the intent to unship didn't receive any negative feedback so far.

It's probably good to have this in ESR 60.

	Pulsebot
	Comment 6 • 6 years ago

Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78dd2074db57
Add a note for deprecating passing a storage option to indexedDB.open(). r=asuth

	Stefan Hindli [:stefan_hindli]
	Comment 7 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/78dd2074db57

	Kohei Yoshino [:kohei]
	Comment 8 • 6 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/storage-option-for-indexeddb-open-has-been-deprecated/

	Johann Hofmann [:johannh] Assignee
	Comment 9 • 6 years ago

It would be good to remove this from prominently appearing on MDN everywhere (and add a deprecation note):

https://developer.mozilla.org/en-US/docs/Web/API/IDBFactory/open#Syntax

https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Browser_storage_limits_and_eviction_criteria#Different_types_of_data_storage

	Chris Mills [:cmills]
	Comment 10 • 6 years ago

(In reply to Johann Hofmann [:johannh] from comment #9)
> It would be good to remove this from prominently appearing on MDN everywhere
> (and add a deprecation note):
> 
> https://developer.mozilla.org/en-US/docs/Web/API/IDBFactory/open#Syntax
> 
> https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/
> Browser_storage_limits_and_eviction_criteria#Different_types_of_data_storage

Thanks for the help with the docs!

I've adjusted the wording and layout a bit, and also added a note to the Fx60 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/60#DOM

Let me know if you'd like to see any other changes.

	Johann Hofmann [:johannh] Assignee
	Comment 11 • 6 years ago

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #10)
> (In reply to Johann Hofmann [:johannh] from comment #9)
> > It would be good to remove this from prominently appearing on MDN everywhere
> > (and add a deprecation note):
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/IDBFactory/open#Syntax
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/
> > Browser_storage_limits_and_eviction_criteria#Different_types_of_data_storage
> 
> Thanks for the help with the docs!
> 
> I've adjusted the wording and layout a bit, and also added a note to the
> Fx60 rel notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/60#DOM
> 
> Let me know if you'd like to see any other changes.

That seems great, thank you!