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 >
