IBM/Sun branch necessary?

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

IBM/Sun branch necessary?

Aaron Leventhal-3
Some good news and an important question --
Sun and IBM have agreed to begin work on fixing our ATK support for
Firefox 3. This relates to the accessible objects we create for
Linux/UNIX screen reader access.

Sun and IBM each have 3 developers devoted to the required changes to
mozilla/accessible. We will be reviewing each others changes. Ginn Chen
is the Sun team lead and I'm the IBM team lead. For the most part we
will be reviewing, rather than writing the actual code.

However, this code needs to be able to evolve rapidly, so we do not want
to use superreview for every patch.
To avoid bottlenecking the team and problems with the vast timezone
differences we all want to look for practical ways to speed up the
collaboration. We could create a branch but would prefer to just do our
changes on the trunk. We will work to ensure crash- and leak-free code,
including fixing the current leak(s) discovered by David Baron.

Should we create a branch if we want to avoid the sr= requirement for
every checkin or can module-owner approval enough for
mozilla/accessible? As the module owner my role this time involves less
coding than reviewing and guiding. Also, this is just for
mozilla/accessible not in core Gecko. It only affects how accessible
objects are created, not focus/key nav changes for example. We don't
expect anyone outside of the Sun/IBM collaboration to need to do
significant changes to this code.

Automated tests are being built by the 2 IBM full time testers working
on accessibility. Not for leaks yet, but I'll have them do that as well.
In addition they're doing non-automated testing and we'll be using
alpha/beta testers.

Do we definitely need super review on every line of code in the
mozilla/accessible module? We're going to have 6 eyes on it as it is.
Where we change APIs etc. I think superreview makes sense.
If we need to do superreviews for absolutely every line of code then I
understand the need for more incremental reviews.
We have agreed on what  changes need to be made. Here is the planning
document: http://www.mozilla.org/access/unix/new-atk.html
To some extent the changes involve moving code out of platform-specific
areas in mozilla/accessible and sharing the code better with the Windows
implemenation. The new approach builds the accessible object tree in a
manner more compatible with what is done for MSAA on Windows.

Please let us know -- we're ready to work together to fix Firefox 3
accessibility on Linux and UNIX.

- Aaron
http://www.mozilla.org/access
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

Boris Zbarsky
Aaron Leventhal wrote:
> However, this code needs to be able to evolve rapidly, so we do not want
> to use superreview for every patch.

What follows is all my opinion.  ;)

In my experience, super-review has been used for two distinct things:

1)  Code cleanliness review.  Things like proper use of XPCOM and our string
classess, mismatched allocators, tabs-vs-spaces, general coding style, etc.  The
sorts of things that module owner review should reasonable be expected to handle
but didn't in some modules at some points in time.

