Accessible::NativeState should be const

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P5
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: surkov, Assigned: dora.tokio, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(20 attachments)

2.92 KB, patch
surkov
: review+
Details | Diff | Splinter Review
970 bytes, patch
surkov
: review+
Details | Diff | Splinter Review
1.68 KB, patch
surkov
: review+
Details | Diff | Splinter Review
2.01 KB, patch
surkov
: review+
Details | Diff | Splinter Review
4.95 KB, patch
surkov
: review+
Details | Diff | Splinter Review
4.52 KB, patch
surkov
: review+
Details | Diff | Splinter Review
48.53 KB, patch
surkov
: review+
Details | Diff | Splinter Review
6.61 KB, patch
surkov
: review+
Details | Diff | Splinter Review
5.47 KB, patch
surkov
: review+
Details | Diff | Splinter Review
1.73 KB, patch
surkov
: review+
Details | Diff | Splinter Review
2.43 KB, patch
Details | Diff | Splinter Review
2.80 KB, patch
surkov
: review+
Details | Diff | Splinter Review
18.11 KB, patch
surkov
: review+
Details | Diff | Splinter Review
2.95 KB, patch
surkov
: review+
Details | Diff | Splinter Review
26.89 KB, patch
surkov
: review+
Details | Diff | Splinter Review
4.20 KB, patch
surkov
: review+
Details | Diff | Splinter Review
15.71 KB, patch
surkov
: review+
Details | Diff | Splinter Review
8.65 KB, patch
surkov
: review+
Details | Diff | Splinter Review
28.13 KB, patch
surkov
: review+
Details | Diff | Splinter Review
34.81 KB, patch
surkov
: review+
Details | Diff | Splinter Review
follow up from bug 857348 comment #5:

It'd be nice to make State/NativeState a const methods, so we are guaranteed that we don't Shutdown() after IsDefunct() call.
Priority: -- → P5
Alexander, will you be mentoring this bug?
Flags: needinfo?(surkov.alexander)
(In reply to Emma Humphries, Bugmaster ☕️ (she/her) [:emceeaich] (UTC-8) needinfo? me from comment #1)
> Alexander, will you be mentoring this bug?

sure, should I put [mentor=:surkov] into the whiteboard?
Flags: needinfo?(surkov.alexander)
Mentor: surkov.alexander
Hi I'm new to Bugzilla, could I work on this as a first bug?
(In reply to James Leja from comment #3)
> Hi I'm new to Bugzilla, could I work on this as a first bug?

sure and welcome! assigning the bug to you, please don't hesitate to ask questions.
Assignee: nobody → james.leja
Thanks! I've been pretty busy with midterms and assignments, but I took a quick look at the previous bug. Should I concentrate just on the generic/Accessible.cpp? I noticed that the generic folder has a bunch of different NativeState methods, for example HyperTextAccessible::NativeState().
(In reply to James Leja from comment #5)
> Thanks! I've been pretty busy with midterms and assignments,

totally understandable

> but I took a
> quick look at the previous bug. Should I concentrate just on the
> generic/Accessible.cpp? I noticed that the generic folder has a bunch of
> different NativeState methods, for example
> HyperTextAccessible::NativeState().

at least this bug should be split into two parts: one patch for NativeState() and another one for State() method. You should change all NativeState/State() over all files for sure. Be careful though, when you start changing NativeState/State() methods, then you will likely figure out that some NativeState/State() method calls a non const method. You should convert those into consts as well (it should be doable in most cases). Please try to keep all dependent methods const-conversions in separate files.
I've been working on the NativeState() patch first. I've changed them all to const methods in the .cpp files, and added const override where necessary to the header files. I am in the process of converting the methods that call NativeState() to const, but  I have now run into NativeRole() methods, which also seemingly must be converted to const, because a Role() method was called in a NativeState() method. I just wanted to make sure I am on the right track, I am pretty new to this code base.
You are definitely on the right path. I don't see what particular NativeRole() caller you talk, it'd be helpful if provided a link, but there are two ways to approach it:
1) file a separate bug to make NativeRole() const method
2) convert Role() call to Accessible::Is...somethihg method, Role/NativeRole() are slow and should be avoided. You could do that in a separate bug too
3) both :)
Hi Alexander,

