Closed Bug 1216736 Opened 4 years ago Closed 4 years ago

Write some tests to ensure fetch is safe to use from chrome script

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

Attachments

(1 file, 2 obsolete files)

The security state client (for OneCRL, HSTS preloads, etc) makes use of kinto.js - to keep variations between the different versions of kinto.js to a minimum, it's useful if the APIs it can use are the same, where possible, across platforms.

kinto.js uses fetch to sync with a kinto server. We need a fetch-like way of making this work.
Attached patch Bug1216736.patch (obsolete) — Splinter Review
This creates a (thin) wrapper around XHR to provide a (minimal) fetch-like interface.
Attachment #8676473 - Flags: review?(rnewman)
Perhaps a silly question, but why can't we just use Fetch?
Flags: needinfo?(mgoodwin)
(In reply to Dave Townsend [:mossop] from comment #3)
> Perhaps a silly question, but why can't we just use Fetch?

Not a silly question; I didn't think we could.

It took me a while to figure out, but...

Cu.importGlobalProperties(['fetch']);

...does the trick.  I have questions, though:

1) How does this work? A peek at the fetch webidl seems to suggest that this API isn't exported to System - what does 'Export' mean in this context
2) I see no-one else doing this outside of tests; should I check this is OK to use in this context, or do I just go ahead?
Flags: needinfo?(mgoodwin) → needinfo?(dtownsend)
(In reply to Mark Goodwin [:mgoodwin] from comment #4)
> (In reply to Dave Townsend [:mossop] from comment #3)
> > Perhaps a silly question, but why can't we just use Fetch?
> 
> Not a silly question; I didn't think we could.
> 
> It took me a while to figure out, but...
> 
> Cu.importGlobalProperties(['fetch']);
> 
> ...does the trick.  I have questions, though:
> 
> 1) How does this work? A peek at the fetch webidl seems to suggest that this
> API isn't exported to System - what does 'Export' mean in this context

I'm not sure what "Export" you're referring to here. Cu.importGlobalProperties allows us to use things normally only exposed to webcontent in jsms and other sandboxes.

> 2) I see no-one else doing this outside of tests; should I check this is OK
> to use in this context, or do I just go ahead?

Andrea, is it ok for us to use Fetch like this?
Flags: needinfo?(dtownsend) → needinfo?(amarchesini)
(In reply to Dave Townsend [:mossop] from comment #5)
> I'm not sure what "Export" you're referring to here.

I meant 'expose'.
09:56 <mgoodwin> Hi, I was wondering if it's OK for me to use fetch from chrome JS
09:57 <baku> it's not exposed to system, so no.
09:57 <mgoodwin> Reason I ask; I see some places doing fetch-like things (e.g. in devtools) and rolling their own instead of using 
                 it
09:57 <baku> we could expose it... I don't see any reason why it's not.
09:57 <mgoodwin> Well it's not exposed but you can Cu.importGlobalProperties(['fetch']);
09:57 <mgoodwin> which works fine
09:58 <mgoodwin> I take it that exposing just add the magic glue so you don't have to do the importGlobalProperties step?
10:00 <baku> back
10:00 <baku> yes, we dont have tests about it.
10:00 <baku> so I cannot tell you if it's fully OK or not.
10:00 <baku> it should work, but yes, would be nice to test it properly.
10:00 <mgoodwin> OK. so I have some options.
10:00 <mgoodwin> 1) Just use it
10:01 <mgoodwin> 2) Write some tests, use it without exposing
10:01 <mgoodwin> 3) Write some tests *and* expose to system
10:02 <baku> I would start with 1, if it works-ish... do 2.
10:02 <mgoodwin> What's your preferred approach? Who should make the call on this?
10:02 <mgoodwin> Oh, it works fine.
10:03 <baku> thanks!
10:03 <mgoodwin> I wrote some tests for bug 1216736 (Where I wrote an alternative impl - you're ni? in that by the way)
10:04 <mgoodwin> I can include just the tests for the bits I need and document in the place where I importGlobal... that if others 
                 do the same they might want to add tests?
10:05 <baku> mgoodwin: sorry if I didn't answer yet, I'm a bit overloaded of NI :/
10:05 <mgoodwin> Yeah, I know people are busy. 
10:05 <baku> but yes, the best would be to have some basic tests
10:05 <mgoodwin> I'll paste this chat in IRC and clear the ni? for you :)
10:05 <baku> and import them in mochitests and chrome/browser tests
10:06 <mgoodwin> where should the test live? With the code that uses it or in some other component?
10:06 <baku> so far we have tests in dom/tests/fetch/
10:06 <mgoodwin> ok, thanks
Flags: needinfo?(amarchesini)
Summary: Create a fetch-like wrapper for XHR for kinto.js (and other things) to use → Write some tests to ensure fetch is safe to use from chrome script
Attached patch Bug1216736.patch (obsolete) — Splinter Review
As a starting point, here are the tests I wrote before moved to dom/tests/unit and reworked to use Fetch via importGlobalProperties.
Attachment #8676473 - Attachment is obsolete: true
Attachment #8676473 - Flags: review?(rnewman)
Attachment #8679421 - Flags: feedback?(amarchesini)
As per comment 8, this probably doesn't need to block bug 1197703
No longer blocks: 1197703
Attachment #8679421 - Flags: feedback?(amarchesini) → feedback+
Bug 1216736 - Provide xpcshell tests for basic Fetch usage from chrome js r?baku
Attachment #8684199 - Flags: review?(amarchesini)
I've marked this is as blocking Bug 1197707 since I want to ensure we have test coverage before we're relying on this.
Blocks: 1197707
Comment on attachment 8684199 [details]
MozReview Request: Bug 1216736 - Provide xpcshell tests for basic Fetch usage from chrome js r?baku

https://reviewboard.mozilla.org/r/24497/#review23243

::: dom/tests/unit/xpcshell.ini:3
(Diff revision 1)
>  tail = 

can you fix these 2 extra spaces?
Attachment #8684199 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2ec0e9bc9ffa8417ab2e6f5672f955eba60e1c
Bug 1216736 - Provide xpcshell tests for basic Fetch usage from chrome js r=baku
Attachment #8679421 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ae2ec0e9bc9f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.