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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

The reasons and details of this bug can be referred to in:
https://bugzilla.mozilla.org/show_bug.cgi?id=1021890#c4
https://bugzilla.mozilla.org/show_bug.cgi?id=1021890#c6

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Attachment: bug-1024276-fix.patch

Comment 2

[Ethan Tseng [:ethan]]

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 3

[Ethan Tseng [:ethan]]

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. ;)

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 5

[Jonathan Hao (inactive) [:jhao]]

Attachment: fixed according to Ethan's suggestions

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Attachment: fixed according to Ethan's suggestions

Comment 7

[Jonathan Hao (inactive) [:jhao]]

https://tbpl.mozilla.org/?tree=Try&rev=6c700f5fd345

Comment 8

[Jonathan Hao (inactive) [:jhao]]

(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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b291e243a8e3

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b291e243a8e3