I'm sorry that I've been a bit MIA, I've been very busy with other assignments as the end of the semester approaches. I hit a snag with this bug, after having changed 5 or 6 methods throughout the Accessibility tree to const, I got a new set of errors including "assignment of member in read-only object". It seems like this bug is a bit more complex than I thought, and beyond my very limited C++ knowledge. I am going to continue to be very busy with school in the near future, so it seems appropriate to remove me as the assignee for this bug. I appreciate your patience and all the help you provided throughout the process.
No worries, James. It is indeed complex issue, however it should be possible to split the patch into smaller ones, starting from depending methods. You may want to upload what you have, to make it easier for next iterations.
Assignee: james.leja → nobody
Hi, I'd like to work on this bug.
Would you tell me the details?
(In reply to dora.tokio from comment #11)
> Hi, I'd like to work on this bug.
> Would you tell me the details?

Hey, sure! Comments #6 to comment #10 may serve as a good description of the problem and can give idea how to approach to it. Please let me know if those need to be refined.
I added const to member functions with the number of const limited to required minimum, and seemingly there are two bottlenecks.
First, LinkableAccessible::ActionWalk needs to be const, but in this function "this" pointer is assigned to non-const variable "walkUpAcc", which is then returned.
Second, Accessible::GetGroupInfo also needs to be const, but this is a destructive method. [2]

The first seems easy to fix, and the second may be not. Above all, the patch I'm writing has got a little long.
So, can I ask your advice before keeping on coding on my own?

[1]https://searchfox.org/mozilla-central/source/accessible/generic/BaseAccessibles.cpp#142
[2]https://searchfox.org/mozilla-central/source/accessible/generic/Accessible.cpp#2726


The const dependencies are as follows. ("A -> B" means "a function B should be const to make A a const function")

Role -> ARIATransformRole -> Name -> NativeName -> Value -> [ActionWalk]

RelationByType -> ItemIterator -> FirstItemOf -> [GetGroupInfo]
I forgot to say, I'm currently trying to make NativeSet const, and I guess if converting Role() call to some other method, doing it as another bug would be better because of simplicity.
(In reply to dora.tokio from comment #13)
> I added const to member functions with the number of const limited to
> required minimum, and seemingly there are two bottlenecks.
> First, LinkableAccessible::ActionWalk needs to be const, but in this
> function "this" pointer is assigned to non-const variable "walkUpAcc", which
> is then returned.

I think it should be possible to keep the pointer const:
const Accessible* walkUp = this;

It'd be nice to put that as a separate patch.

> Second, Accessible::GetGroupInfo also needs to be const, but this is a
> destructive method. [2]

It's probably a bit ugly but should be ok to mark mStageFlags and mBits as mutable, that will allow their modification inside const method.


> The first seems easy to fix, and the second may be not. Above all, the patch
> I'm writing has got a little long.

please try to break into smaller patches, going from bottom to up
Assignee: nobody → dora.tokio
(In reply to alexander :surkov from comment #15)

> I think it should be possible to keep the pointer const:
> const Accessible* walkUp = this;

This fails to compile and error says "invalid conversion from ‘const mozilla::a11y::Accessible*’ to ‘mozilla::a11y::Accessible*’" because of "return walkUpAcc;"

> please try to break into smaller patches, going from bottom to up

I got it.
(In reply to dora.tokio from comment #16)
> (In reply to alexander :surkov from comment #15)
> 
> > I think it should be possible to keep the pointer const:
> > const Accessible* walkUp = this;
> 
> This fails to compile and error says "invalid conversion from ‘const
> mozilla::a11y::Accessible*’ to ‘mozilla::a11y::Accessible*’" because of
> "return walkUpAcc;"

ActionWalk() could return const Accessible* type, which means we should turn a bunch of other methods into const, like TakeFocus() and Value(), and it should be ok, just the path is getting longer.

> > please try to break into smaller patches, going from bottom to up
> 
> I got it.

thank you
Posted patch part1: IsLinkSplinter Review
turn IsLink() into a const function
turn Accessible::FirstChild & LastChild into const functions
turn XULTreeItemAccessibleBase::GetCellName & IsExpandable into const functions
turn Accessible* in AccIterator into const
Attachment #8973181 - Attachment description: State and NativeState should be const → IsLink
Attachment #8973181 - Attachment description: IsLink → part1: IsLink
Attachment #8973184 - Attachment description: State and NativeState should be const → part2: FirstChild & LastChild
Attachment #8973184 - Flags: review+
Comment on attachment 8973181 [details] [diff] [review]
part1: IsLink

Review of attachment 8973181 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thanks! please make sure to request review, when you attach the patch, so the reviwer is aware the action is required :)

also it'd be good to give different names for the patches
Attachment #8973181 - Flags: review+
Attachment #8973190 - Attachment description: State and NativeState should be const → part3: XULTreeItemAccessibleBase
Attachment #8973190 - Flags: review+
These are miscellaneous patches which have no dependencies on each other, and as I don't know how much small I have to cut a patch into pieces, I separated it by function for now.
Next I'll throw a few small patches and then 3 or 4 large patches which cannot be divided anymore after trying try-server.

By the way, I thought the patches' name should be associated Bug's name, but could I change it with other name?
(In reply to dora.tokio from comment #23)
> These are miscellaneous patches which have no dependencies on each other,
> and as I don't know how much small I have to cut a patch into pieces, I
> separated it by function for now.

this size of your patches is a pleasure to review :)

> Next I'll throw a few small patches and then 3 or 4 large patches which
> cannot be divided anymore after trying try-server.

that's fine for sure

> By the way, I thought the patches' name should be associated Bug's name, but
> could I change it with other name?

I guess you could give them any reasonable name, just make sure to refer the bug number
Keywords: leave-open
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c5ffbe6cbf
turn Accessible::FirstChild & LastChild into const functions, r=surkov
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e568ee5e7499
turn XULTreeItemAccessibleBase::GetCellName & IsExpandable into const functions, r=surkov
Attachment #8973204 - Attachment description: State and NativeState should be const → part4: AccIterator
Attachment #8973204 - Flags: review+
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a585d77f897f
turn Accessible* in AccIterator into const, r=surkov
turn ColCount and IsMultiColumn into const functions
turn Accessible::DoCommand into a const function
Attachment #8973475 - Flags: review+
turn NativeRole into const functions
The next and last patch will be too large, including changes of more than 30 kinds of functions, because its const-dependencies are circular and cannot be taken apart anymore.
what would be the best option, throwing this patch as it is, or separating it forcibly by using const_cast or something?
(In reply to dora.tokio from comment #34)
> The next and last patch will be too large, including changes of more than 30
> kinds of functions, because its const-dependencies are circular and cannot
> be taken apart anymore.
> what would be the best option, throwing this patch as it is, or separating
> it forcibly by using const_cast or something?

I can manage a large size patch for review. Another option would be splitting it into bunch of patchesm keeping logically related stuff together, which can be reviewed separately but landed at once.

Btw, if you got try server access, then it makes sense to send unlanded patches to a try server, and then ask for review
Attachment #8973545 - Flags: review+
turn SetCurrentItem & GetChromeFlags & HasChildren into const functions
turn Accessible::CurrentItem into const functions
turn Accessible* in XULTreeItemIterator into const
When I sent patches to try-server, would it be better to say something to that effect?
(In reply to dora.tokio from comment #42)
> When I sent patches to try-server, would it be better to say something to
> that effect?

when you send a patch to the try server, make sure to include bug number, in this case a record about a push will appear here automatically.
Oh, I sent patches to try-server sometimes, but have its records properly posted?

By the way, I found some new NativeRole functions were added to ARIAGridAccessible.h when pulling latest source codes. So, I'll send a patch about ARIAGridAccessible::NativeRole.
turn ARIAGridAccessible::NativeRole into const functions
(In reply to dora.tokio from comment #44)
> Oh, I sent patches to try-server sometimes, but have its records properly
> posted?

it seems no, any links to your try runs?

> By the way, I found some new NativeRole functions were added to
> ARIAGridAccessible.h when pulling latest source codes. So, I'll send a patch
> about ARIAGridAccessible::NativeRole.

I applied your patch and made some modifications already, so no worries :) I'll land it soon.

Btw, you should ask for review explicitly to make sure it doens't skip off my radar.
(In reply to alexander :surkov from comment #46)
> it seems no, any links to your try runs?
This is the latest try-server link [1], in which I reran patches part 7 ~ 10.

> I applied your patch and made some modifications already, so no worries :)
> I'll land it soon.
Thanks!

> Btw, you should ask for review explicitly to make sure it doens't skip off
> my radar.
I thought adding "r=surkov" on a patch led to notify you until now, but is it wrong?


[1]https://treeherder.mozilla.org/#/jobs?repo=try&revision=d06fa7c23111885e7a1d50694a3d929c2cfd5131
Attachment #8973784 - Flags: review+
(In reply to alexander :surkov from comment #46)

(In reply to dora.tokio from comment #47)
> (In reply to alexander :surkov from comment #46)
> > it seems no, any links to your try runs?
> This is the latest try-server link [1], in which I reran patches part 7 ~ 10.

It seems try runs are no longer posted on a bug. Anyway this information is rather for you - if you've got any failures, then you should fix them :)

> 
> > I applied your patch and made some modifications already, so no worries :)
> > I'll land it soon.
> Thanks!

np. btw I found out that HTMLWin32ObjectOwnerAccessible::NativeRole was skipped on the patch. I also have fixed it.

> > Btw, you should ask for review explicitly to make sure it doens't skip off
> > my radar.
> I thought adding "r=surkov" on a patch led to notify you until now, but is
> it wrong?

you should request review via bugzilla UI. You can do this when uploading a patch.
Attachment #8975325 - Flags: review+
Attachment #8975327 - Flags: review+
Attachment #8975326 - Flags: review+
Attachment #8975963 - Flags: review?(surkov.alexander)
Attachment #8975964 - Flags: review?(surkov.alexander)
Attachment #8975965 - Flags: review?(surkov.alexander)
Attachment #8975967 - Flags: review?(surkov.alexander)
Attachment #8975969 - Flags: review?(surkov.alexander)
Attachment #8975970 - Flags: review?(surkov.alexander)
Attachment #8975971 - Flags: review?(surkov.alexander)
Attachment #8975973 - Flags: review?(surkov.alexander)
Part 11-1 ~ 9 are divided by functions for reviewing, and please put together these patches when applying.
Attachment #8975966 - Flags: review?(surkov.alexander)
Attachment #8975963 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8975964 [details] [diff] [review]
part11-2: RelationByType

Review of attachment 8975964 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/Accessible.h
@@ +1155,4 @@
>    /**
>     * Keep in sync with StateFlags, ContextFlags, and AccTypes.
>     */
> +  mutable uint32_t mStateFlags : kStateFlagsBits;

Just realized that we need to figure out better approach, since it ruins down the whole idea of the bug, i.e. if mStateFlags is mutable, then we may theoretically shutdown during a const call.

Let's land it as is but file a new bug to address the issue.
Attachment #8975964 - Flags: review?(surkov.alexander) → review+
Attachment #8975965 - Flags: review?(surkov.alexander) → review+
Attachment #8975966 - Flags: review?(surkov.alexander) → review+
Attachment #8975967 - Flags: review?(surkov.alexander) → review+
Attachment #8975969 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8975970 [details] [diff] [review]
part11-7: TakeFocus

Review of attachment 8975970 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/Accessible.cpp
@@ +741,4 @@
>  }
>  
>  void
> +Accessible::TakeFocus() const

technically I don't have any problems to make TakeFocus const, since it doesn't change accessible object members, but just curious, why it was necessary?
(In reply to alexander :surkov from comment #64)
> Comment on attachment 8975970 [details] [diff] [review]
> part11-7: TakeFocus
> 
> Review of attachment 8975970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/Accessible.cpp
> @@ +741,4 @@
> >  }
> >  
> >  void
> > +Accessible::TakeFocus() const
> 
> technically I don't have any problems to make TakeFocus const, since it
> doesn't change accessible object members, but just curious, why it was
> necessary?

There is a const Accessible calling TakeFocus, which was a return value of ActionWalk. [1]
In addition, TakeFocus is called in DoAction, which will be turned into const on a later patch due to ActionWalk. [2]

[1] https://searchfox.org/mozilla-central/source/accessible/generic/BaseAccessibles.cpp#73
[2] https://searchfox.org/mozilla-central/source/accessible/html/HTMLFormControlAccessible.cpp#459
Attachment #8975970 - Flags: review?(surkov.alexander) → review+
Attachment #8975971 - Flags: review?(surkov.alexander) → review+
Since this one is getting really huge, filing a new bug 1462430 for Accessible::State() method
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Summary: State and NativeState should be const → Accessible::NativeState should be const
Comment on attachment 8975973 [details] [diff] [review]
part11-9: NativeState

Review of attachment 8975973 [details] [diff] [review]:
-----------------------------------------------------------------

apparently I forgot r+ this one

::: accessible/generic/BaseAccessibles.h
@@ +116,4 @@
>    explicit DummyAccessible(DocAccessible* aDocument = nullptr) :
>      AccessibleWrap(nullptr, aDocument) { }
>  
> +  uint64_t NativeState() const final;

please file a follow up bug to make all these method virtual and having override specifier
Attachment #8975973 - Flags: review?(surkov.alexander) → review+
I filed bug 1464115 for DummyAccessible bug.
Depends on: 1479642
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.