Monthly Archives: January 2012

Bug 698381 – My Work So Far

When compared to bugs 698384 and 698385, this one is very similar. The problems I am finding myself having aren’t to do with how IDL defined optional arguments are handled but following the massive inheritance chain of nsIDOMNode. Compound that with the complexities of a code base like Mozilla and it can be pretty difficult at times really trying to figure out where or what you should be looking for.

Various helpful mxr searches have helped me find where cloneNode actually has been implemented (at least from what I can tell) however it seems to be more complicated than simply updating those methods to the new definition of cloneNode. Currently when compiling I get these errors:


/Users/matthewschranz/Sites/repos/mozilla-central/content/html/content/src/nsHTMLOptionElement.h:70:3: error: 
      too many arguments to function call, expected 2, have 3
  NS_FORWARD_NSIDOMNODE(nsGenericHTMLElement::) 
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../dist/include/nsIDOMNode.h:225:135: note: instantiated from:
  ...* *_retval NS_OUTPARAM) { return _to CloneNode(deep, _argc, _retval); } \
                                                                 ^~~~~~~
/Users/matthewschranz/Sites/repos/mozilla-central/content/base/src/nsGenericElement.h:437:3: note: 
      'CloneNode' declared here
  nsresult CloneNode(bool aDeep, nsIDOMNode **aResult)
  ^
In file included from /Users/matthewschranz/Sites/repos/mozilla-central/js/xpconnect/src/dombindings.cpp:1206:
In file included from ./dombindings_gen.cpp:3:
In file included from /Users/matthewschranz/Sites/repos/mozilla-central/content/html/content/src/nsHTMLSelectElement.h:61:

/Users/matthewschranz/Sites/repos/mozilla-central/content/html/content/src/nsHTMLFormElement.h:109:3: error: 
      too many arguments to function call, expected 2, have 3
  NS_FORWARD_NSIDOMNODE(nsGenericHTMLElement::) 
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../dist/include/nsIDOMNode.h:225:135: note: instantiated from:
  ...* *_retval NS_OUTPARAM) { return _to CloneNode(deep, _argc, _retval); } \
                                                                 ^~~~~~~
/Users/matthewschranz/Sites/repos/mozilla-central/content/base/src/nsGenericElement.h:437:3: note: 
      'CloneNode' declared here
  nsresult CloneNode(bool aDeep, nsIDOMNode **aResult)
  ^
In file included from /Users/matthewschranz/Sites/repos/mozilla-central/js/xpconnect/src/dombindings.cpp:1206:
In file included from ./dombindings_gen.cpp:3:


/Users/matthewschranz/Sites/repos/mozilla-central/content/html/content/src/nsHTMLSelectElement.h:262:3: error: 
      too many arguments to function call, expected 2, have 3
  NS_FORWARD_NSIDOMNODE(nsGenericHTMLFormElement::) 
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../dist/include/nsIDOMNode.h:225:135: note: instantiated from:
  ...* *_retval NS_OUTPARAM) { return _to CloneNode(deep, _argc, _retval); }

The troubles I am having is attempting to figure out what those two forward declarations are calling.

Honestly this is the only thing that’s leaving me stuck at this point. For now I think I’ll go get myself level 1 Try server access so I can finish another ticket.

OSD700 Release 1 – Updating the DOM

First release! Already? Hate to sound not enthused but other courses are bringing down my mood right now 😛 Not this one though!

During these first two and a half weeks I have been able to get three tickets done with two of them landed and the third’s patch review+ and just needed to be run on the try server to finalize if it’s acceptable or not.

Said three bugs are:
Bug 698384
Bug 698385
Bug 718274

You may notice that there is not actual patch for 698385. That is because the changes to that one and 698384 affect the same area of code so in order to properly fix one the other has to be done at the same time. Hence why the patch for both is located in 698384.

The first two were simply updates to the DOM specifications. Both document.createTreeWalker and document.createNodeIterator now had optional arguments according to the DOM Spec and obviously Mozilla wants to adhere to this spec as much as possible. The actual coding changes for these were pretty simple in the end once I knew what to do but getting there was another story. I can’t make the claim that I am an experienced C++ developer so coming in I had no idea how to deal with optional arguments. In the end I figured it out which can be read here.