2)  "integration" or "big picture" review.  Things like proper use of the APIs
exposed by other modules (eg the recurring bugs due to incorrect URI object
creation we've had), general proper interaction with the rest of the app, review
of API designs.

Frankly, the first use case of sr is silly.  It was needed because some module
owners would "review" patches without ever looking at them, as far as I could
tell at the time.  But that's not happening now, so there's no reason for sr to
cover that eventuality.

The second case is more interesting.  I can see cases where module owners may
want to waive sr for such situations; that basically implies that the module
owner is going to handle integration review himself.  Which is fine with me,
more or less, if they're willing to accept that burden and can cope with it.  If
they can't cope with it, then it's clearly no good.  The way I see it, a module
owner should do this if they feel that their knowledge of the various modules
their module interacts with is good enough to catch possible issues arising in
the interaction (or at least do so as well as the srs would).

So to apply all this to this specific case, I'd think that changes to the
accessibility code that are limited to the accessibility module (eg changes to
internal data structures, changes to the interaction with the OS accessibility
APIs or with accessibility software, etc) can get away without sr if they get
decent module owner review.  Quite frankly, I doubt any of the srs would be able
to do useful "integration review" on those changes anyway.  On the other hand,
changes to the way accessibility interacts with layout and content (event
listeners it sets, implementations of APIs like nsIDocumentObserver, changes to
accessibility hooks in the frame constructor or presshell, and so forth) are
likely to benefit from sr by someone who knows the layout/content bits in
question.  I don't know what other modules accessibility really interacts with,
but similar reasoning would apply for those.

Again, if the accessibility module owners and peers feel that they understand
well (as well as the relevant srs) how the APIs in question, eg. layout/content
ones, work (including implementation details that are not documented in the
API), I can see a case for waiving sr for changes that interact with those APIs.
Are we in that case here?

> We could create a branch but would prefer to just do our changes on the trunk.

Generally speaking, unless your work would leave the browser in an unusable
state that would prevent dogfooding by other developers, I think you should stay
on trunk.

My $0.04,

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

Aaron Leventhal-3
Boris Zbarsky wrote:

Thanks Boris. We're basically using the same APIs we already use but we
need to clean some things up, move code around and create our tree of
objects in a different order. Where we reach territory that we have
questions about we're smart enough to ensure we get the correct usage by
asking the relevant expert. Our changes won't effect other developers.

I'll take on the burden of ensuring that what gets done has high
quality. If there's something complex I may pull in another high quality
reviewer or one of the super reviewers to look at it.

- Aaron

> Aaron Leventhal wrote:
>> However, this code needs to be able to evolve rapidly, so we do not
>> want to use superreview for every patch.
>
> What follows is all my opinion.  ;)
>
> In my experience, super-review has been used for two distinct things:
>
> 1)  Code cleanliness review.  Things like proper use of XPCOM and our
> string classess, mismatched allocators, tabs-vs-spaces, general coding
> style, etc.  The sorts of things that module owner review should
> reasonable be expected to handle but didn't in some modules at some
> points in time.
>
> 2)  "integration" or "big picture" review.  Things like proper use of
> the APIs exposed by other modules (eg the recurring bugs due to
> incorrect URI object creation we've had), general proper interaction
> with the rest of the app, review of API designs.
>
> Frankly, the first use case of sr is silly.  It was needed because some
> module owners would "review" patches without ever looking at them, as
> far as I could tell at the time.  But that's not happening now, so
> there's no reason for sr to cover that eventuality.
>
> The second case is more interesting.  I can see cases where module
> owners may want to waive sr for such situations; that basically implies
> that the module owner is going to handle integration review himself.  
> Which is fine with me, more or less, if they're willing to accept that
> burden and can cope with it.  If they can't cope with it, then it's
> clearly no good.  The way I see it, a module owner should do this if
> they feel that their knowledge of the various modules their module
> interacts with is good enough to catch possible issues arising in the
> interaction (or at least do so as well as the srs would).
>
> So to apply all this to this specific case, I'd think that changes to
> the accessibility code that are limited to the accessibility module (eg
> changes to internal data structures, changes to the interaction with the
> OS accessibility APIs or with accessibility software, etc) can get away
> without sr if they get decent module owner review.  Quite frankly, I
> doubt any of the srs would be able to do useful "integration review" on
> those changes anyway.  On the other hand, changes to the way
> accessibility interacts with layout and content (event listeners it
> sets, implementations of APIs like nsIDocumentObserver, changes to
> accessibility hooks in the frame constructor or presshell, and so forth)
> are likely to benefit from sr by someone who knows the layout/content
> bits in question.  I don't know what other modules accessibility really
> interacts with, but similar reasoning would apply for those.
>
> Again, if the accessibility module owners and peers feel that they
> understand well (as well as the relevant srs) how the APIs in question,
> eg. layout/content ones, work (including implementation details that are
> not documented in the API), I can see a case for waiving sr for changes
> that interact with those APIs. Are we in that case here?
>
>> We could create a branch but would prefer to just do our changes on
>> the trunk.
>
> Generally speaking, unless your work would leave the browser in an
> unusable state that would prevent dogfooding by other developers, I
> think you should stay on trunk.
>
> My $0.04,
>
> -Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

Axel Hecht-2
In reply to this post by Aaron Leventhal-3
Hi,

you mentioned testing, but still my question would be, this sounds like
a huge overhaul, could there be a degrading performance or stability
during that?
If so, I'd recommend a branch.

I guess it's still drivers that would do a change of review policy for a
part of the tree, if you're seeking to get rid of the sr requirement in
general. Maybe you could go the "2nd review"-way instead, to make sure
that each patch has three pairs of eyes on it.

Axel
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

Aaron Leventhal-3
It won't degrade performance or stability in the general case.
Accessibility has to be turned on explicitly or the code is not even run.

I want to keep the process from becoming heavy-weight so that we can
address all the issues efficiently and effectively. We're bridging
cultures and have a 12 hour gap between timezones.

- Aaron

Axel Hecht wrote:

> Hi,
>
> you mentioned testing, but still my question would be, this sounds like
> a huge overhaul, could there be a degrading performance or stability
> during that?
> If so, I'd recommend a branch.
>
> I guess it's still drivers that would do a change of review policy for a
> part of the tree, if you're seeking to get rid of the sr requirement in
> general. Maybe you could go the "2nd review"-way instead, to make sure
> that each patch has three pairs of eyes on it.
>
> Axel
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

Aaron Leventhal-3
In reply to this post by Axel Hecht-2
I should add that I don't mind using 2nd review for anything that is a
significant change. In fact the 2nd review will more than likely be
helpful and save us time. Getting multiple reviews from this extended
team is not a problem.

- Aaron



Axel Hecht wrote:

> Hi,
>
> you mentioned testing, but still my question would be, this sounds like
> a huge overhaul, could there be a degrading performance or stability
> during that?
> If so, I'd recommend a branch.
>
> I guess it's still drivers that would do a change of review policy for a
> part of the tree, if you're seeking to get rid of the sr requirement in
> general. Maybe you could go the "2nd review"-way instead, to make sure
> that each patch has three pairs of eyes on it.
>
> Axel
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

L. David Baron
In reply to this post by Aaron Leventhal-3
On Wednesday 2006-04-05 14:27 -0400, Aaron Leventhal wrote:
> It won't degrade performance or stability in the general case.
> Accessibility has to be turned on explicitly or the code is not even run.

I'm skeptical of this claim, partly since nobody has answered my
questions in
http://groups.google.com/group/mozilla.dev.accessibility/msg/ed653782ce4b326f

I'm also skeptical that (even if it's true) it should matter as far as
approval policies go.  See the same message for why.

