Closed Bug 1437893 Opened 3 years ago Closed 3 years ago

cdm-test-decryptor.cpp: Remove an unnecessary string initialization

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Sylvestre, Assigned: vprabhu, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [good first bug][lang=C++])

Attachments

(1 file)

https://dxr.mozilla.org/mozilla-central/source/dom/media/fake-cdm/cdm-test-decryptor.cpp?q=cdm-test-decryptor.cpp&redirect_type=direct#429

Found by http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-init.html
(only occurrence left in our code base).

The checker will be enabled at review phase.

A trivial bug, marking it as a good first bug to learn about our workflows.
I'd like to take this on as a first bug.
I would like to work on this bug as my first task.
If you're interested in working on this bug, please assign it to yourself and post a patch for comments. Whoever takes it first can have a first go, but feel free to work together.
Priority: -- → P3
Can anyone tell me -
how to clone this repository?
how to submit a patch?

I am a beginner.
Thank you
If you're interested in getting started on Firefox development here is a good place to start: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Can someone tell me how I can run the test to check if this bug has been fixed? I ran the './mach xpcshell-test services/common/tests/unit/' but it fails 1 test even without my changes.

(newbie here)
I don't think you need to run the tests, this should be transparent!
I am done with the changes....can anyone tell me how to submit it as a patch?
Thank you
Sir, can u please list the steps for a first patch contribution, there are lots of documentation but I am finding them a bit confusing. It will be of great help. Thank you
The exercise here is exactly about that: learning how to read documentations and use it.
Just use the Bugzilla method explained in the link from comment #9. It should be enough!
(In reply to Venkatesh Prabhu :vprabhu from comment #12)
> Created attachment 8954339 [details]
> Bug 1437893 - Removed an unnecessary string initialization in
> cdm-test-decryptor.cpp
> 
> Review commit: https://reviewboard.mozilla.org/r/223442/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/223442/

I have made the change. Please let me know if that is what was needed.
Comment on attachment 8954339 [details]
Bug 1437893 - Removed an unnecessary string initialization in cdm-test-decryptor.cpp

https://reviewboard.mozilla.org/r/223442/#review229540

::: dom/media/fake-cdm/cdm-test-decryptor.cpp:429
(Diff revision 1)
>    ShutdownTimeout,
>    ShutdownStoreToken
>  };
>  
>  static ShutdownMode sShutdownMode = ShutdownNormal;
> -static string sShutdownToken = "";
> +static string sShutdownToken ;

No spaces before the ";"
(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Comment on attachment 8954339 [details]
> Bug 1437893 - Removed an unnecessary string initialization in
> cdm-test-decryptor.cpp
> 
> https://reviewboard.mozilla.org/r/223442/#review229540
> 
> ::: dom/media/fake-cdm/cdm-test-decryptor.cpp:429
> (Diff revision 1)
> >    ShutdownTimeout,
> >    ShutdownStoreToken
> >  };
> >  
> >  static ShutdownMode sShutdownMode = ShutdownNormal;
> > -static string sShutdownToken = "";
> > +static string sShutdownToken ;
> 
> No spaces before the ";"

Done. Sorry about that. I thought of that after pushing the changes for review.
Assignee: nobody → venkateshprabhu2
No worries! 
Now, ask for a review to cpearce (the code owner)
Attachment #8954339 - Flags: review?(cpearce)
Comment on attachment 8954339 [details]
Bug 1437893 - Removed an unnecessary string initialization in cdm-test-decryptor.cpp

https://reviewboard.mozilla.org/r/223442/#review229674

Thank you for the patch!
Attachment #8954339 - Flags: review?(cpearce) → review+
Venkatesh: If you can mark the issue about the extra space that Sylvestre pointed out as resolved in MozReview, our patch landing system can push this code to our main repository.
(In reply to Chris Pearce (:cpearce) from comment #19)
> Venkatesh: If you can mark the issue about the extra space that Sylvestre
> pointed out as resolved in MozReview, our patch landing system can push this
> code to our main repository.

Done.
And I just landed it for you. Should be in Firefox 60.
Bravo!
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ce5b5962798
Removed an unnecessary string initialization in cdm-test-decryptor.cpp r=cpearce
https://hg.mozilla.org/mozilla-central/rev/7ce5b5962798
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.