1251712 - propagate a context flag for alerts

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Before I review can you tell me the short story of 'why' this change?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #1)
> Before I review can you tell me the short story of 'why' this change?

this reduce a need to run the tree up on every insertion, that should help for a case when you make lot of insertions deep in the tree

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #2)
> (In reply to David Bolter [:davidb] from comment #1)
> > Before I review can you tell me the short story of 'why' this change?
> 
> this reduce a need to run the tree up on every insertion, that should help
> for a case when you make lot of insertions deep in the tree

Thanks! :)

Comment 4

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99963a1217f1

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8724178 [details] [diff] [review]
patch

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

r=me, with questions:

::: accessible/generic/Accessible.cpp
@@ +1984,5 @@
>      SetARIAHidden(true);
> +
> +  mContextFlags |=
> +    static_cast<uint32_t>((mParent->IsAlert() ||
> +                           mParent->IsInsideAlert())) & eInsideAlert;

I assume the boolean gets converted to 0xFFF... for the bitwise & operation? (Oh C++ sometimes you smell funny)

::: accessible/generic/Accessible.h
@@ +1148,5 @@
>    int32_t mIndexInParent;
>  
>    static const uint8_t kChildrenFlagsBits = 2;
>    static const uint8_t kStateFlagsBits = 11;
> +  static const uint8_t kContextFlagsBits = 3;

Aside: I don't remember why we later initialize mContextFlags to kContextFlagsBits and not 0?

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to David Bolter [:davidb] from comment #5)
> I assume the boolean gets converted to 0xFFF... for the bitwise & operation?

Or 0x000 of course.

Comment 7

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=407e8f8db5ba

Comment 8

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #5)

> >    static const uint8_t kChildrenFlagsBits = 2;
> >    static const uint8_t kStateFlagsBits = 11;
> > +  static const uint8_t kContextFlagsBits = 3;
> 
> Aside: I don't remember why we later initialize mContextFlags to
> kContextFlagsBits and not 0?

it's a memory size

Comment 9

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff87fac5346cecc9118046c39418feca73e4cce
Bug 1251712 - propagate a context flag for alerts, r=davdib

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ff87fac5346