1297082 - Convert the Curl.jsm module to curl.js, use it in HAR builder

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Jarda Snajdr [:jsnajdr]]

- convert the Curl.jsm module to CommonJS (curl.js)
- start using it in the HAR builder, remove duplicate code (getHeadersFromMultipartText, isUrlEncodedRequest)

This is one of changes I did for bug 1176050 (Netmonitor performance optimizations), but it's uncertain if that patch will ever land - it failed to deliver the promised performance improvements so far. Therefore, I want to land some valuable parts of it separately.

Comment 1

[Jarda Snajdr [:jsnajdr]]

Attachment: Bug 1297082 - Part 1: Convert Curl.jsm to a CommonJS module

Review commit: https://reviewboard.mozilla.org/r/73552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73552/

Comment 2

[Jarda Snajdr [:jsnajdr]]

Attachment: Bug 1297082 - Part 2: Use curl.js utility functions in HAR builder

Review commit: https://reviewboard.mozilla.org/r/73554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73554/

Comment 3

[Jarda Snajdr [:jsnajdr]]

Attachment: Bug 1297082 - Part 3: Fix eslint issues in curl.js

Review commit: https://reviewboard.mozilla.org/r/73556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73556/

Comment 4

[Jarda Snajdr [:jsnajdr]]

Part 1:
- rename Curl.jsm to curl.js and fix the imports

Part 2:
- use CurlUtils in har-builder.js, remove duplicate functions
- using CurlUtils.getHeadersFromMultipartText to extract upload stream headers fixes issue discussed in bug 1278923 (see [1])

Part 3:
- make the curl.js file eslint-clean
 
You need to apply this patch on top of the fix from bug 1297068 (multipart boundaries)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1278923#c17

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8783897 [details]
Bug 1297082 - Part 1: Convert Curl.jsm to a CommonJS module

https://reviewboard.mozilla.org/r/73552/#review72030

LGTM

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Btw. I assume there are no changes in the curl.js file, correct?
Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8783898 [details]
Bug 1297082 - Part 2: Use curl.js utility functions in HAR builder

https://reviewboard.mozilla.org/r/73554/#review72032

R+ assuming try is green.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8783899 [details]
Bug 1297082 - Part 3: Fix eslint issues in curl.js

https://reviewboard.mozilla.org/r/73556/#review72034

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf02a86c242d
Part 1: Convert Curl.jsm to a CommonJS module r=Honza
https://hg.mozilla.org/integration/autoland/rev/47874267eb44
Part 2: Use curl.js utility functions in HAR builder r=Honza
https://hg.mozilla.org/integration/autoland/rev/4dd3a3052ef0
Part 3: Fix eslint issues in curl.js r=Honza

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf02a86c242d
https://hg.mozilla.org/mozilla-central/rev/47874267eb44
https://hg.mozilla.org/mozilla-central/rev/4dd3a3052ef0