Tom Tromey :tromey Assignee
	Description • 8 years ago

Bug 1266847 points out that we need to handle the sdk/index-db.js dependency
somehow.  This affects async-storage.js.

One approach would be to change builtin-modules.js to expose indexDb as a global.
This is consistent with what we've done elsewhere.

	Tom Tromey :tromey Assignee
	Comment 1 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/69052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69052/

	Tom Tromey :tromey Assignee
	Comment 2 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/69054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69054/

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 3 • 8 years ago

Comment on attachment 8777513 [details]
Bug 1291834 - make devtools/shared/async-storage.js eslint-clean;

https://reviewboard.mozilla.org/r/69052/#review66420

Looks good, thanks!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 4 • 8 years ago

Comment on attachment 8777514 [details]
Bug 1291834 - make indexDB a global via devtools loader;

https://reviewboard.mozilla.org/r/69054/#review66452

::: devtools/shared/builtin-modules.js:296
(Diff revision 1)
>  });
>  defineLazyGetter(globals, "CSSRule", () => Ci.nsIDOMCSSRule);
>  defineLazyGetter(globals, "DOMParser", () => {
>    return CC("@mozilla.org/xmlextras/domparser;1", "nsIDOMParser");
>  });
> +lazyRequireGetter(globals, "indexedDB", "sdk/indexed-db", true);

In general, I agree it makes sense to expose an `indexedDB` global like you are doing here, since that is how it's exposed to regular web content.

However, with the current patch here, we are in a confusing state, because we also expose an `indexedDB` _module_ earlier in the file.  For additional confusing, that variation is just the indexedDB DOM API directly without the SDK wrapper.  

These two variants aren't equivalent either, because the SDK wrapper constructs a special principal derived from IDs in the loader, which isolates data stored using that wrapper from other DBs, so dropping the SDK wrapper (for example) would means losing access to existing data that was previously stored using the wrapper.  (This may not matter a whole lot in practice, since devtools.html in content would also lose this data since the origin changes, but just something to think about.)

It looks like devtools/server/actors/storage.js is the only consumer of the existing `indexedDB` module.  Maybe a good approach is to remove the existing module from here and make it a regular function inside the storage actor.  Then your global could remain as is, and we'd continue with SDK wrapper on the UI side for the moment.

	Tom Tromey :tromey Assignee
	Comment 5 • 8 years ago

I'm sorry I missed the indexedDB module in builtin-modules.js.  Oops!
The new patch does as you suggest.

	Tom Tromey :tromey Assignee
	Comment 6 • 8 years ago

Comment on attachment 8777513 [details]
Bug 1291834 - make devtools/shared/async-storage.js eslint-clean;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69052/diff/1-2/

	Tom Tromey :tromey Assignee
	Comment 7 • 8 years ago

Comment on attachment 8777514 [details]
Bug 1291834 - make indexDB a global via devtools loader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69054/diff/1-2/

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 8 • 8 years ago

Comment on attachment 8777514 [details]
Bug 1291834 - make indexDB a global via devtools loader;

https://reviewboard.mozilla.org/r/69054/#review66792

Thanks, looks good to me!

	Pulsebot
	Comment 11 • 8 years ago

Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4ad9dd4deae
make devtools/shared/async-storage.js eslint-clean; r=jryans
https://hg.mozilla.org/integration/autoland/rev/006c9345adb4
make indexDB a global via devtools loader; r=jryans

	Carsten Book [:Tomcat]
	Comment 12 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c4ad9dd4deae
https://hg.mozilla.org/mozilla-central/rev/006c9345adb4