-David

--
L. David Baron                                <URL: http://dbaron.org/ >
           Technical Lead, Layout & CSS, Mozilla Corporation

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning

attachment0 (198 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

L. David Baron
In reply to this post by Aaron Leventhal-3
On Wednesday 2006-04-05 10:19 -0400, Aaron Leventhal wrote:
> Thanks Boris. We're basically using the same APIs we already use but we
> need to clean some things up, move code around and create our tree of
> objects in a different order. Where we reach territory that we have
> questions about we're smart enough to ensure we get the correct usage by
> asking the relevant expert. Our changes won't effect other developers.

I think you definitely do need design review on things like the
following:
 * on which other tree (content or frame) should the accessible tree be
   based?
 * which APIs should be used to keep the accessible tree in sync with
   that tree?
 * what ownership model should be used for the accessible tree to ensure
   that it goes away at the right time, even in error cases?

(Based on looking at the current code for bug 330624, I suspect the
current code's answer to the first question is correct, its answer to
the second may be incorrect, and its answer to the third is probably
incorrect, although the answers to the latter seem too poorly documented
for me to be sure what they are.)

I'm also concerned about issues surrounding testing of this code; the
fact that whether it's triggered is left totally up to some undocumented
black magic coming from the OS makes it hard to test and hard to
understand when it gets triggered.  (See my other message in this
thread.)

-David

--
L. David Baron                                <URL: http://dbaron.org/ >
           Technical Lead, Layout & CSS, Mozilla Corporation

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning

attachment0 (198 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

Aaron Leventhal-3
In reply to this post by Aaron Leventhal-3
L. David Baron wrote:
> On Wednesday 2006-04-05 14:27 -0400, Aaron Leventhal wrote:
>> It won't degrade performance or stability in the general case.
>> Accessibility has to be turned on explicitly or the code is not even run.
>
> I'm skeptical of this claim, partly since nobody has answered my
> questions in
> http://groups.google.com/group/mozilla.dev.accessibility/msg/ed653782ce4b326f
Well, your question is answered. Apologies for the delay on that.

>
> I'm also skeptical that (even if it's true) it should matter as far as
> approval policies go.  See the same message for why.
I don't mind that we have stricter approval this time around. But for
during the meaty part of the Firefox 3 development I want to try to keep
the spread-out development functioning on all cylinders. Not trying to
avoid having high quality code. More reviews are better especially if
they aren't holding up the team a lot while we wait for them.

Just to explain what we're trying to do ...

IBM & Sun's goal for Firefox 3 is to bring the Windows and Linux
implementations of accessibility together a lot more. This can now
happen because of the recent work I was involved in, standardizing how a
containment hierarchy should be exposed with ATK. That is documented at
http://www.mozilla.org/access/unix/new-atk.html
In the future the APIs on both platforms will be synchronized a bit more
-- I'm currently involved in the design of APIs on both platforms. This
will reduce the amount of code in general because there will be less
special-casing for different platforms.

No doubt there will be a ton of tiny polishing fixes required after we
do that piece.



>
> -David
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: IBM/Sun branch necessary?

Aaron Leventhal-3
In reply to this post by Aaron Leventhal-3
L. David Baron wrote:
> I think you definitely do need design review on things like the
> following:
>  * on which other tree (content or frame) should the accessible tree be
>    based?
>  * which APIs should be used to keep the accessible tree in sync with
>    that tree?
>  * what ownership model should be used for the accessible tree to ensure
>    that it goes away at the right time, even in error cases?

I would be extremely happy to get this kind of design review for the
accessibility module. The more I can get top Mozilla architects to look
at mozilla/accessible the better it will end up. In the past I was
stretched a bit thin, but I think we've done very well in terms of
stability and performance. The leak is very unfortunate. We have been
leak-free in the past and I hope/believe your patch will set things
right again.

 > (Based on looking at the current code for bug 330624, I suspect the
 > current code's answer to the first question is correct, its answer to
 > the second may be incorrect, and its answer to the third is probably
 > incorrect, although the answers to the latter seem too poorly documented
 > for me to be sure what they are.)

Here are docs on the architecture -- it explains that we use hash tables
to own the accessible nodes, so that neither the DOM nor layout need to
own accessible objects:
http://www.mozilla.org/access/architecture

 >
 > I'm also concerned about issues surrounding testing of this code; the
 > fact that whether it's triggered is left totally up to some undocumented
 > black magic coming from the OS makes it hard to test and hard to
 > understand when it gets triggered.  (See my other message in this
 > thread.)

The mechanisms for enabling accessibility aren't really undocumented --
they are documented in the MSAA and ATK documentation. On Mac
accessibility is never enabled -- so if you see the leaks there it's not
the fault of accessibility. In any case, we can put some more test hooks
into the code if you have some ideas. It would be good to be able to get
statistics (perhaps via talkback) as to how many people have a11y
enabled on each platform.

BTW, apologies for delays into getting to the leak and your questions.
That standardization work (and the DHTML standardization work) as well
as post-1.5 evangelism are the reasons. I'm now getting into gear to get
back into the code again, along with more help this time.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning