WEBVTT: Unit tests, unit tests...
TLDR: I made some progress on the Gecko code for the WEBVTT parser integration, but was unable to finish because I was moved over to finishing the WEBVTT parser exclusively as we needed to move fast on it. This resulted in unit test fixing galore.
I haven't written in a quite a while so this is going to be a pretty long post.
I was working on the Gecko integration stuff before I got pulled away and made some decent progress on that. I got some feedback on my first WIP and I addressed most of the issues that the reviewers pointed out. A lot of it was minor nits like code style or incorrect code that I just needed to remove. However, there were a few big things:
- Getting the
OnDataAvailable()function to use the ReadSegments() function of the stream it is passed. To do this you need to create your own function that conforms to the function prototype of nsWriteSegmentFunc. What that does is it allows the
nsIInputStreamto read in your buffer for you and then you can just hand that buffer over to whatever you need to. This prevents memory leaks as it creates a central location where the memory leak would happen, and presumably, the code in the tree is already written for you and probably better then what you've written—`so just use it.
- Use an nsAutoRefTrait to manage the lifetime of the nsAutoRef<webvtt_parser>. That ended up not being so hard. What an
xTypedoes is define what an
xTypeneeds to do during it's lifetime. In our code, for example, all we need to do is define a
Release()function that tells the
nsAutoRefpointer what to call when it is being released. There are a few more behaviours that you can define like what do do on setup, etc. This is necessary since most things in the Gecko code are all smart pointers and objects, so you don't want to manage their lifetimes explicitly, but you do want to provide a way to define the appropriate steps it needs to do during its lifetime. The awesome thing about this is that once you create an
xTypewill be automatically linked to it.
The other couple of things I did before I submitted my last WIP for feeback was fleshing out a lot of the code that will convert the c structs from the c parser to DOM elements, getting the code that deals with
DocumentFragments working correctly, and a few other minor things. You can check out the full WIP that I submitted for my last review here.
The other thing that I was really trying to do up until this point, and currently, is drive the issue list. I've noticed that the issue list tends to stagnate if no one is there monitoring it. To this point I have been looking over the issue list, about once every other day, and closing issues that need to be closed, i.e. onces that have been resolved, seem to be going nowhere, or seem to have gone as far as they are able to, as well as trying to push issues that seem to have stalled by @mentioning the parties involved, or asking a question about where we should head in light of the issue. This process has been very helpful because it has enabled us to keep our issue list from being cluttered by issues that are no longer needed and to also drive issues that people might have forgotten about.
Parser Unit Test Debugging
About three weeks into working on the Gecko integration as well as the parser I was asked to step back from the Gecko integration for a while and to start working exclusively on getting the parser to pass all the unit tests. To that end, I relinquished my duties on the Gecko integration for a bit. My first job was to help Caitlin get her massive patch split up and landed. To do this she asked me to write a finish parsing function that would allow the patch to be split up more easily. After that was done I started to work on the payload unit tests, as I anticipated Caitlin's cue code to be landing soon. I was able to get around 90 % of the payload unit tests working over reading week, valgrind and all, and solve many bugs in the parser. A lot of the bugs that were effecting the payload tests were also effecting the cue tests, so I was able to hit two birds with one stone in many cases.
The Importance of Passing Unit Tests
I came across a situation in this sprint where I re-discovered the importance of passing unit tests. How it all started was myself fixing a problem we were having with a function that we use to attach a parsed cue text node to another parent node. The function kept segfaulting and it was pretty easy to figure out what had been happening—it was dereferencing a variable before allocating memory for it. To fix this I allocated memory before hand, as well as rearranged the function a bit to simplify it and make it look cleaner. The code got landed and all was good, or so I thought. Later I uncovered that some of the valgrind errors we were getting in another unit tests were actually caused by what I had landed earlier. It was an easy fix, but it just goes to show how important passing unit tests are. It's kind of impossible to tell if the code you write breaks anything when everything is broken to begin with.
The Art of Face Palming
As always this last week's sprint had its fair share of face palming. Over the years I've gotten pretty good at it—you might even call me a face palm artist. This weeks face palm that takes first prize was an error we were getting in the grow function for strings where we were using the
== operator when we really wanted the
= operator. This was in an if statement, no less, so it took me a couple looks and one double-take to realize what was happening. C you upset me so much, I think we should break up.
Reference Counting is Really Nice
This is the first time I've really used reference counting myself, but I have to say it's pretty awesome. I've heard some talk smack about it, but from my small experience working on the WEBVTT parser with it, it makes things so much easier. Especially since a lot of our objects are shared in multiple places in and outside of the parser. The problem that I found reference counting solves is when a project gets so big that is hard to tell where an object is used, especially when they are passed around a lot. So you might have the case where someone decides to delete an object that another piece of code is trying to use and it blows up. This isn't even the worse case. The worse case is when you do this and it doesn't blow up at first—so it's landed. Then later, for what seems like a random reason, it will start blowing up because the use case where the code that you just landed causes the code to blow up is taking place. So now the reason why it is blowing up is not entirely clear at first. Reference counting makes this so much easier because when you are coding you can, in most cases, just assume that the calling function increased the reference count of the object when it passed it to your code, and so at the end of the code you can safely call the release function. No more explicitly managing the objects, your objects know when they need to be deleted. Due to this I've implemented reference counting on both the c nodes and our c string lists. Both of these structs are used extensively, passed around a lot, and are made available externally through the C++ wrappers. So it's nice to have ref counting on them.
What Makes a Good Pull Request
One of the final things that really got reinforced to me over this last week's sprint was the factors that make a good pull request, or patch. A good pull request is kind of like a good unit test. You want it to be focused in what it is trying to accomplish and atomic in scale i.e. it touches as little as possible. That doesn't mean you can't make pull requests that change like a billion lines. What it does mean is that those billion lines are all being changed for the same focused reason. Granted, there are times when these rules cannot be followed, but you should try to follow them in ≈99% of your pull requests.
My Pull Requests are Over 9000 (not really, I'm a nub)
If you'd like to check out the work I've been doing for the past couple of weeks you can look at my list of pull requests.
Next week, March 14, we will be travelling to the Mozilla Toronto Office for our class. Our Professor, Dave Humphrey, wants us to have a demo and presentation ready to show the devs at Mozilla. So we will be sprinting on getting the last pieces of the puzzle for the WEBVTT integration locked in. This is mostly the integration code that I had to leave off on last week. I'm going to be getting a lot of help this time for that so hopefully we will get it done.
Until next time.- Tagged in mozilla, open-source, cdot, and seneca college.
comments powered by Disqus