Bug 698381 is still a work in progress. The types of changes are the same but with this one the update is to a class that has a VERY long and big inheritance chain. It literally is the basis for almost anything web related so finding all the various implementations of that method proved difficult at first but thanks to the help of some DXR searches I should have it finished shortly.

With 718274 I have to admit I thought the fixes needed might actually be more difficult when I first read over it. Not to say it wasn’t interesting but the changes were pretty quick. To sum it up, there was a desire to add a method to nsContentUtils called DispatchUntrustedEvent. The name pretty much tells you what it’s intent is, to send untrusted events. What is an untrusted event? That’s another story entirely.

However at the same time they wanted to introduce another private method called DispatchEvent that both DispatchUntrustedEvent and DispatchTrustedEvent would call as they both would be written the same way other than what they would set a boolean variable aTrusted too. I won’t bother showing the .h files additions but here is the transformation in the .cpp:

From…

static
nsresult GetEventAndTarget(nsIDocument* aDoc, nsISupports* aTarget,
                           const nsAString& aEventName,
                           bool aCanBubble, bool aCancelable,
                           nsIDOMEvent** aEvent,
                           nsIDOMEventTarget** aTargetOut)
{
  nsCOMPtr domDoc = do_QueryInterface(aDoc);
  nsCOMPtr target(do_QueryInterface(aTarget));
  NS_ENSURE_TRUE(domDoc && target, NS_ERROR_INVALID_ARG);

  nsCOMPtr event;
  nsresult rv =
    domDoc->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
  NS_ENSURE_SUCCESS(rv, rv);

  nsCOMPtr privateEvent(do_QueryInterface(event));
  NS_ENSURE_TRUE(privateEvent, NS_ERROR_FAILURE);

  rv = event->InitEvent(aEventName, aCanBubble, aCancelable);
  NS_ENSURE_SUCCESS(rv, rv);

  rv = privateEvent->SetTrusted(true);
  NS_ENSURE_SUCCESS(rv, rv);

  rv = privateEvent->SetTarget(target);
  NS_ENSURE_SUCCESS(rv, rv);

  event.forget(aEvent);
  target.forget(aTargetOut);
  return NS_OK;
}

// static
nsresult
nsContentUtils::DispatchTrustedEvent(nsIDocument* aDoc, nsISupports* aTarget,
                                     const nsAString& aEventName,
                                     bool aCanBubble, bool aCancelable,
                                     bool *aDefaultAction)
{
  nsCOMPtr event;
  nsCOMPtr target;
  nsresult rv = GetEventAndTarget(aDoc, aTarget, aEventName, aCanBubble,
                                  aCancelable, getter_AddRefs(event),
                                  getter_AddRefs(target));
  NS_ENSURE_SUCCESS(rv, rv);

  bool dummy;
  return target->DispatchEvent(event, aDefaultAction ? aDefaultAction : &dummy);
}

To…

static
nsresult GetEventAndTarget(nsIDocument* aDoc, nsISupports* aTarget,
                           const nsAString& aEventName,
                           bool aCanBubble, bool aCancelable,
                           bool aTrusted, nsIDOMEvent** aEvent,
                           nsIDOMEventTarget** aTargetOut)
{
  nsCOMPtr domDoc = do_QueryInterface(aDoc);
  nsCOMPtr target(do_QueryInterface(aTarget));
  NS_ENSURE_TRUE(domDoc && target, NS_ERROR_INVALID_ARG);

  nsCOMPtr event;
  nsresult rv =
    domDoc->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
  NS_ENSURE_SUCCESS(rv, rv);

  nsCOMPtr privateEvent(do_QueryInterface(event));
  NS_ENSURE_TRUE(privateEvent, NS_ERROR_FAILURE);

  rv = event->InitEvent(aEventName, aCanBubble, aCancelable);
  NS_ENSURE_SUCCESS(rv, rv);

  rv = privateEvent->SetTrusted(aTrusted);
  NS_ENSURE_SUCCESS(rv, rv);

  rv = privateEvent->SetTarget(target);
  NS_ENSURE_SUCCESS(rv, rv);

  event.forget(aEvent);
  target.forget(aTargetOut);
  return NS_OK;
}

