Closed Bug 1291834 Opened 4 years ago Closed 4 years ago

remove require of sdk/index-db.js

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Status
firefox51 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [reserve-html])

Attachments

(2 files)

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.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Blocks: 1266847
Iteration: --- → 51.1 - Aug 15
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [devtools-html] → [reserve-html]
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!
Attachment #8777513 - Flags: review?(jryans) → review+
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.
Attachment #8777514 - Flags: review?(jryans)
I'm sorry I missed the indexedDB module in builtin-modules.js.  Oops!
The new patch does as you suggest.
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/
Attachment #8777514 - Flags: review?(jryans)
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/
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!
Attachment #8777514 - Flags: review?(jryans) → review+
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
https://hg.mozilla.org/mozilla-central/rev/c4ad9dd4deae
https://hg.mozilla.org/mozilla-central/rev/006c9345adb4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.