Generate RequestInit with the IDL compiler |
||||||||
Issue descriptionIn the RequestInit class we now perform some logic to validate the members [1] of the RequestInit dictionary [2]. This involves checking to make sure provided strings are one of the specified values for the respective enums, and throwing an TypeError otherwise. The result, upon construction with no validation issues, in a RequestInit object with String members representing validated dictionary values. In Request (request.cc)'s CreateRequestWithRequestOrString method, there are several case ([3] & [4], for example), where we use the validated members of a RequestInit to set a Request object's members. The attributes that we set are not Strings, so here we perform conversions from several RequestInit String members => network::mojom::Fetch* enum values & set them appropriately. As a result, we end up performing the same String equality checks for RequestInit members twice: 1.) In RequestInit::CheckEnumValues, to ensure that the values are at least valid 2.) In Request::CreateRequestWithRequestOrString ([3] & [4], for example again), to turn String members into the correct enum values. After talking to Yoav about this, we figured it seems wise to perform this check only once for both performance and code health. Ideally, the RequestInit class can deal in fetch enum values instead of String members, so that we can perform the pesky String value checks only once, and use the members (enum values) to directly set attributes on a Request object in CreateRequestWithRequestOrString. There may be some reason (that we're missing) that mandates the RequestInit object to deal only in Strings and not the fetch enum values, but if we can gather some opinions to see if this is a viable move, it seems worthwhile. [1]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/fetch/request_init.cc?q=request_init.cc&sq=package:chromium&l=135 [2]: https://fetch.spec.whatwg.org/#requestinit [3]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/fetch/request.cc?q=CreateRequestWithRe&sq=package:chromium&l=248
,
Apr 26 2018
Sounds good.
,
Apr 26 2018
,
Apr 27 2018
,
May 31 2018
,
Jun 12 2018
After talking with peria@ and yhirano@, it looks like we can probably use the IDL compiler to generate our RequestInit implementation, with a workaround for handling RequestInit's body member. Going to look into this further, and if I can get something working correctly I'll change the title/description of this issue and continue from there.
,
Jun 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/95c9b507808dea5e059be4375042d108640caf51 commit 95c9b507808dea5e059be4375042d108640caf51 Author: Dominic Farolino <domfarolino@gmail.com> Date: Fri Jun 15 05:15:40 2018 Preparation work to generate RequestInit with IDL compiler This CL takes most of the logic in RequestInit::SetUpBody, intended to implement "extract a body", modulo the ReadableStream conversion (https://fetch.spec.whatwg.org/#concept-bodyinit-extract), and moves it to a static function in request.cc. This is some preparation work before adding request_init.idl and generating the RequestInit implementation with the IDL compiler, which is itself some preparation work to getting the BodyInit typedef to work properly in every place it is used (right now only in Request::CreateRequestWithRequestOrString and Response::Create). R=kinuko@chromium.org, kouhei@chromium.org, yhirano@chromium.org, yoav@yoav.ws Bug: 836873 Change-Id: I229c97c761800e0d2615f9f19fd822564978db6e Reviewed-on: https://chromium-review.googlesource.com/1098676 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Cr-Commit-Position: refs/heads/master@{#567553} [modify] https://crrev.com/95c9b507808dea5e059be4375042d108640caf51/third_party/blink/renderer/core/fetch/request.cc [modify] https://crrev.com/95c9b507808dea5e059be4375042d108640caf51/third_party/blink/renderer/core/fetch/request_init.cc [modify] https://crrev.com/95c9b507808dea5e059be4375042d108640caf51/third_party/blink/renderer/core/fetch/request_init.h
,
Jun 15 2018
,
Jun 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4fb331ac25e10fce679b7d6d61677fe77388b8da commit 4fb331ac25e10fce679b7d6d61677fe77388b8da Author: Dominic Farolino <domfarolino@gmail.com> Date: Fri Jun 22 19:07:01 2018 Move RequestInit constructor logic to Request Before this CL we have some Request constructor logic in our implementation of RequestInit. Once we generate RequestInit with the IDL compiler, that logic will have to be inlined back into the Request constructor. This CL moves that logic, revolving around the referrer property, back to the Request constructor to minimize the size of the CL that removes our implementation of RequestInit. R=kinuko@chromium.org, kouhei@chromium.org, yhirano@chromium.org, yoav@yoav.ws Bug: 836873 Change-Id: Id351d9503ead1ef4e377fa0bd1cc5eaac70cfbed Reviewed-on: https://chromium-review.googlesource.com/1103786 Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Yoav Weiss <yoav@yoav.ws> Cr-Commit-Position: refs/heads/master@{#569721} [modify] https://crrev.com/4fb331ac25e10fce679b7d6d61677fe77388b8da/third_party/blink/renderer/core/fetch/request.cc [modify] https://crrev.com/4fb331ac25e10fce679b7d6d61677fe77388b8da/third_party/blink/renderer/core/fetch/request_init.cc [modify] https://crrev.com/4fb331ac25e10fce679b7d6d61677fe77388b8da/third_party/blink/renderer/core/fetch/request_init.h
,
Jun 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b3559bebea6016d9fc5d825f47dd0837c2068145 commit b3559bebea6016d9fc5d825f47dd0837c2068145 Author: Dominic Farolino <domfarolino@gmail.com> Date: Thu Jun 28 01:43:12 2018 Remove RequestInit implementation in favor of IDL compiler's version This CL removes our RequestInit implementation and replaces it with one the IDL compiler generates. R=kinuko@chromium.org, kouhei@chromium.org, yhirano@chromium.org, yoav@yoav.ws Bug: 836873 Change-Id: I29c55be8756deb5053cec04a5f82677d48413533 Reviewed-on: https://chromium-review.googlesource.com/1116418 Reviewed-by: Yoav Weiss <yoav@yoav.ws> Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Cr-Commit-Position: refs/heads/master@{#570997} [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/core_idl_files.gni [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/BUILD.gn [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/global_fetch.cc [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/global_fetch.h [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/request.cc [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/request.h [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/request.idl [delete] https://crrev.com/2c474b750cacded56a3164bb9cbe3619121253e7/third_party/blink/renderer/core/fetch/request_init.cc [delete] https://crrev.com/2c474b750cacded56a3164bb9cbe3619121253e7/third_party/blink/renderer/core/fetch/request_init.h [add] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/request_init.idl [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/window_fetch.idl [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/core/fetch/worker_fetch.idl [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/modules/background_fetch/background_fetch_manager_test.cc [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/modules/cache_storage/cache.cc [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/modules/cache_storage/cache_test.cc [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.cc [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.h [modify] https://crrev.com/b3559bebea6016d9fc5d825f47dd0837c2068145/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.idl
,
Jun 28 2018
,
Jul 3
Re-opening this since there was a regression introduced that is being tackled in http://crrev.com/c/1123771.
,
Jul 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a63b165102676ece361dd598659551c5b0d0ba4 commit 9a63b165102676ece361dd598659551c5b0d0ba4 Author: Dominic Farolino <domfarolino@gmail.com> Date: Wed Jul 04 02:17:11 2018 Fix AbortSignal conversion regression By working around a bug in the IDL compiler when generating RequestInit with said compiler, a regression was introduced that stopped TypeErrors from being thrown when RequestInit's signal member does not implement the AbortSignal interface. This CL fixes that regression and adds tests for it. R=kinuko@chromium.org, yoav@yoav.ws Bug: 836873 Change-Id: I6ab7be2ee8356eb7a997b904d121a7061986096a Reviewed-on: https://chromium-review.googlesource.com/1123771 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Adam Rice <ricea@chromium.org> Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Cr-Commit-Position: refs/heads/master@{#572449} [modify] https://crrev.com/9a63b165102676ece361dd598659551c5b0d0ba4/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js [modify] https://crrev.com/9a63b165102676ece361dd598659551c5b0d0ba4/third_party/blink/renderer/core/fetch/request.cc
,
Jul 4
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by yhirano@chromium.org
, Apr 26 2018