Closed
Bug 1305402
Opened 8 years ago
Closed 8 years ago
Implement AtkTableCell interface
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jdiggs, Assigned: tbsaunde)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files)
6.55 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
6.32 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
10.14 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
2.70 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
A while back, we added a TableCell interface to ATK and AT-SPI2. ATK docs here: https://developer.gnome.org/atk/unstable/AtkTableCell.html It was largely based on the IA2 table cell interface, so hopefully it won't be too much effort to expose the same information via ATK. Thanks!
Assignee | ||
Comment 1•8 years ago
|
||
This is a somewhat new header upstream that we need to import to support the AtkTableCell interface. The one tricky bit is that upstream has started using AVAILABLE_IN_ATK_X_Y macros defined in a generated header. To avoid needing to generate that header on every build or checking in generated code we just define the macros manually in atk.h.
Attachment #8795732 -
Flags: review?(dbolter)
Assignee | ||
Comment 2•8 years ago
|
||
To implement the interface with 1 IPC message per method we need these new messages.
Attachment #8795734 -
Flags: review?(dbolter)
Assignee | ||
Comment 3•8 years ago
|
||
This is a pretty straight forward wrapper around the internal table cell interface. However we need to be careful to only expose this interface if we are dealing with a version of ATK new enough to understand it.
Attachment #8795735 -
Flags: review?(dbolter)
Updated•8 years ago
|
Assignee: nobody → tbsaunde+mozbugs
Updated•8 years ago
|
Attachment #8795732 -
Flags: review?(dbolter) → review+
Updated•8 years ago
|
Attachment #8795734 -
Flags: review?(dbolter) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8795735 [details] [diff] [review] implement the AtkTablecell interface Review of attachment 8795735 [details] [diff] [review]: ----------------------------------------------------------------- Looks okay to me, with comments: ::: accessible/atk/AccessibleWrap.cpp @@ +66,5 @@ > MAI_INTERFACE_TABLE, > MAI_INTERFACE_TEXT, > MAI_INTERFACE_DOCUMENT, > + MAI_INTERFACE_IMAGE, /* 10 */ > + MAI_INTERFACE_TABLE_CELL Why do we we need to have that 10 comment? (And I guess it should be 11 now) @@ +95,5 @@ > return ATK_TYPE_DOCUMENT; > case MAI_INTERFACE_IMAGE: > return ATK_TYPE_IMAGE; > + case MAI_INTERFACE_TABLE_CELL: > + MOZ_ASSERT(false); Maybe you should add a comment to the atk_if_infos definition saying "MAI_INTERFACE_TABLE_CELL intentionally left out" or something. @@ +427,5 @@ > &atk_if_infos[index]); > } > } > > + if (IsAtkVersionAtLeast(2, 12) && (interfacesBits & (1 << MAI_INTERFACE_TABLE_CELL))) { Maybe add a comment before this check to say why we need to special case this interface? ::: accessible/atk/nsMaiInterfaceTableCell.cpp @@ +67,5 @@ > + *aRow = rowIdx; > + return true; > + } > + > + return false; Probably should keep consistent style. You return 0 in previous functions. Your choice. @@ +78,5 @@ > + TableCellAccessible* cellAcc = accWrap->AsTableCell(); > + *aCol = cellAcc->ColIdx(); > + *aRow = cellAcc->RowIdx(); > + *aColExtent = cellAcc->ColExtent(); > + *aRowExtent = cellAcc->ColExtent();return true; nit: newline for return. @@ +198,5 @@ > +} > +} > + > +void > +tableCellInterfaceInitCB(AtkTableCellIface* aIface) Did you mean to have this outside extern "C" block?
Attachment #8795735 -
Flags: review?(dbolter) → review+
Assignee | ||
Comment 5•8 years ago
|
||
> ::: accessible/atk/AccessibleWrap.cpp > @@ +66,5 @@ > > MAI_INTERFACE_TABLE, > > MAI_INTERFACE_TEXT, > > MAI_INTERFACE_DOCUMENT, > > + MAI_INTERFACE_IMAGE, /* 10 */ > > + MAI_INTERFACE_TABLE_CELL > > Why do we we need to have that 10 comment? (And I guess it should be 11 now) not sure, I just added the ',' and left it. It does seem kind of useless. > ::: accessible/atk/nsMaiInterfaceTableCell.cpp > @@ +67,5 @@ > > + *aRow = rowIdx; > > + return true; > > + } > > + > > + return false; > > Probably should keep consistent style. You return 0 in previous functions. > Your choice. funny story, that functions return type is supposed to be gboolean not gint, but somehow it compiles anyway. I suspect gboolean is just a typedef of int :( > > +} > > +} > > + > > +void > > +tableCellInterfaceInitCB(AtkTableCellIface* aIface) > > Did you mean to have this outside extern "C" block? yes, it doesn't need to have C linkage just like all the other interface init functions.
Pushed by tsaunders@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b044f5da3670 import atktablecell.h r=davidb https://hg.mozilla.org/integration/mozilla-inbound/rev/28272f824ad0 add new IPC messages needed for AtkTableCell interface r=davidb https://hg.mozilla.org/integration/mozilla-inbound/rev/3152577aab9b implement the AtkTablecell interface r=davidb
Comment 7•8 years ago
|
||
Backed out for build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d604653b1cfa8cead02fd81c50b951caa80d45 https://hg.mozilla.org/integration/mozilla-inbound/rev/7afc2dc9016908e58ebd9fa942ef14426230a005 https://hg.mozilla.org/integration/mozilla-inbound/rev/34bc558363a1b446d77cd945bfa1a8da285739bc https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0f175618d350058a2547fd1a3023dbe826eb3d (the previous follow-up fix) Last push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=27509f16a0f2a011998b4dff8bba1fff6b48240a
Flags: needinfo?(tbsaunde+mozbugs)
Assignee | ||
Comment 8•8 years ago
|
||
We need to deal with versions of the atk library that don't provide this function, so we need to dynamically look it up instead of statically refering to it.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8801691 -
Flags: review?(dbolter)
Comment 9•8 years ago
|
||
Comment on attachment 8801691 [details] [diff] [review] dynamically lookup the atk_table_cell_get_type function Review of attachment 8801691 [details] [diff] [review]: ----------------------------------------------------------------- r=me (with the tweak to previous patch)
Attachment #8801691 -
Flags: review?(dbolter) → review+
Comment 10•8 years ago
|
||
Pushed by tsaunders@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5938913dcd import atktablecell.h r=davidb https://hg.mozilla.org/integration/mozilla-inbound/rev/bcabd365e0de add new IPC messages needed for AtkTableCell interface r=davidb https://hg.mozilla.org/integration/mozilla-inbound/rev/4e5f6ceea72b dynamically lookup the atk_table_cell_get_type function r=davidb https://hg.mozilla.org/integration/mozilla-inbound/rev/25e25969288c implement the AtkTablecell interface r=davidb
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0c5938913dcd https://hg.mozilla.org/mozilla-central/rev/bcabd365e0de https://hg.mozilla.org/mozilla-central/rev/4e5f6ceea72b https://hg.mozilla.org/mozilla-central/rev/25e25969288c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•