Bug 30964 – [Gtk] Implement AtkDocument

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

We need to implement the AtkDocument interface.
http://library.gnome.org/devel/atk/unstable/AtkDocument.html

[Joanmarie Diggs]

Created attachment 42252 [details]
Part 1 - bare bones implementation

[Joanmarie Diggs]

Created attachment 42253 [details]
Part 2 - Attributes (requires part 1)

[Joanmarie Diggs]

Created attachment 42254 [details]
Part 3 - language/locale (requires part 1)

[Jan Alonzo]

Comment on attachment 42252 [details]
Part 1 - bare bones implementation

Looks fine. r=me.

[Jan Alonzo]

(In reply to comment #2)
> Created an attachment (id=42253) [details]
> Part 2 - Attributes (requires part 1)

I think we should start writing layout tests for these. Would that be possible today or there's a plan to write layout tests for these changes soon?

[Joanmarie Diggs]

(In reply to comment #5)
> (In reply to comment #2)
> > Created an attachment (id=42253) [details] [details]
> > Part 2 - Attributes (requires part 1)
> 
> I think we should start writing layout tests for these. Would that be possible
> today or there's a plan to write layout tests for these changes soon?

Learning about (and then writing) layout tests was on my to-do list, but not my today-/this-weekend list. :-) However, I will re-adjust my list. Last time I looked at merely running the tests (once they were running) I kept getting failures, so I've got a few things to work on before I can start generating tests.

[Xan Lopez]

(In reply to comment #5)
> (In reply to comment #2)
> > Created an attachment (id=42253) [details] [details]
> > Part 2 - Attributes (requires part 1)
> 
> I think we should start writing layout tests for these. Would that be possible
> today or there's a plan to write layout tests for these changes soon?

Mmm, do you mean unit tests here? Or how could we test ATK APIs from layout tests in JavaScript?

[Jan Alonzo]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > Created an attachment (id=42253) [details] [details] [details]
> > > Part 2 - Attributes (requires part 1)
> > 
> > I think we should start writing layout tests for these. Would that be possible
> > today or there's a plan to write layout tests for these changes soon?
> 
> Mmm, do you mean unit tests here? Or how could we test ATK APIs from layout
> tests in JavaScript?

Layout tests. We're not testing ATK APIs but testing the accessible characteristics of AX objects. AccessibilityController exposes some of this attributes that we can test in JavaScript. It's still missing a few implementations for Gtk though, but shouldn't be difficult to implement those.

[Xan Lopez]

(In reply to comment #8)
> > Mmm, do you mean unit tests here? Or how could we test ATK APIs from layout
> > tests in JavaScript?
> 
> Layout tests. We're not testing ATK APIs but testing the accessible
> characteristics of AX objects. AccessibilityController exposes some of this
> attributes that we can test in JavaScript. It's still missing a few
> implementations for Gtk though, but shouldn't be difficult to implement those.

OK. Since most of what we are doing here is implementing the ATK APIs I assumed you were referring to that. In any case, testing that all those APIs work as expected is, at least, as important as doing layout tests for any core AX functionality we are using or adding.

[Eric Seidel (no email)]

bugzilla-tool is not smart enough to leave bugs open after landing due to bug 28230.  So this bug will need to be re-opened after the commit-queue lands patch #1.  Either someone (which can be me), needs to fix bug 28230, or better yet, we should follow the one-change-per-bug style that is more common in WebKit. :)

[WebKit Commit Bot]

Comment on attachment 42252 [details]
Part 1 - bare bones implementation

Clearing flags on attachment: 42252

Committed r50393: <http://trac.webkit.org/changeset/50393>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joanmarie Diggs]

Comment on attachment 42253 [details]
Part 2 - Attributes (requires part 1)

Obsoleting this patch and clearing the review flag.

Bug 30997 has been spun off so that we only have one change per bug.

[Joanmarie Diggs]

Comment on attachment 42254 [details]
Part 3 - language/locale (requires part 1)

Obsoleting this patch and clearing the review flag.

Bug 30998 has been spun off so that we only have one change per bug.

[Joanmarie Diggs]

(In reply to comment #10)
> bugzilla-tool is not smart enough to leave bugs open after landing due to bug
> 28230.  So this bug will need to be re-opened after the commit-queue lands
> patch #1.  Either someone (which can be me), needs to fix bug 28230, or better
> yet, we should follow the one-change-per-bug style that is more common in
> WebKit. :)

Sorry. New bugs spun off for each patch.

Should I also file new bugs for the tests which I will (hopefully) write for each of those bugs, or do tests associated with a fix constitute part of a single change?

[Eric Seidel (no email)]

(In reply to comment #15)
> (In reply to comment #10)
> > bugzilla-tool is not smart enough to leave bugs open after landing due to bug
> > 28230.  So this bug will need to be re-opened after the commit-queue lands
> > patch #1.  Either someone (which can be me), needs to fix bug 28230, or better
> > yet, we should follow the one-change-per-bug style that is more common in
> > WebKit. :)
> 
> Sorry. New bugs spun off for each patch.
> 
> Should I also file new bugs for the tests which I will (hopefully) write for
> each of those bugs, or do tests associated with a fix constitute part of a
> single change?

Certainly nothing to apologize for!  :)   I just wanted you to know that our tools don't handle multiple-patches-per-bug very well.

Tests should go along with fixes.  When I review patches often seeing and understanding what the tests are doing is more important than the code itself.  If we have a test, we'll never break the code in the same way again. :)