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

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: cliffplaysdrums, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [lang=C++])

Attachments

(1 attachment, 1 obsolete attachment)

59 bytes, text/x-review-board-request
benjamin
: review+
sylvestre
: feedback+
Details
Reporter

Description

2 years ago
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

Comment 1

2 years ago
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!

Comment 2

2 years ago
i'm working on it i'm a beginner too.
Reporter

Comment 3

2 years ago
I will assign it to the first submitting a patch.
Fyi, bug 1334265 is also a good first bug for a beginner
Reporter

Updated

2 years ago
Keywords: good-first-bug
Whiteboard: [good first bug][lang=C++] → [lang=C++]
Comment hidden (mozreview-request)
Reporter

Comment 5

2 years ago
mozreview-review
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
Reporter

Comment 6

2 years ago
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.
Reporter

Updated

2 years ago
Assignee: nobody → cliffplaysdrums
Comment hidden (mozreview-request)
Assignee

Comment 8

2 years ago
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?
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8831898 - Attachment is obsolete: true
Assignee

Comment 10

2 years ago
(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.
Reporter

Comment 11

2 years ago
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+
Assignee

Comment 12

2 years ago
(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?

Comment 13

2 years ago
(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!
Reporter

Comment 14

2 years ago
(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 :)
Assignee

Comment 15

2 years ago
(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 :)
Reporter

Comment 16

2 years ago
this is the same person :)
Just ask to bsmedberg
and you should be fine
Assignee

Updated

2 years ago
Attachment #8831582 - Flags: review?(benjamin)

Comment 17

2 years ago
mozreview-review
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)

Comment 19

2 years ago
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fe9a4b7b037
'if' statement is unnecessary r=bsmedberg

Updated

2 years ago
Flags: needinfo?(benjamin)

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fe9a4b7b037
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.