Closed
Bug 1024276
Opened 10 years ago
Closed 10 years ago
[RTSP] Follow-up of 1021980 - Replace multiple character literals by numeric literals
Categories
(Firefox OS Graveyard :: RTSP, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S5 (26sep)
People
(Reporter: ethan, Assigned: jhao)
Details
Attachments
(1 file, 2 obsolete files)
44.92 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → ettseng
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8491382 -
Flags: feedback?(ettseng)
Reporter | ||
Comment 2•10 years ago
|
||
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.)
Reporter | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Assignee: ettseng → jhao
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8492014 -
Flags: review?(sworkman)
Attachment #8492014 -
Flags: feedback?(ettseng)
Assignee | ||
Updated•10 years ago
|
Attachment #8491382 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8492014 -
Flags: review?(sworkman)
Attachment #8492014 -
Flags: feedback?(ettseng)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8492014 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 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.
Description
•