Difference between revisions of "Development:Coding"

From Camino Wiki
Jump to navigation Jump to search
(→‎Proper patch format: fill in instructions here, since I did this the other day on cvs-www. also warn about dropping -N from diffs with add/remove operations on their files)
Line 67: Line 67:
 
Sometimes the code you are writing requires the creation of new source files.  In order to allow these files to be included in your single patch file, you must perform the additional step of <tt>cvs add</tt>ing the file, or, if you do not have cvs write privileges, of imitating the <tt>cvs add</tt> process.
 
Sometimes the code you are writing requires the creation of new source files.  In order to allow these files to be included in your single patch file, you must perform the additional step of <tt>cvs add</tt>ing the file, or, if you do not have cvs write privileges, of imitating the <tt>cvs add</tt> process.
  
To make a patch including a new file (which otherwise requires write access to the repository), use the third-party [http://developer.mozilla.org/en/docs/Creating_a_patch#Including_new_files_in_a_patch cvsdo] utility (which you'll need to install) to imitate the <tt>cvs add</tt> operation.  <tt>cvsdo add</tt> each new file (alternatively add <code>/Foo.h/0/dummy timestamp//</code> to <tt>path/to/file/CVS/Entries</tt>), then diff as normal.
+
To make a patch including a new file (which otherwise requires write access to the repository), use the third-party [http://developer.mozilla.org/en/docs/Creating_a_patch#Including_new_files_in_a_patch cvsdo] utility (which you'll need to install) to imitate the <tt>cvs add</tt> operation.  <tt>cvsdo add</tt> each new file (alternatively add <code>/Foo.h/0/dummy timestamp//</code> to <tt>path/to/file/CVS/Entries</tt>), then diff as normal. ''If you do not include <code>-N</code> in your <code>cvs diff</code> command, the diff will fail.''
  
To make a patch including files in new directories, first <tt>cvsdo add</tt> each new directory and then <tt>cvsdo add</tt> the files in those directories.  Then create a diff of everything NOT in a new directory, and finally <tt>cvsdo diff</tt> on ''each'' new directory, appending the changes to your existing diff (<tt>cvsdo diff path/to/newdir/ >> EXISTING_DIFF</tt>)
+
To make a patch including files in new directories, first <tt>cvsdo add</tt> each new directory and then <tt>cvsdo add</tt> the files in those directories.  Then create a diff of everything NOT in a new directory, and finally <tt>cvsdo diff</tt> on ''each'' new directory, appending the changes to your existing diff (<tt>cvsdo diff path/to/newdir/ >> EXISTING_DIFF</tt>).  ''If you do not include <code>-N</code> in your <code>cvs diff</code> command, the diff will fail.''
  
 
====Removing files in a patch====
 
====Removing files in a patch====
Sometimes your patch obsoletes existing files in the tree.   
+
Sometimes your patch obsoletes existing files in the tree.  If you do not have write access to the repository, you will have to use the third-party [http://developer.mozilla.org/en/docs/Creating_a_patch#Including_new_files_in_a_patch cvsdo] utility (which you'll need to install) to imitate the <tt>cvs remove</tt> operation.  To indicate the file has been removed, first delete the file.  Then <code>cvsdo remove</code> each removed file and diff as usual.  ''If you do not include <code>-N</code> in your <code>cvs diff</code> command, the diff will fail.''
 
 
''Instructions TBA''
 
  
 
=== <tt>Localizable.strings</tt> changes ===
 
=== <tt>Localizable.strings</tt> changes ===

Revision as of 15:26, 19 May 2009

Here are some useful tips for coding a feature or bug fix for Camino and creating a patch to send your changes through review.

Code Style

Link to Mozilla coding style guidelines as well as create some for Camino, so we avoid Bug 308942 comment 8 and 14 and Bug 228840 comments 15, 16, 18, and 19.

Also link to Hacking Mozilla general intro somewhere in our contributor introduction

All new code should conform to Camino's style guidelines, which is a descendent of the Mozilla style guidelines (per decree by our leader):

if (foo) {
  bar;
  blam;
}
else {
  baz;
  zap;
}

Braces for single statements are optional (and discouraged, but some still include them).

We explicitly have no style rule regarding the placement of *. Aim to be consistent within a file in as much as the files are consistent.

Coding for latest-OS-version features or using private APIs

Each situation is unique, but we have provided some existing examples of various strategies employed to correctly code for these situations.

Most of the code mentioned below was originally written for Camino 1.6 and allowed Camino to run on Mac OS X version 10.3.9 through 10.5, whether or not the particular feature was supported on that OS or declared in the default Camino SDK (10.4u). The learnWord: code dates from Camino 1.5, which had the same runtime requirements but built against the 10.3.9 SDK on PPC. The code for setting proxies dates from Camino 1.0, which ran on Mac OS X 10.2.8 through 10.4 (and built against the 10.2.8 SDK on PPC, 10.4u SDK on Intel).

The spell-checker's learnWord: method is undocumented, but it is the only way to achieve proper "learned spelling" integration with the Mac OS X spelling system. The code supporting Camino’s use of this method is here and here.

On Mac OS X 10.5, Camino alsos support building a Language sub-menu for switching spell-check languages.

Setting Spotlight metadata on files is still undocumented as of the 10.5 SDK. Our download listener code has a very well-documented and commented example of how to use a method like this safely while running on Mac OS X 10.4 and 10.5.

See also the Quarantine attributes code for an example of working with features available only on Mac OS X 10.5 while remaining compatible with older versions of the OS.

Setting Mac OS X proxy preferences is available only in the 10.4u SDK and later, although the feature is available at least as far back as Mac OS X 10.3.2. PreferenceManager.mm implements this feature safely across all Mac OS X versions Camino supports; see here.

mento also gives a pseudo-code explanation of how to guard and do runtime look-up for older SDKs in bug 397717 comment 21.

Situation

Description and mxr link

Setting breakpoints in external files

Sometimes when debugging you will want to set breakpoints in files that are part of Camino, but not in the XCode project. For example, widget/src/cocoa/nsNativeThemeCocoa.mm. If you get an error about gdb not being able to find the file:linenumber, you will need to add the relevant shared library. Head to Run->Show->Shared Libraries... and set the Starting Level for the library to All. Which library to set it for depends on the file; for nsNativeThemeCocoa.mm, it is libwidget_mac.dylib. Turning off "Lazy Symbol Loading" in Debugger preferences may also help.

Preparing a patch to submit for review

Before creating a patch for review, be sure that your source tree is up-to-date and that Camino builds with your changes.

In the $SRCDIR, cd mozilla/camino and then cvs up -dP. By default, cvs will attempt to merge any changes from the repository into your working copy. If it fails to merge the repository changes with your changes, cvs will inform you with a C mark before the file and a message about conflicts. You will need to resolve the conflict manually.

After updating your tree, switch back to $OBJDIR/camino and make (or build in Xcode) to make sure your changes and the latest code build. Ideally you would also launch Camino and test your changes one last time.

Proper patch format

Use cvs diff -u8N for patches to Camino code. Diffs should be done from $SRCDIR/mozilla/camino or $SRCDIR/mozilla for consistency and ease of application by reviewers and committers. All changes—including new source code files and project file changes, if applicable—should be submitted as a single patch (e.g., cvs diff -u8N src/foo/bar src/baz/bam > bugnumber.patch), with the exception of changes to nibs and images; see below for more about requirements for those three file types.

Adding new files in a patch

Sometimes the code you are writing requires the creation of new source files. In order to allow these files to be included in your single patch file, you must perform the additional step of cvs adding the file, or, if you do not have cvs write privileges, of imitating the cvs add process.

To make a patch including a new file (which otherwise requires write access to the repository), use the third-party cvsdo utility (which you'll need to install) to imitate the cvs add operation. cvsdo add each new file (alternatively add /Foo.h/0/dummy timestamp// to path/to/file/CVS/Entries), then diff as normal. If you do not include -N in your cvs diff command, the diff will fail.

To make a patch including files in new directories, first cvsdo add each new directory and then cvsdo add the files in those directories. Then create a diff of everything NOT in a new directory, and finally cvsdo diff on each new directory, appending the changes to your existing diff (cvsdo diff path/to/newdir/ >> EXISTING_DIFF). If you do not include -N in your cvs diff command, the diff will fail.

Removing files in a patch

Sometimes your patch obsoletes existing files in the tree. If you do not have write access to the repository, you will have to use the third-party cvsdo utility (which you'll need to install) to imitate the cvs remove operation. To indicate the file has been removed, first delete the file. Then cvsdo remove each removed file and diff as usual. If you do not include -N in your cvs diff command, the diff will fail.

Localizable.strings changes

Any UI string that appears in the code rather than in a nib needs to have an entry in Localizable.strings (or, in rare instances, a preference pane's Localizable.strings or one of the other .strings files). When writing new strings, always use “curly quotes” in the actual string text.

Beginning with Camino 1.6a1 (23-Oct-2007 nightlies), we have converted all .strings files to UTF-8 .strings.in files which can be diffed and which are then turned into UTF-16 .strings files in the build process. Any changes to .strings.in files should be included in your diff. Any new .strings files should be added to the tree as .strings.in files, converted to UTF-16 .strings files via the STRINGS_FILES mechanism in Makefile.in, and then add the resulting .strings files in generated/ to the project in the appropriate places.

To see string changes, you must rebuild Camino from the command line.

MOZILLA_1_8_BRANCH

Baring any critical security bugs requiring .strings changes, the MOZILLA_1_8_BRANCH is string frozen, and no string changes should occur there.

Project (Camino.xcodeproj) changes

Making project changes has to be done using Xcode (when adding, deleting, or moving files, as well as changing most build/compiler options), specifically when adding files. After making any project changes, simply diff the project file as well and include it in your patch (if you made any changes to build configuration or settings, be sure to diff camino/config/ as well to pick up xcconfig changes). Please check to ensure that your project patches do not change the state of the Camino.app entries in the Camino and CaminoStatic targets (this is usually a problem with srcdir configurations).

(Removing files can be done by hand if you don't have the proper version of Xcode, but this isn't recommended because removing the obvious entries for files will still leave non-obvious entries in the project file and cause crashes/build failures.)

Adding Gecko build products to Camino and the project file

  • If the item is "optional" module (e.g., extension) and is not built by default, fix up confvars.sh (on the branch, instead fix up the Camino section of configure.in and regenerate configure) to make Camino pull and build the item.
    • In some cases you can use an ac_add_options flag in your .mozconfig rather than patching confvars.sh/configure.in, but anything that's required to make Camino not burn when all is done needs to be declared in confvars.sh/configure.in.
  • If the item required confvars.sh/configure.in or .mozconfig changes, do a full rebuild of your tree.
    • You'll typically have to build both non-static and static unless you're intimately familiar with the location of build products and naming conventions
  • If the library or .xpt is something that seems relevant to all embeddors, embedding/config/basebrowser-mac-macho should probably be modified to deliver the build products to dist/Embed/* rather than dist/*
  • Add the libraries to appropriate build target(s), and using "Relative to Project" paths when adding
    • In most cases you'll want to add to the non-static (default) and static target separately
    • There are separate folders in the Xcode "sources" tree for static and non-static libs, and for components and "regular" libs
  • Adding is best done from a srcdir build, but can be done with an objdir build if you carefully fix paths manually; note that in some cases Xcode will resolve symlinks when adding, so you'll have to fix the paths to point back to dist/Embed/* (or dist/*, if appropriate)
  • Move the products from Bundle Resources Copy Phase to the correct copy phase for that sort of Gecko product (for static builds, the static libs belong in the Libraries and Frameworks phase). Different versions of Xcode mis-guess the Copy Phase destinations in different ways.
  • Repeat the previous two steps for any .xpt files that the libraries might require
  • Check for, and remove if necessary, any bizarre changes to the header or library search paths that Xcode may have “helpfully” added for you.
  • Fix nsStaticComponents.cpp for static builds

Nib changes

When editing nibs, be sure to follow the guidelines in Development:Editing Nibs. Until further notice, all nib work should be done only in Interface Builder 2.5.x (preferably IB 2.5.4 under Mac OS X 10.4, as a number of features do not work properly in IB 2.5.6 under Mac OS X 10.5 and its nibs can load more slowly, but it can be run under in a pinch).

The Nib changes and other resource files section on the Reviewing page includes additional information about preparing your nib changes for review.

Images and other binary resources

Images should use the .tiff extension (with two “t"s) and be post-processed[1] with tiffutil. See Development:Preparing Graphics for full details.