Refactoring RequestInit to use a Builder Pattern by lucasfantacuci · Pull Request #22521 · servo/servo

lucasfantacuci · 2018-12-21T11:07:37Z

If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Use builder pattern with RequestInit #22427
These changes do not require tests because it is a code refactoring.

This change is

highfive · 2018-12-21T11:07:43Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

highfive · 2018-12-21T11:07:44Z

Heads up! This PR modifies the following files:

@KiChjang: components/net_traits/request.rs

lucasfantacuci · 2018-12-21T18:11:20Z

~~I implemented the builder pattern as a non-consuming builder. ref: https://doc.rust-lang.org/1.0.0/style/ownership/builders.html~~

components/net_traits/request.rs

jdm

Sorry about the delay! Thanks for making these changes; they have made clear some interesting holes in our current network requests that I plan to file issues about!

components/net_traits/request.rs

jdm · 2019-01-15T18:35:19Z

components/script/fetch.rs

-        cache_mode: request.cache_mode,
-        ..NetTraitsRequestInit::default()
-    }
+    NetTraitsRequestInit::new(request.url())


I would actually like to revert the change to this function, and remove the use of ..NetTraitsRequesInit::default(). That will make the compiler catch when we add members to NetTraitsRequestInit and need to be sure they are initialized correctly.

Sorry, I didn't get you.

I would like to revert this particular change: this method should construct a new NetTraitsRequestInit object with an explicit structvliteral that initializes every field (instead of using ..NetTraitsRequestInit::default()).

What do you think about a method overload to the method to the Builder new passing all the arguments or the Request Object?

I'm just thinking about the separation of concerns.

What do you think now?

Then we will be able to construct this object without setting the properties directly by removing the "pub" from them.

e.g.:

RequestBuilder { url: url ... }

It will throw an error if you are building outside of his scope.

The advantage of allowing some code like request_init_from_request to create the RequestInit value directly is that it's easier to understand code like use_url_credentials: false instead of a false value that is one of 20 arguments. I think it's fine to keep the field values of RequestInit public and build it directly, rather than adding a method.

yeah, i've got you.

jdm · 2019-01-15T18:38:16Z

error: unused import: `std::default::Default`
  --> components/net_traits/request.rs:11:5
   |
11 | use std::default::Default;
   |     ^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

lucasfantacuci · 2019-01-15T18:43:46Z

My apologies, I was away. I gonna retake this task and finish it.

bors-servo · 2019-04-03T10:38:09Z

☔ The latest upstream changes (presumably #23103) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2019-04-05T00:12:14Z

@lucasfantacuci: 🔑 Insufficient privileges: not in try users

bors-servo · 2019-04-05T00:12:40Z

@lucasfantacuci: 🔑 Insufficient privileges: not in try users

…nit, r=jdm Refactoring RequestInit to use a Builder Pattern  If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22427 - [x] These changes do not require tests because it is a code refactoring.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)

jdm · 2019-04-10T14:58:25Z

@bors-servo retry

bors-servo · 2019-04-10T14:58:29Z

⌛ Testing commit c3803b0 with merge 5370151...

…nit, r=jdm Refactoring RequestInit to use a Builder Pattern  If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22427 - [x] These changes do not require tests because it is a code refactoring.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)

KiChjang · 2019-04-10T15:48:43Z

components/script/fetch.rs

 use ipc_channel::router::ROUTER;
 use js::jsapi::JSAutoCompartment;
-use net_traits::request::RequestInit as NetTraitsRequestInit;
+use net_traits::request::RequestBuilder as NetTraitsRequestInit;


This probably doesn't need the renaming anymore.

KiChjang · 2019-04-10T15:49:10Z

components/script/fetch.rs

        use_cors_preflight: request.use_cors_preflight,
        credentials_mode: request.credentials_mode,
-        use_url_credentials: request.use_url_credentials,
+        use_url_credentials: false,


Why is this changed to false?

It may be a lack of attention.

The default value is false.

KiChjang · 2019-04-10T17:36:00Z

@bors-servo r=jdm,KiChjang

Thanks!

bors-servo · 2019-04-10T17:36:02Z

📌 Commit 6b2be9b has been approved by jdm,KiChjang

bors-servo · 2019-04-10T17:36:08Z

⌛ Testing commit 6b2be9b with merge cb5dce5...

…nit, r=jdm,KiChjang Refactoring RequestInit to use a Builder Pattern  If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22427 - [x] These changes do not require tests because it is a code refactoring.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)

bors-servo · 2019-04-11T03:36:08Z

