HttpChannelParent::OnStartRequest deep-copies nsHttpHeaderArray

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mayhemer, Assigned: jaas)

Tracking

Trunk
mozilla12
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

17.18 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
Reporter

Description

8 years ago
See [1].  I don't think this is made purposely.  Just a typo - forgotten & at the declaration.  We might get a small perf/memory gain here.

As part of this bug, I'm curious why we use RequestHeaderTuples (that we have to copy the headers into) instead of nsHttpHeaderArray directly for which both we have ParamTraits defined.  But maybe I just forgot why we don't.

Fixing that is related to how/when bug 597706 and bug 646507 gets fixed.

[1] http://hg.mozilla.org/mozilla-central/diff/3a794e272196/netwerk/protocol/http/HttpChannelParent.cpp

Comment 1

8 years ago
You're right--we could use a reference to avoid a copy here.

And actually we could pass nsHttpHeaderArray directly to IPDL here, as you suggest.  We use the Tuple class when going from the child->parent, in order to set 'merge' correctly (the idea being that it's generally cheaper to send a log of the SetHeader calls the necko client has made, rather than copy the entire header array including default headers that the parent creates for us anyway).  But we don't need it in the parent->child direction.
Reporter

Updated

8 years ago
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Reporter

Comment 2

8 years ago
Good first bug...
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Assignee

Updated

8 years ago
Assignee: nobody → joshmoz
Assignee

Comment 3

8 years ago
Posted patch fix v1.0Splinter Review
Assignee

Updated

8 years ago
Attachment #589661 - Flags: review?(jduell.mcbugs)
Assignee

Comment 4

8 years ago
Fix v1.0 passes on try server.

Comment 5

8 years ago
Comment on attachment 589661 [details] [diff] [review]
fix v1.0

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

Very nice!  Thanks.  I'm adding a comment to nsIHttpChannelChild.idl describing the clientSetHeaders field, and then I'll land it.
Attachment #589661 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/a6e3185f8aa9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.