Closed Bug 1206166 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox43 --- affected
firefox47 --- fixed

People

(Reporter: Lina, Assigned: zafar.huma, Mentored)

References

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 1 obsolete file)

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?
Sounds like a good first bug.
Mentor: kcambridge
Whiteboard: [good first bug][lang=c++]
I'd like to work on this.
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).
Assignee: nobody → zafar.huma
Status: NEW → ASSIGNED
Depends on: 1209361
Attachment #8669420 - Flags: review?(kcambridge)
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`.
Attachment #8669420 - Flags: review?(kcambridge)
Huma, are you planning to address the final comments here?
Flags: needinfo?(zafar.huma)
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.
Attachment #8669420 - Attachment is obsolete: true
Flags: needinfo?(zafar.huma)
Attachment #8723384 - Flags: review?(kcambridge)
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
Attachment #8723384 - Flags: review?(kcambridge) → review+
https://hg.mozilla.org/mozilla-central/rev/faaf448b9788
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.