// static
nsresult
nsContentUtils::DispatchTrustedEvent(nsIDocument* aDoc, nsISupports* aTarget,
                                     const nsAString& aEventName,
                                     bool aCanBubble, bool aCancelable,
                                     bool *aDefaultAction)
{
  return DispatchEvent(aDoc, aTarget, aEventName, aCanBubble, aCancelable,
                       true, aDefaultAction);
}

// static
nsresult
nsContentUtils::DispatchUntrustedEvent(nsIDocument* aDoc, nsISupports* aTarget,
                                       const nsAString& aEventName,
                                       bool aCanBubble, bool aCancelable,
                                       bool *aDefaultAction)
{
  return DispatchEvent(aDoc, aTarget, aEventName, aCanBubble, aCancelable,
                       false, aDefaultAction);
}

// static
nsresult
nsContentUtils::DispatchEvent(nsIDocument* aDoc, nsISupports* aTarget,
                              const nsAString& aEventName,
                              bool aCanBubble, bool aCancelable,
                              bool aTrusted, bool *aDefaultAction)
{
  nsCOMPtr event;
  nsCOMPtr target;
  nsresult rv = GetEventAndTarget(aDoc, aTarget, aEventName, aCanBubble,
                                  aCancelable, aTrusted, getter_AddRefs(event),
                                  getter_AddRefs(target));
  NS_ENSURE_SUCCESS(rv, rv);

  bool dummy;
  return target->DispatchEvent(event, aDefaultAction ? aDefaultAction : &dummy);
}

Hoping to finish 698381 soon. Once I’m done with that I hope to find something a little bit more in-depth to help me learn more as a developer!

Handling optional method parameters with Mozilla

The central focus of the bugs I have been working on over the last 2 weeks has been handling methods that take optional arguments and how to know if they have been passed. From here you can easily have code to handle setting those arguments to specific values in the event they were not passed in allowing your code to be more robust but also forgiving.

The bugs in question are the following:

Bug 698384
Bug 698385
Bug 698381

While I haven’t had a chance to do much with the third as it is slightly different, the other two are essentially done. My reviews are nearly done for them and I expect it to be over with sometime tomorrow.

The first step to making arguments in a method optional is adding the [optional] flag infront of the argument you are making optional it its IDL file.

  nsIDOMNodeIterator createNodeIterator(in nsIDOMNode root,
                                        [optional] in unsigned long whatToShow,
                                        [optional] in nsIDOMNodeFilter filter)
                                          raises(DOMException);
  nsIDOMTreeWalker   createTreeWalker(in nsIDOMNode root,
                                      [optional] in unsigned long whatToShow,
                                      [optional] in nsIDOMNodeFilter filter)
                                        raises(DOMException);

So now all users of this IDL will know that both createNodeIterator and createTreeWalker accept arguments that are optional.

While that’s a nice feature, its not particularly useful if those optional arguments are actually required to be used somewhere in the method forcing them to have a valid value at some point or another. That brings the question, how does one know if they were actually passed or not? Is there an event of some sort? Do they have default values?

No there isn’t any event associated with it. There ARE defaults but that isn’t completely reliable. For example, an optional argument of type boolean will default to false. But what if the user actually passed in false because that was the value they wanted? Well if your code was looking to see if that argument was false and taking action based on that your users would never be able to pass in false which is obviously no good.

The solution is another addition to our IDL file that will provide a useful counter for how many of the optional arguments were provided.

  [optional_argc] nsIDOMNodeIterator createNodeIterator(in nsIDOMNode root,
                                                        [optional] in unsigned long whatToShow,
                                                        [optional] in nsIDOMNodeFilter filter)
                                                          raises(DOMException);
  [optional_argc] nsIDOMTreeWalker   createTreeWalker(in nsIDOMNode root,
                                                      [optional] in unsigned long whatToShow,
                                                      [optional] in nsIDOMNodeFilter filter)
                                                        raises(DOMException);

