Closed Bug 1024276 Opened 6 years ago Closed 5 years ago

[RTSP] Follow-up of 1021980 - Replace multiple character literals by numeric literals

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S5 (26sep)

People

(Reporter: ethan, Assigned: jhao)

Details

Attachments

(1 file, 2 obsolete files)

Assignee: nobody → ettseng
Attached patch bug-1024276-fix.patch (obsolete) — Splinter Review
Attachment #8491382 - Flags: feedback?(ettseng)
Comment on attachment 8491382 [details] [diff] [review]
bug-1024276-fix.patch

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

This patch looks good to me.
Some minor suggestions are made inline.

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +82,5 @@
>      }
>  }
>  
>  struct RtspConnectionHandler : public AHandler {
>      enum {

This enumeration is outstanding in its number of constants and the
complexity of messages constructed using these constants.
Furthermore, some of them are used by RTSPSource.
I suggest to start the constant from a value other than 1, such as 1000.
That might be easier to distinguish these constants while debugging.

@@ +90,5 @@
> +        kWhatSetup,
> +        kWhatPerformPause,
> +        kWhatPerformSeek,
> +        kWhatPerformPlay,
> +        kWhatPerformResume,

I suggest to remove the word "Perform" in these four enumerators.
In that way we won't be confused with the constants defined in RTSPSource.h
(kWhatPerformPlay is only used in RTSPSource; and kWhatPlay is only used 
in RtspConnectionHandler.)
Comment on attachment 8491382 [details] [diff] [review]
bug-1024276-fix.patch

Hi Steve,
Could you help to review this patch?

BTW, Jonathan Hao is our new team member. ;)
Attachment #8491382 - Flags: review?(sworkman)
Attachment #8491382 - Flags: feedback?(ettseng)
Attachment #8491382 - Flags: feedback+
Comment on attachment 8491382 [details] [diff] [review]
bug-1024276-fix.patch

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

Welcome Jonathan! Good to have you in Mozilla!

I agree with Ethan's comments. Otherwise, r=me.
Attachment #8491382 - Flags: review?(sworkman) → review+
Assignee: ettseng → jhao
Attachment #8492014 - Flags: review?(sworkman)
Attachment #8492014 - Flags: feedback?(ettseng)
Attachment #8491382 - Attachment is obsolete: true
Attachment #8492014 - Flags: review?(sworkman)
Attachment #8492014 - Flags: feedback?(ettseng)
Attachment #8492014 - Attachment is obsolete: true
(In reply to Jonathan Hao [:jhao] from comment #7)
> https://tbpl.mozilla.org/?tree=Try&rev=6c700f5fd345

Try Server build (25f3d2bce417) was successfully completed except on "Mulet Linux x64 Opt", which seems to be a problematic new platform and makes every patch's build fail.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b291e243a8e3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
You need to log in before you can comment on or make changes to this bug.