Closed Bug 1334104 Opened 5 years ago Closed 5 years ago

'if' statement is unnecessary; deleting null pointer has no effect

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: Sylvestre, Assigned: cliffplaysdrums, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

59 bytes, text/x-review-board-request
benjamin
: review+
Sylvestre
: feedback+
Details
This is a very easy good first bug for a beginner.

The following if declarations are useless as delete foo; doesn't have any effect if foo is null.

https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp?q=nsPluginHost.cpp&redirect_type=direct#3138
https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp?q=nsPluginHost.cpp&redirect_type=direct#3153

We should just remove the if
Hi Sylvestre. I'm a beginner and I thought this would be a good experience for me to try and have a crack at this. Please let me know if it's still available!
i'm working on it i'm a beginner too.
I will assign it to the first submitting a patch.
Fyi, bug 1334265 is also a good first bug for a beginner
Keywords: good-first-bug
Whiteboard: [good first bug][lang=C++] → [lang=C++]
Comment on attachment 8831582 [details]
Bug 1334104 - 'if' statement is unnecessary

https://reviewboard.mozilla.org/r/108114/#review109192

::: dom/plugins/base/nsPluginHost.cpp:3151
(Diff revision 1)
>        version,
>        (const char* const*)mimetypes,
>        (const char* const*)mimedescriptions,
>        (const char* const*)extensions,
>        mimetypecount, lastmod, fromExtension, true);
> -    if (heapalloced)
> +

You should remove this line

::: dom/plugins/base/nsPluginHost.cpp:3152
(Diff revision 1)
>        (const char* const*)mimetypes,
>        (const char* const*)mimedescriptions,
>        (const char* const*)extensions,
>        mimetypecount, lastmod, fromExtension, true);
> -    if (heapalloced)
> +
>        delete [] heapalloced;

The indentation level is incorrect here
Jason, Muhammed, if you want another good first bug to start with, please send me an email and I will create and assign one for you.
Assignee: nobody → cliffplaysdrums
Sylvestre, could you let me know if I properly updated my patch? I see there are now 2 commits. Is there a way I could have just updated my first commit, or is this right?
Attachment #8831898 - Attachment is obsolete: true
(In reply to cliffplaysdrums from comment #8)
> Sylvestre, could you let me know if I properly updated my patch? I see there
> are now 2 commits. Is there a way I could have just updated my first commit,
> or is this right?

Note: I got some help and used histedit to combine the patches. Hopefully it all turned out okay.
Comment on attachment 8831582 [details]
Bug 1334104 - 'if' statement is unnecessary

Good, now, you have to find a reviewer for this:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed
Attachment #8831582 - Flags: feedback+
(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Comment on attachment 8831582 [details]
> Bug 1334104 - 'if' statement is unnecessary
> 
> Good, now, you have to find a reviewer for this:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Introduction#Step_4_-_Get_your_code_reviewed

Are you the right person to ask for a review, or is it someone else?
(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> Jason, Muhammed, if you want another good first bug to start with, please
> send me an email and I will create and assign one for you.

Hi, apologies for the late response if there still is another GFB you could point me to I would like to try tackling it. Thanks again!
(In reply to cliffplaysdrums from comment #12)
> Are you the right person to ask for a review, or is it someone else?
Should be someone else :)
(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> (In reply to cliffplaysdrums from comment #12)
> > Are you the right person to ask for a review, or is it someone else?
> Should be someone else :)

I see the name "Benjamin" frequently around this area of the code. I'm guessing this could be the triage owner, but I'm having trouble finding out what a triage owner is, and in this case, the triage owner is ":bsmedberg" and not "Benjamin" so I don't know if that's a coincidence or a display setting. How do you suggest finding the right reviewer? The first step *does say* to ask your mentor if it's a mentored bug :)
this is the same person :)
Just ask to bsmedberg
and you should be fine
Attachment #8831582 - Flags: review?(benjamin)
Comment on attachment 8831582 [details]
Bug 1334104 - 'if' statement is unnecessary

https://reviewboard.mozilla.org/r/108114/#review113806

Look good to me! thank you. I'll trigger a try build and then autoland if it works.
Attachment #8831582 - Flags: review?(benjamin) → review+
NI?myself to keep this on my radar to check try results.
Flags: needinfo?(benjamin)
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fe9a4b7b037
'if' statement is unnecessary r=bsmedberg
Flags: needinfo?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/1fe9a4b7b037
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.