Closed Bug 1478727 Opened Last year Closed Last year

Move IDSet out of windows directory

Categories

(Core :: Disability Access APIs, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 1 obsolete file)

We need a way to have 32 bit IDs in Android as well. So I propose reusing Windows' IDSet for that.

Another thing we need is platform specific implementations of Accessible::UniqueID(). I know that making it virtual comes at a cost, but I don't know if it's a big deal. Would a preprocessor conditional per-platform make more sense?

I think we can also fold Windows' GetExistingID into this as well.
(In reply to Eitan Isaacson [:eeejay] from comment #0)
> Another thing we need is platform specific implementations of
> Accessible::UniqueID().

Is this so you can just use the ids pushed up from content, rather than pushing up a separate id (like we do on Windows) or generating a separate id in parent?

> I know that making it virtual comes at a cost, but I
> don't know if it's a big deal.

I've never been very good at the memory usage tradeoff question at scale. :) Having said that, it does seem strange to make it virtual when it's never going to be overridden *except* at the platform specific base level.

> Would a preprocessor conditional per-platform
> make more sense?

It'd be more efficient, but I'm trying to imagine how we'd do this without hideous code, since the id generation code for each platform ideally needs to be in the platform specific directory.

> I think we can also fold Windows' GetExistingID into this as well.

Probably. There are two code paths: one for proxies and one for accessibles originating in this process. However, I imagine that's also true for Android.

I need to give this some more thought. Might be worth syncing up on this one.
Comment on attachment 8995253 [details] [diff] [review]
Move IDSet out of windows to base for Android use. r?jteh

Eitan and I discussed this. Current thinking is to get rid of the virtual and do pre-processor stuff. We'll have a protected mID member which gets set in AccessibleWrap for the platforms which need it.

Cancelling review until these changes are made.
Attachment #8995253 - Flags: review?(jteh)
We discussed this, and I'll get back with a different patch.
Removed the virtual change. When I introduce the Android dir, i'll put in some preprocessor magic like we discussed.
Attachment #8999019 - Flags: review?(jteh)
Attachment #8995253 - Attachment is obsolete: true
Attachment #8999019 - Flags: review?(jteh) → review+
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b07b4bf548
Move IDSet out of windows to base for Android use. r=jteh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/18b07b4bf548
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.