💥 Test timed out

KiChjang · 2019-04-11T05:18:38Z

@bors-servo retry

bors-servo · 2019-04-11T14:31:21Z

⌛ Testing commit 6b2be9b with merge f14d199...

…nit, r=jdm,KiChjang Refactoring RequestInit to use a Builder Pattern  If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22427 - [x] These changes do not require tests because it is a code refactoring.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)

jdm · 2019-04-11T16:58:30Z

@bors-servo retry

bors-servo · 2019-04-11T16:58:36Z

⌛ Testing commit 6b2be9b with merge 9ab0af0...

…nit, r=jdm,KiChjang Refactoring RequestInit to use a Builder Pattern  If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22427 - [x] These changes do not require tests because it is a code refactoring.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)

bors-servo · 2019-04-11T18:33:40Z

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: jdm,KiChjang
Pushing 9ab0af0 to master...

highfive assigned jdm Dec 21, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 21, 2018

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 21, 2018

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 21, 2018

lucasfantacuci force-pushed the use_build_pattern_with_requestinit branch from 579725c to 7a5ef1b Compare December 21, 2018 19:39

lucasfantacuci changed the title ~~WIP: Use builder pattern with RequestInit~~ Refactoring RequestInit to use a Builder Pattern Dec 21, 2018

KiChjang reviewed Dec 22, 2018

View reviewed changes

components/net_traits/request.rs Outdated Show resolved Hide resolved

jdm requested changes Jan 15, 2019

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 15, 2019

highfive added the S-needs-rebase There are merge conflict errors. label Apr 3, 2019

lucasfantacuci force-pushed the use_build_pattern_with_requestinit branch from 7a5ef1b to 1bb9073 Compare April 3, 2019 20:43

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 3, 2019

lucasfantacuci changed the title ~~Refactoring RequestInit to use a Builder Pattern~~ [WIP]Refactoring RequestInit to use a Builder Pattern Apr 4, 2019

lucasfantacuci force-pushed the use_build_pattern_with_requestinit branch 5 times, most recently from f3b6ef9 to 2712351 Compare April 4, 2019 18:29

KiChjang reviewed Apr 10, 2019

View reviewed changes

Implementing the builder pattern for RequestInit

6b2be9b

lucasfantacuci force-pushed the use_build_pattern_with_requestinit branch from c3803b0 to 6b2be9b Compare April 10, 2019 17:02

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 10, 2019

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 10, 2019

bors-servo merged commit 6b2be9b into servo:master Apr 11, 2019

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 11, 2019

This was referenced Apr 11, 2019

Upgrade to SpiderMonkey 66 #23163

Merged

Add referrer to navigation fetch request #23090

Merged

[WIP] Implement parallel CSS parsing #22478

Closed

Use a threadpool for fetch #22769

Merged

lucasfantacuci deleted the use_build_pattern_with_requestinit branch April 11, 2019 23:08

Uh oh!

Conversation

lucasfantacuci commented Dec 21, 2018 • edited by CYBAI Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Dec 21, 2018

Uh oh!

highfive commented Dec 21, 2018

Uh oh!

lucasfantacuci commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasfantacuci Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasfantacuci Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdm Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdm commented Jan 15, 2019

Uh oh!

lucasfantacuci commented Jan 15, 2019

Uh oh!

bors-servo commented Apr 3, 2019

Uh oh!

bors-servo commented Apr 5, 2019

Uh oh!

bors-servo commented Apr 5, 2019

Uh oh!

jdm commented Apr 10, 2019

Uh oh!

bors-servo commented Apr 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasfantacuci Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KiChjang commented Apr 10, 2019

Uh oh!

bors-servo commented Apr 10, 2019

Uh oh!

bors-servo commented Apr 10, 2019

Uh oh!

bors-servo commented Apr 11, 2019

Uh oh!

KiChjang commented Apr 11, 2019

Uh oh!

bors-servo commented Apr 11, 2019

Uh oh!

jdm commented Apr 11, 2019

Uh oh!

bors-servo commented Apr 11, 2019

Uh oh!

bors-servo commented Apr 11, 2019

lucasfantacuci commented Dec 21, 2018 •

edited by CYBAI

Loading

lucasfantacuci commented Dec 21, 2018 •

edited

Loading

lucasfantacuci Apr 4, 2019 •

edited

Loading

lucasfantacuci Apr 5, 2019 •

edited

Loading

jdm Apr 8, 2019 •

edited

Loading

lucasfantacuci Apr 10, 2019 •

edited

Loading