Created attachment 271924 [details] Patch

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.

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.

(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 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.

Created attachment 272132 [details] Updating AtomicString

(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.

(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.

(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 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

(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 on attachment 271924 [details] Patch Clearing flags on attachment: 271924 Committed r197049: <http://trac.webkit.org/changeset/197049>

All reviewed patches have been landed. Closing bug.

(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.

(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.