From here all it requires is a simple addition of an argument in the language you are implementing in. For example, in the C++ implementation of this code for Mozilla you would do the following:

NS_IMETHODIMP
nsDocument::CreateNodeIterator(nsIDOMNode *aRoot,
                               PRUint32 aWhatToShow,
                               nsIDOMNodeFilter *aFilter,
                               PRUint8 aOptionalArgc,
                               nsIDOMNodeIterator **_retval)

NS_IMETHODIMP
nsDocument::CreateTreeWalker(nsIDOMNode *aRoot,
                             PRUint32 aWhatToShow,
                             nsIDOMNodeFilter *aFilter,
                             PRUint8 aOptionalArgc,
                             nsIDOMTreeWalker **_retval)

If the method has some sort of return value, you will want it to be the second last argument as Mozilla’s system expects their return value as the last argument in methods. If not then place it as the last one.

From there it’s simple. If aOptionalArgc is 0 none were given, if aOptionalArgc is 1 the first was given and if aOptionalArgc is 2 then both were given.

From there you can easily add logic to handle all the different scenarios aOptionalArgc could be and make your code very flexible!

Mozilla Bug Process using Git

With OSD700 now under way and many of the students in our class working on various sections of the mozilla code base I figured it would be helpful to give everyone a few pointers and a brief explanation of how to go about this.

Typical work flow might be like this:

  1. Get a bug to work on. Seems dumb to put but hear me out. With Bugzilla you aren’t able to just grab bugs as you please as a new user. You either need to get someone with the access rights to assign people or ask around about this. David Humphrey would obviously be a good place to start.
  2. Code Code Code Code Code Co…………. You get the point. Work on your bug. When you have it to point where either you feel it solves all the problems adequately or you just want some feed back proceed to point 3.
  3. At this point you have probably made many different commits. Maybe they were small changes, maybe they weren’t. Either way it’s best to rebase your code against master before doing a diff on your code to make the patch itself.
    1. Make a new temporary branch that is literally a duplicate of the one you have been working in.

      git checkout -b branchNameHere

    2. Until you get used to the process it’s best to take this precaution incase you mess things up (trust me, it hurts when you do).

    3. Go and update your master branch. You will want to make sure you have the most recent version of mozilla-central on your machine. Then return to the branch containing your changes.
    4. git rebase -i master


      This will allow you to rebase your code and, for what we want to do, combine all of our commits into ONE so we have only one nice patch file. After doing that you will wind up getting something like this on your screen:

    5. If you read the comment’s in blue you can see that there are options we can take while rebasing. What were are concerned with here is f, or fixup. We need one commit to still have p/pick beside it but every other commit needs to have f/fixup instead. For the one commit that will have pick beside it you need to ensure that it’s commit message is an appropriate one as it won’t be accepted otherwise by your reviewers. Something like the first commit in my example.
    6. After continuing you just need to finish off the rebase process and get ready to use a super duper awesome alias!
  4. Once you have finished rebasing you are on the home stretch. All it takes is one simple little git command and your are done. The only problem is this command isn’t available by default because it helps setup your patches to be in an acceptable format for Mozilla. Add this to your .gitconfig under [ alias ]:
    hgp = show --format=\"From: %an %n%s%n%b\" -U8

    Once you have this in your .gitconfig all it takes is a simple command to make a patch that will show the differences in your code compared to your local mozilla-central (Which should be up to date if you followed the instructions!):

    git hgp >place/file/in/whatever/directory/you/want/it/to/be/makeThisAMeaningFulNameForYourBug.patch
    
    For Example:
    
    git hgp >~/Desktop/bug698384.patch
    
  5. Woot! Now you just need to upload it to your bug, set it for review? and then who is going to review it! Wait, how do you know who should/could review it? Fear not! This should help you. If it doesn’t well, ask Humph if all else fails. Or #developers.

One final note. If your work ever requires you to use git mv on any of your files (maybe it needs to move to a different spot or you need to rename it completely) you will want to add this to your .gitconfig:

[diff]
  renames = copy

This will help ensure your patches are in an acceptable format for Mozilla.

Hope this helps!