Closed Bug 1089705 Opened 5 years ago Closed 5 years ago

Replace "assert" with "MOZ_ASSERT" in /media/gmp-clearkey/0.1/ and /dom/media/gmp-plugin/

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dholbert, Assigned: givanica, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

It looks like some GMP code uses "assert()" instead of MOZ_ASSERT for some reason. (ClearKeyUtils.cpp actually uses a mix of the two.)

We should consistently use MOZ_ASSERT in this code, unless there's a strong reason to avoid it.

(At first I thought maybe the code was intended for usage outside of our build system, but I odn't think that's the case, because it uses other MFBT headers.)
cpearce, are you ok with this change? (or, is there some reason we've opted for "assert()" here?)

If you don't object, maybe we can make this a mentored bug and let a new contributor do the conversion.
Flags: needinfo?(cpearce)
(In reply to Daniel Holbert [:dholbert] from comment #1)
> cpearce, are you ok with this change? (or, is there some reason we've opted
> for "assert()" here?)
> 
> If you don't object, maybe we can make this a mentored bug and let a new
> contributor do the conversion.

I was originally trying to keep the code as independent of gecko stuff as possible, but I guess it shouldn't matter in this instance. +1 on mentored bug.
Cool, thanks. Marking as mentored.

To whoever wants to take this mentored bug: we need to replace "assert(" with "MOZ_ASSERT(" in these files:
http://mxr.mozilla.org/mozilla-central/source/dom/media/gmp-plugin/gmp-test-decryptor.cpp
http://mxr.mozilla.org/mozilla-central/source/dom/media/gmp-plugin/gmp-test-storage.cpp
http://mxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/ClearKeyUtils.cpp
http://mxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp

You'll also need to add
  #include "mozilla/Assertions.h"
to each of those files (in alphabetical order, ideally).

If you haven't written a patch before, this should help you get off the ground with compiling Firefox (which you'll need to do to be sure your patch doesn't break something):
 https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

And, here's some information about what needs to be included in your patch file:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Thanks in advance!
Mentor: dholbert
Flags: needinfo?(cpearce)
Whiteboard: [lang=c++]
(by "in alphabetical order, ideally" I mean "add the new #include at the appropriate alphabetical place in the file's existing #include list".)
Hi, Can I be assigned to this bug?
I know what I have to do (+1 for this great info). Haven't submitted any patch till now but I know how to build Firefox. I can send a patch probably by tomorrow if anything works as expected.
Thanks! Assigned you. Glad to hear that you can already build Firefox. For generating the patch, make sure to see the last link in comment 3, to be sure your patch has your email address & commit message in the patch headers.
Assignee: nobody → gabriel.ivanica
edwin or I would both be fine as reviewers for this code, too, so feel free to tag either of us as the reviewer. (I haven't written any code in this module, but given comment 2, I'm comfortable r+'ing a patch for a straightforward bug like this.)
Any particular reason why http://mxr.mozilla.org/mozilla-central/source/dom/media/gmp-plugin/gmp-fake.cpp isn't listed in comment 3 ? It does include assert() code.
Actually, I take that back -- it looks like gmp-fake is largely/entirely taken from an upstream repo. (and doesn't have any mozilla-specific #includes, as far as I can tell)

So, probably better to leave that one untouched.
(I probably missed that one in comment 3 because I'd searched for "assert(", and this one has "assert (" with a space before the open-paren. But anyway, per comment 10, I'd lean towards leaving that one untouched.)
Attached patch Patch for Bug 1089705 (obsolete) — Splinter Review
Attachment #8513063 - Flags: review?(dholbert)
Should the patch commit contain the "r=reviewer" part ? Or that's just for commits through HG. 
Also, is there a naming convention for uploaded patch filenames ?

This one is pretty straightforward... I guess that uploading patches usually means that I tested out everything works and run the provided tests no ?
Comment on attachment 8513063 [details] [diff] [review]
Patch for Bug 1089705

Looks great! Just one nit:

>Bug 1089705 - Replaced "assert" with "MOZ_ASSERT"; r=dholbert

Please add "in some media code" or something like that to the commit message. (otherwise, it sounds like you're making this change across the whole tree.)

r=me with that.  Do you have Try access? If not, I can give this a TryServer run for you, to make sure it builds successfully everywhere before we add "checkin-needed".
Attachment #8513063 - Flags: review?(dholbert) → review+
(In reply to Gabriel Ivanica [:givanica] from comment #13)
> Should the patch commit contain the "r=reviewer" part ?

Yes.

> Or that's just for commits through HG.

(A patch turns into a hg commit, so the commit message in the patch = the commit message that ends up being shown in hg.)

> Also, is there a naming convention for uploaded patch filenames ?

Nope. I usually use filenames like "1089705-fix.patch", but it doesn't really matter; the filename doesn't persist in hg.

> This one is pretty straightforward... I guess that uploading patches usually
> means that I tested out everything works and run the provided tests no ?

Yup, usually you'll want to make sure the patch works before posting it & asking for review. (I don't think test-runs are required here.)  We should get this through TryServer (https://wiki.mozilla.org/ReleaseEngineering/TryServer ) before we get it landed, though, to prove that it builds successfully on a handful of platforms.  Again, I'm happy to do that for you; once you've fixed a few bugs, you'll want to get Try access yourself.
Attached patch 212757.patchSplinter Review
Attachment #8513063 - Attachment is obsolete: true
Attachment #8513085 - Flags: review?(dholbert)
...fighting a little bit with HG since I only know Git and SVN.
I don't have a TryServer account so please send the test

> r=me with that.  
If you don't mind what exactly did you mean by this?
(In reply to Gabriel Ivanica [:givanica] from comment #17)
> ...fighting a little bit with HG since I only know Git and SVN.

BTW, if you really *really* like git, I'm told there are tools/workflows to make it easier to hack on Gecko code using only git. (google & irc.mozilla.org #developers / #git are probably your friend there, if you want to pursue that; I'm a mercurial guy, myself.)

> I don't have a TryServer account so please send the test

Cool. Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e2c90ab6ed2c

If all of the gray builds there turn green (probably in several hours, or tomorrow if you're asleep at that point), please go ahead and add "checkin-needed" to the keywords field here, and someone will come along and land the patch for you.

Normally the Try run would include builds + some unit tests, but this bug is the sort of change where you know it's correct if it compiles.  (since it's not actually changing any logic)

> > r=me with that.  
> If you don't mind what exactly did you mean by this?

"r=me" means "I grant review on this patch".

"r=me with [stuff addressed] means "please re-upload the patch, with this minor thing fixed, and grant yourself the "review+" flag (in the attachment-upload page), and say "carrying forward r=dholbert".

(Since you already re-uploaded the patch, I'll just explicitly mark the new version as r+)
Comment on attachment 8513085 [details] [diff] [review]
212757.patch

Thanks again for the patch! In all likelihood, we should be all set here. (pending the TryServer results)
Attachment #8513085 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/80356dd0012d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.