Thanks Jack! My signals got crossed this morning, and my understanding was that I was to check my stuff in right after Karen. So, checked my stuff in and then saw your comments. Sorry about that. :-(
Anyhow, I'm all for consistent style and I'm happy to make the changes. A new webrev has been uploaded to http://cr.opensolaris.org/~ww36193/slim_3785-2/ for your review. Thanks again, Will On Thu, 2008-10-09 at 10:04 -0700, Jack Schwartz wrote: > Hi Will. > > Overall, looks good. > > slim_install/svc/Makefile: > -------------------------- > Nit: when possible try to keep lines under 80 chars. Can you please > wrap line 56 to two lines? > > slim-install/svc/live-a11y: > --------------------------- > The script will be more robust or at least more explicit if full > pathnames (e.g. /usr/sbin/prtconf rather than just prtconf) are used. > > Please try to tend to the 80 char line maximum. > > The de-facto shell style guide we are using ( > http://opensolaris.org/os/community/on/shellstyle/ ) talks of doing > if-then-elses like this: > > if [ condition ] ; then > do it > else > do that > fi > > The point is for putting "then"s on the same line as the if. The style > guide is wrong about one thing though: we're supposed to use [[ > condition ]] (2 brackets each side, not one). > > Not a real big deal, but this is my understanding of the proper way of > doing shell coding. > > live-fs-root: > ------------- > Same comment about spelling out full pathnames to binaries. > > - Suggestion: you can define full pathnames near the top, and then use > their definitions, like this: > PRTCONF=/usr/sbin/prtconf > SED=/usr/bin/sed > CUT=/usr/bin/cut > ... > ... > assistive_tech=`$PRTCONF -v /devices|$SED -n > '/assistive_tech/{;n;p;}'|$CUT -f 2 -d\'` > > - Consider this a personal preference though since what's already there > doesn't do that... > > The rest looks fine to me. > > Thanks, > Jack > > > > > Willie Walker wrote: > > Hey All (especially Jack, Jean, and Karen): > > > > This is my first code review request for Caiman, so I hope I'm giving you > > the > > right info. Please review the changes to enable accessibility (a11y) for > > the > > live CD. > > > > Bug 3785 - Add accessibility support to the 2008.11 live CD > > http://defect.opensolaris.org/bz/show_bug.cgi?id=3785 > > > > Webrev: http://cr.opensolaris.org/~ww36193/slim_3785/ > > > > The description of the changes is in comment #1 of the bug report: > > http://defect.opensolaris.org/bz/show_bug.cgi?id=3785#c1 > > > > TESTING: > > > > Systems used: VirtualBox on my AMD-64 Opteron desktop > > DVD media on my Toshiba Tecra M2 laptop > > > > Procedures: > > 1) Boot with the default option. Make sure no a11y > > features are enabled on the system. > > 2) Boot with the screen reader option. Make sure a11y > > features were enabled and the screen reader started. > > Install. Verify that the only change that was copied > > to the system was the /lib/inetd/nwamd wrapper. > > 3) Same as #2, but with magnifier. > > > > NOTES: > > > > JackS is working on http://defect.opensolaris.org/bz/show_bug.cgi?id=3600 > > to > > support adding grub options to the manifest. If 3600 gets done, the > > finalizer > > from this patch can be removed and replaced with additional grub lines in > > slim_cd.xml. > > > > Since development freeze is Monday, though, I wanted to get this reviewed > > soon > > so that I could start work on any suggested changes as soon as possible. > > > > Thanks! > > > > Will > > > > _______________________________________________ > > caiman-discuss mailing list > > caiman-discuss at opensolaris.org > > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > > >
