Hi Will.
Thanks for the respin. Looks much better to me. Just a couple of quick
things...
slimcd_grub_a11y: line 72 also has grep and head. Can you please do
these up like sed?
live_a11y: references mv, cat, chmod. Please do these up like sed too.
Thanks for making the format changes.
As far as I'm concerned, no need to resubmit another review for making
these changes.
Thanks,
Jack
Willie Walker wrote:
> 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
>>>
>>>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
