Int and Float properties for Accessibility Object Model phase 1

Int and Float properties for Accessibility Object Model phase 1

BUG=680345
Review-Url: https://codereview.chromium.org/2894103002
Cr-Commit-Position: refs/heads/master@{#473999}
Committed: https://chromium.googlesource.com/chromium/src/+/0a2243c09c7513d55bde53ceeae0ea89a8334e79

[dmazzoni, 2017-05-19 04:34:05]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[dmazzoni, 2017-05-19 04:34:32]

Description was changed from

==========
Int and Float properties for Accessibility Object Model phase 1

BUG=680345
==========

to

==========
Int and Float properties for Accessibility Object Model phase 1

BUG=680345
==========

[dmazzoni, 2017-05-19 04:34:32]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, mkwst@chromium.org

[commit-bot: I haz the power, 2017-05-19 04:34:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-05-19 05:09:47]

Really only nits, but did the layout tests somehow get missed?

https://codereview.chromium.org/2894103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right):

https://codereview.chromium.org/2894103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:39: static T
GetPropertyHelper(P property,
Naming suggestion: FindPropertyValue

https://codereview.chromium.org/2894103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:78: return 0.0;
Why not something like:

float default = 0.0f;
if (element && element->ExistingAccessibleNode()) {
     return GetPropertyHelper(
          property, is_null,
element->ExistingAccessibleNode()->float_properties_,
          default);
} 
return default;

Also, I would probably invert these two cases, i.e. 
if (!element || !element->ExistingAccessibleNode())
    return default;
...

Up to you, though.

[commit-bot: I haz the power, 2017-05-19 05:56:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 05:56:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2017-05-19 14:53:50]

On Thu, May 18, 2017 at 10:09 PM <aboxhall@chromium.org> wrote:

> Really only nits, but did the layout tests somehow get missed?
>

Elliott suggested we move to *only* SimTests. Maybe that's a bit extreme.
I'll add both.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-05-19 14:53:52]

On Thu, May 18, 2017 at 10:09 PM <aboxhall@chromium.org> wrote:

> Really only nits, but did the layout tests somehow get missed?
>

Elliott suggested we move to *only* SimTests. Maybe that's a bit extreme.
I'll add both.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-05-19 20:23:08]

Added LayoutTest coverage too. Every property
is covered by at least one test.

https://codereview.chromium.org/2894103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right):

https://codereview.chromium.org/2894103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:39: static T
GetPropertyHelper(P property,
On 2017/05/19 05:09:47, aboxhall wrote:
> Naming suggestion: FindPropertyValue

Done.

https://codereview.chromium.org/2894103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:78: return 0.0;
On 2017/05/19 05:09:47, aboxhall wrote:
> Why not something like:
> 
> float default = 0.0f;
> if (element && element->ExistingAccessibleNode()) {
>      return GetPropertyHelper(
>           property, is_null,
> element->ExistingAccessibleNode()->float_properties_,
>           default);
> } 
> return default;
> 
> Also, I would probably invert these two cases, i.e. 
> if (!element || !element->ExistingAccessibleNode())
>     return default;
> ...
> 
> Up to you, though.
> 
>  

Good suggestions, done.

[dmazzoni, 2017-05-22 17:33:37]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 17:34:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 19:34:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 19:34:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aboxhall, 2017-05-23 06:08:25]

lgtm

[dmazzoni, 2017-05-23 16:28:23]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 16:29:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2017-05-23 18:34:43]

LGTM

[dmazzoni, 2017-05-23 18:36:18]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2017-05-23 18:36:24]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-05-23 18:36:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/2894103002/#ps40001
(title: "Update webexposed/")

[commit-bot: I haz the power, 2017-05-23 18:37:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 18:43:45]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495564584375050,
"parent_rev": "f494f8655828bca8094e8a71a9bda2ecdddd280d", "commit_rev":
"0a2243c09c7513d55bde53ceeae0ea89a8334e79"}

[commit-bot: I haz the power, 2017-05-23 18:44:02]

Description was changed from

==========
Int and Float properties for Accessibility Object Model phase 1

BUG=680345
==========

to

==========
Int and Float properties for Accessibility Object Model phase 1

BUG=680345

Review-Url: https://codereview.chromium.org/2894103002
Cr-Commit-Position: refs/heads/master@{#473999}
Committed:
https://chromium.googlesource.com/chromium/src/+/0a2243c09c7513d55bde53ceeae0...
==========

[commit-bot: I haz the power, 2017-05-23 18:44:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0a2243c09c7513d55bde53ceeae0...

[tkent, 2017-07-28 02:31:13]

tkent@chromium.org changed reviewers:
+ tkent@chromium.org

[tkent, 2017-07-28 02:31:14]

https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right):

https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:280: return
attr_value.GetString().ToUInt();
Is String::ToUInt() correct for aria attribute parsing?
It accepts leading/trailing Unicode whitespaces, and trailing garbage.

I couldn't find specification for parsing numbers in aria attribute values.

https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:310: return
attr_value.ToInt();
Ditto.

[dmazzoni, 2017-07-28 03:52:37]

Looks like WebKit uses WTFString::toInt for some int ARIA attributes, FWIW.
I'll have to experiment with Firefox to see what it accepts.

I'm assuming you mean we should use ParseHTMLInteger instead?


On Thu, Jul 27, 2017 at 7:31 PM <tkent@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right):
>
>
>
https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/AccessibleNode.cpp:280: return
> attr_value.GetString().ToUInt();
> Is String::ToUInt() correct for aria attribute parsing?
> It accepts leading/trailing Unicode whitespaces, and trailing garbage.
>
> I couldn't find specification for parsing numbers in aria attribute
> values.
>
>
>
https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/AccessibleNode.cpp:310: return
> attr_value.ToInt();
> Ditto.
>
> https://codereview.chromium.org/2894103002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-07-28 03:52:38]

Looks like WebKit uses WTFString::toInt for some int ARIA attributes, FWIW.
I'll have to experiment with Firefox to see what it accepts.

I'm assuming you mean we should use ParseHTMLInteger instead?


On Thu, Jul 27, 2017 at 7:31 PM <tkent@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right):
>
>
>
https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/AccessibleNode.cpp:280: return
> attr_value.GetString().ToUInt();
> Is String::ToUInt() correct for aria attribute parsing?
> It accepts leading/trailing Unicode whitespaces, and trailing garbage.
>
> I couldn't find specification for parsing numbers in aria attribute
> values.
>
>
>
https://codereview.chromium.org/2894103002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/AccessibleNode.cpp:310: return
> attr_value.ToInt();
> Ditto.
>
> https://codereview.chromium.org/2894103002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.