Bug 154536

Summary: [Fetch API] Implement Fetch API Response
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 151937    
Attachments:
Description Flags
Patch
none
Updating AtomicString none

Description youenn fablet 2016-02-22 08:19:46 PST
We should implement https://fetch.spec.whatwg.org/#response-class
Comment 1 youenn fablet 2016-02-22 08:50:40 PST
Created attachment 271924 [details]
Patch
Comment 2 Alex Christensen 2016-02-22 10:16:14 PST
Comment on attachment 271924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271924&action=review

> Source/WebCore/Modules/fetch/FetchResponse.cpp:134
> +        return ASCIILiteral("basic");

These could be AtomicStrings because there are a few well-known strings that are probably going to be used a lot.

> Source/WebCore/Modules/fetch/FetchResponse.h:62
> +    bool ok() const { return status() >= 200 && status() <= 299; }

I think this should be called "successful".  I know the spec calls it "ok" in https://fetch.spec.whatwg.org/#ok-status but I think that should be changed because of rfc 2616.
Comment 3 youenn fablet 2016-02-22 10:26:12 PST
Thanks for the feedback.

(In reply to comment #2)
> Comment on attachment 271924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271924&action=review
> 
> > Source/WebCore/Modules/fetch/FetchResponse.cpp:134
> > +        return ASCIILiteral("basic");
> 
> These could be AtomicStrings because there are a few well-known strings that
> are probably going to be used a lot.

OK.

> > Source/WebCore/Modules/fetch/FetchResponse.h:62
> > +    bool ok() const { return status() >= 200 && status() <= 299; }
> 
> I think this should be called "successful".  I know the spec calls it "ok"
> in https://fetch.spec.whatwg.org/#ok-status but I think that should be
> changed because of rfc 2616.

OK. Let's change it in FetchResponse only for now, and not in the IDL.
Do you want to bring that feedback to whatwg fetch github?
Also, it might be good to refer to RFC 7230 now.
Comment 4 Alex Christensen 2016-02-22 11:00:12 PST
(In reply to comment #3)
> OK. Let's change it in FetchResponse only for now, and not in the IDL.
OK.
> Do you want to bring that feedback to whatwg fetch github?
I think at this point there would be high risk (breaking compatibility with existing code) and little reward (making the api a little less confusing for web developers).  Let the record show I think "ok" is a bad name, but I think it's too late to change it.
Comment 5 Alex Christensen 2016-02-23 22:56:51 PST
Comment on attachment 271924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271924&action=review

This is pretty good WIP, and I would almost r+ it, but I think we need String -> AtomicString

> Source/WebCore/Modules/fetch/FetchBody.h:73
>      static FetchBody fromJSValue(JSC::ExecState&, JSC::JSValue);
>      static FetchBody fromRequestBody(FetchBody*);
> +    static FetchBody empty() { return FetchBody(); }

Is there a reason these aren't just constructors?  Then we could use { } to return an empty fetch body.
Comment 6 youenn fablet 2016-02-24 11:55:16 PST
Created attachment 272132 [details]
Updating AtomicString
Comment 7 youenn fablet 2016-02-24 12:00:12 PST
(In reply to comment #5)
> Comment on attachment 271924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271924&action=review
> 
> This is pretty good WIP, and I would almost r+ it, but I think we need
> String -> AtomicString

I updated the patch to use AtomicString.
I wonder what is the big picture here. Should all enum-returning functions use AtomicString?
FetchRequest should be updated also I guess.
Also, it should really be the binding generator that should generate the code to convert the C++ enum-type into an AtomicString.

> > Source/WebCore/Modules/fetch/FetchBody.h:73
> >      static FetchBody fromJSValue(JSC::ExecState&, JSC::JSValue);
> >      static FetchBody fromRequestBody(FetchBody*);
> > +    static FetchBody empty() { return FetchBody(); }
> 
> Is there a reason these aren't just constructors?  Then we could use { } to
> return an empty fetch body.

I agree they should be reworked a bit.
There is a FIXME in FetchBody to finalize FetchBody::fromRequestBody.
I plan to do that refactoring as a follow-up patch.
Comment 8 Alex Christensen 2016-02-24 12:28:04 PST
(In reply to comment #7)
> I updated the patch to use AtomicString.
> I wonder what is the big picture here. Should all enum-returning functions
> use AtomicString?
I was just thinking we shouldn't allocate a string each time we call this function if we know that there is a small number of constant strings it could return.  AtomicStrings are usually used for this, but I made a mistake: AtomicStrings can only be used from the main thread, which means we can't use them for the fetch api.  I think the correct solution is to use std::call_once to initialize a bunch of NeverDestroyed<String> (not AtomicString).
> FetchRequest should be updated also I guess.
I think so.  Maybe not in this patch.
> Also, it should really be the binding generator that should generate the
> code to convert the C++ enum-type into an AtomicString.
This might be nice to add to the bindings generator eventually.
Comment 9 youenn fablet 2016-02-24 12:47:00 PST
(In reply to comment #8)
> (In reply to comment #7)
> > I updated the patch to use AtomicString.
> > I wonder what is the big picture here. Should all enum-returning functions
> > use AtomicString?
> I was just thinking we shouldn't allocate a string each time we call this
> function if we know that there is a small number of constant strings it
> could return.  AtomicStrings are usually used for this, but I made a
> mistake: AtomicStrings can only be used from the main thread, which means we
> can't use them for the fetch api.  I think the correct solution is to use
> std::call_once to initialize a bunch of NeverDestroyed<String> (not
> AtomicString).

Would it be possible then to land the previous patch as is and update at once FetchRequest and FetchResponse as a follow-up patch?
Comment 10 Alex Christensen 2016-02-24 12:49:40 PST
Comment on attachment 271924 [details]
Patch

(In reply to comment #9)
> Would it be possible then to land the previous patch as is and update at
> once FetchRequest and FetchResponse as a follow-up patch?
I think so.  Sorry for making you jump through hoops
Comment 11 youenn fablet 2016-02-24 12:56:07 PST
(In reply to comment #10)
> Comment on attachment 271924 [details]
> Patch
> 
> (In reply to comment #9)
> > Would it be possible then to land the previous patch as is and update at
> > once FetchRequest and FetchResponse as a follow-up patch?
> I think so.  Sorry for making you jump through hoops

Thanks for the r+/cq+ :)
Also, the AtomicString discussion is interesting and will probably be beneficial in the future.
Comment 12 WebKit Commit Bot 2016-02-24 13:41:47 PST
Comment on attachment 271924 [details]
Patch

Clearing flags on attachment: 271924

Committed r197049: <http://trac.webkit.org/changeset/197049>
Comment 13 WebKit Commit Bot 2016-02-24 13:41:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2016-02-25 08:51:49 PST
(In reply to comment #8)
> I think the correct solution is to use
> std::call_once to initialize a bunch of NeverDestroyed<String> (not
> AtomicString).

That won’t work. We can’t use a single WTF::String on multiple threads. The reference counting is not thread safe and these strings would end up with incorrect reference counts.
Comment 15 Alex Christensen 2016-02-25 10:23:25 PST
(In reply to comment #14)
> (In reply to comment #8)
> > I think the correct solution is to use
> > std::call_once to initialize a bunch of NeverDestroyed<String> (not
> > AtomicString).
> 
> That won’t work. We can’t use a single WTF::String on multiple threads. The
> reference counting is not thread safe and these strings would end up with
> incorrect reference counts.
IIRC there used to be a way to make Strings that last forever by using an odd ref count, because the ref counts are incremented and decremented by two.  Then it doesn't matter if threads mess up the refcount, because it will always be a non-zero number.  Search for s_refCountFlagIsStaticString.  I'm not sure if that's still used.