Closed Bug 1273712 Opened 3 years ago Closed 3 years ago

Clean up Base64.{h,cpp}

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(2 files)

I want to add a variant of Base64Encode() that will help fix bug 1273711. I'll also do some clean-up while I'm here.
Blocks: 1273711
The argument naming in Base64.{h,cpp} is horribly confused, with a lot of them
gotten backwards. This patch fixes that, and also introduces a more consistent
naming scheme for arguments and local variables: "binary" is used for binary
data, and "base64" is used for base64-encoded data.

This patch doesn't change any functionality.
Attachment #8753611 - Flags: review?(erahm)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
This will be used in bug 1273711 to avoid an OOM.

This also tweaks one of the existing overloadings of Base64Encode to return
NS_ERROR_OUT_OF_MEMORY on OOM instead of NS_ERROR_INVALID_ARG.
Attachment #8753616 - Flags: review?(erahm)
Comment on attachment 8753611 [details] [diff] [review]
(part 1) - Rename arguments and variables in Base64.{h,cpp}

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

Thanks for cleaning this up, it's much easier to follow now.

::: xpcom/io/Base64.cpp
@@ -320,1 @@
>  {

Everything about this function makes me sad.
Attachment #8753611 - Flags: review?(erahm) → review+
Comment on attachment 8753616 [details] [diff] [review]
(part 2) - Add a new overloading of Base64Encode()

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

Looks good, one suggestion.

::: xpcom/io/Base64.h
@@ +24,5 @@
>                          uint32_t aCount,
>                          uint32_t aOffset = 0);
>  
>  nsresult
> +Base64Encode(const char* aBinary, uint32_t aBinaryLen, char** aBase64);

Seems like we'd want to mark this as MUST_USE_RESULT or whatever it's called (probably the rest too, but that could be a follow up).
Attachment #8753616 - Flags: review?(erahm) → review+
> Seems like we'd want to mark this as MUST_USE_RESULT or whatever it's called
> (probably the rest too, but that could be a follow up).

I tried that and there were missing checks. So, yes, follow-up! And thank you for reminding me to revisit that.
https://hg.mozilla.org/mozilla-central/rev/1e1e0b415418
https://hg.mozilla.org/mozilla-central/rev/02dea57c6c0f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1240197
You need to log in before you can comment on or make changes to this bug.