1206166 - Move `FetchUtil::Consume*` methods into a `BodyUtil` class

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler (ex-Mozilla)]

Bug 1149195 extracted body parsing methods into the `FetchUtil` class so that Fetch and Push could use them. Since they're shared, `FetchUtil` seems like a misnomer; maybe `BodyUtil` or another neutral name is appropriate?

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Sounds like a good first bug.

Comment 2

[Huma Zafar]

I'd like to work on this.

Comment 3

[Lina Butler (ex-Mozilla)]

Awesome; glad you're interested! I think the approach here is to add a `BodyUtil` class to `dom/base`, and move the `Consume*` static methods out of `FetchUtil`. Next, you'll want to add the new files to the `EXPORTS.mozilla.dom` and `UNIFIED_SOURCES` sections in `dom/base/moz.build`.

Once you've migrated the methods, you can update `fetch/Fetch.cpp` and `workers/ServiceWorkerEvents.cpp` to use the new class (IIRC, those are the only two callers).

Comment 4

[Huma Zafar]

Attachment: Moved FetchUtil::Consume methods into separate BodyUtil class and updated Fetch.cpp and ServiceWorkerEvents.cpp accordingly.

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8669420 [details] [diff] [review]
Moved FetchUtil::Consume methods into separate BodyUtil class and updated Fetch.cpp and ServiceWorkerEvents.cpp accordingly.

Review of attachment 8669420 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few nits, but looks good! The next step is to test out your change using the Try server (https://wiki.mozilla.org/ReleaseEngineering/TryServer). I can push it for you, but it's handy to have access if you want to work on other bugs in the future. If you'd like an account, please file a bug for Level 1 commit access (https://www.mozilla.org/en-US/about/governance/policies/commit/) and needinfo? me; I'll vouch for you.

Thanks again for the patch!

::: dom/base/BodyUtil.cpp
@@ +1,1 @@
> +#include "BodyUtil.h"

Please add a license header to the top of this file (and `BodyUtil.h`): https://www.mozilla.org/en-US/MPL/headers/

@@ +2,5 @@
> +
> +#include "nsError.h"
> +#include "nsString.h"
> +#include "nsFormData.h"
> +#include "nsIDocument.h"

I think you can drop a lot of these...just from this section, `nsIDocument`, `nsIStreamLoader`, `nsIThreadRetargetableRequest`, and `nsIUnicodeEncoder` appear to be unused.

::: dom/fetch/Fetch.cpp
@@ +19,5 @@
>  #include "nsReadableUtils.h"
>  #include "nsStreamUtils.h"
>  #include "nsStringStream.h"
>  
> +#include "mozilla/dom/BodyUtil.h"

Nit: Please move this below `mozilla/ErrorResult.h`, so it's in alphabetical order. Ditto for `ServiceWorkerEvents.cpp`.

Comment 6

[Josh Matthews [:jdm]]

Huma, are you planning to address the final comments here?

Comment 7

[Huma Zafar]

Attachment: 0001-Bug-1206166-Move-FetchUtil-Consume-methods-into-a-BodyUtil-class.patch

My apologies; new patch attached to address the latest comments. Some other updates were required in order to compile with the latest code, in particular dom/push/PushNotifier.cpp has also been modified to use BodyUtil in place of FetchUtil.

Comment 8

[Lina Butler (ex-Mozilla)]

Comment on attachment 8723384 [details] [diff] [review]
0001-Bug-1206166-Move-FetchUtil-Consume-methods-into-a-BodyUtil-class.patch

Review of attachment 8723384 [details] [diff] [review]:
-----------------------------------------------------------------

Gah, I'm so sorry for letting this slide. This looks good, Huma. Thanks for picking it up again!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5061c2d3f9c

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faaf448b9788

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/faaf448b9788