Fixed algorithms that compute bounding rectangles and word start offsets to take into account IA2 h…

Fixed algorithms that compute bounding rectangles and word start offsets to take into account IA2 hypertext and i18n.

1. std::string was being used instead of base::string16.
2. Start offset and len did not take into account embedded objects in IA2 hypertext and existing functions worked on the assumption that object hierarchy had been flattened.
BUG=578341
R=dmazzoni@chromium.org
TESTED=NVDA console, browser tests

Committed: https://crrev.com/31a53111bcf69efa7ff1481a0efd19b89a3b7cad
Cr-Commit-Position: refs/heads/master@{#370788}

[dmazzoni, 2016-01-20 01:10:43]

lgtm

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_win_browsertest.cc (right):

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_win_browsertest.cc:1043: for (int
test_case = 0; test_case < 1; ++test_case) {
Delete this loop since there's only 1 test case now

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_win_browsertest.cc:1065:
EXPECT_LT(0, x) << "at offset 0";
Do you also want to assert that this is actually on the next line? Your test
would succeed if it put everything on the same line.

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:267: // Embedded objects
are of length one, since they are represented only by
This comment confuses me. It talks about embedded objects, but the code checks
if the child is a text-only object, which is a static text or line break.

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:276: else
nit: Need braces around both blocks

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:958: // DCHECK_EQ(2U,
product_components.size());
Remove this? Where did this fail, can you fix it instead?

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.h:728: base::string16
GetText() const override { return win_attributes_->hypertext; }
Since this is an inline accessor, it should be lowercase, i.e. just text(). If
you want to call it GetText(), it shouldn't be inline.

[chromium-reviews, 2016-01-20 18:02:10]

Also added special handling for standard text fields.

>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> File content/browser/accessibility/accessibility_win_browsertest.cc
> (right):
>
>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/accessibility_win_browsertest.cc:1043: for
> (int test_case = 0; test_case < 1; ++test_case) {
> Delete this loop since there's only 1 test case now
>
No, there are two test cases: Screen_relative vs. parent_relative 
coordinates.
Changed loop to make it more clear.
for (int i = IA2_COORDTYPE_SCREEN_RELATIVE; i < 
IA2_COORDTYPE_PARENT_RELATIVE; ++i) {
auto coordinate_type = static_cast<IA2CoordinateType>(i);
....
}

>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/accessibility_win_browsertest.cc:1065:
> EXPECT_LT(0, x) << "at offset 0";
> Do you also want to assert that this is actually on the next line? Your
> test would succeed if it put everything on the same line.
>
Done. Good Great suggestion.

>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> File content/browser/accessibility/browser_accessibility.cc (right):
>
>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility.cc:267: // Embedded
> objects are of length one, since they are represented only by
> This comment confuses me. It talks about embedded objects, but the code
> checks if the child is a text-only object, which is a static text or
> line break.
>
Text-only objects are the only ones that Firefox and our Windows code 
doesn't treat as embedded objects.
Re-worded comment.
// Child objects are of length one, since they are represented by a
       // single special character. The exception is text-only objects.

>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility.cc:276: else
> nit: Need braces around both blocks
>
Removed else clause by initializing length to 1.

>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> File content/browser/accessibility/browser_accessibility_win.cc (right):
>
>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility_win.cc:958: //
> DCHECK_EQ(2U, product_components.size());
> Remove this? Where did this fail, can you fix it instead?
>
Removed.

>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> File content/browser/accessibility/browser_accessibility_win.h (right):
>
>
https://codereview.chromium.org/1598583002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility_win.h:728:
> base::string16 GetText() const override { return
> win_attributes_->hypertext; }
> Since this is an inline accessor, it should be lowercase, i.e. just
> text(). If you want to call it GetText(), it shouldn't be inline.
>
Made it non-inline.

-- 
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.

[nektarios, 2016-01-20 18:42:09]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-01-20 18:42:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1598583002/#ps40001
(title: "Addressed comments by reviewer.")

[commit-bot: I haz the power, 2016-01-20 18:44:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598583002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598583002/40001

[commit-bot: I haz the power, 2016-01-20 18:46:46]

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

[commit-bot: I haz the power, 2016-01-20 18:46:46]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dmazzoni, 2016-01-20 19:44:41]

https://codereview.chromium.org/1598583002/diff/40001/content/browser/accessi...
File content/browser/accessibility/accessibility_win_browsertest.cc (right):

https://codereview.chromium.org/1598583002/diff/40001/content/browser/accessi...
content/browser/accessibility/accessibility_win_browsertest.cc:1040: for (int i
= IA2_COORDTYPE_SCREEN_RELATIVE; i < IA2_COORDTYPE_PARENT_RELATIVE;
This still isn't correct, if you want to test both of those you need <= instead
of <.

Also, I'd make coordinate_type be the name of the loop variable, not "i".

[nektarios, 2016-01-20 23:55:24]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-01-20 23:55:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1598583002/#ps60001
(title: "Fixed all unit tests.")

[commit-bot: I haz the power, 2016-01-20 23:57:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598583002/60001

[nektarios, 2016-01-21 00:22:23]

The CQ bit was unchecked by nektar@chromium.org

[nektarios, 2016-01-21 00:26:25]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-01-21 00:26:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1598583002/#ps80001
(title: "Made passing start and len arguments that are outside the text
illegal.")

[commit-bot: I haz the power, 2016-01-21 00:29:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598583002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598583002/80001

[commit-bot: I haz the power, 2016-01-21 00:56:18]

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

[commit-bot: I haz the power, 2016-01-21 00:56:19]

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

[nektarios, 2016-01-21 01:13:21]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-01-21 01:13:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1598583002/#ps100001
(title: "Fixed another unit test.")

[commit-bot: I haz the power, 2016-01-21 01:15:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598583002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598583002/100001

[commit-bot: I haz the power, 2016-01-21 02:08:17]

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

[commit-bot: I haz the power, 2016-01-21 02:08:18]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nektarios, 2016-01-21 18:00:34]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-01-21 18:00:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1598583002/#ps120001
(title: "Added test with embedded image.")

[commit-bot: I haz the power, 2016-01-21 18:02:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598583002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598583002/120001

[commit-bot: I haz the power, 2016-01-21 18:15:08]

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

[commit-bot: I haz the power, 2016-01-21 18:15:09]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[nektarios, 2016-01-21 18:58:15]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-01-21 18:58:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1598583002/#ps140001
(title: "Fixed Android manager.")

[commit-bot: I haz the power, 2016-01-21 18:59:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598583002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598583002/140001

[commit-bot: I haz the power, 2016-01-21 19:22:13]

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

[commit-bot: I haz the power, 2016-01-21 19:22:14]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nektarios, 2016-01-21 20:20:00]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-01-21 20:20:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1598583002/#ps180001
(title: "Added a TODO to investigate test failure on Linux.")

[commit-bot: I haz the power, 2016-01-21 20:21:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1598583002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1598583002/180001

[commit-bot: I haz the power, 2016-01-21 21:24:32]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-21 21:25:36]

Description was changed from

==========
Fixed algorithms that compute bounding rectangles and word start offsets to take
into account IA2 hypertext and i18n.

1. std::string was being used instead of base::string16.
2. Start offset and len did not take into account embedded objects in IA2
hypertext and existing functions worked on the assumption that object hierarchy
had been flattened.
BUG=578341
R=dmazzoni@chromium.org
TESTED=NVDA console, browser tests
==========

to

==========
Fixed algorithms that compute bounding rectangles and word start offsets to take
into account IA2 hypertext and i18n.

1. std::string was being used instead of base::string16.
2. Start offset and len did not take into account embedded objects in IA2
hypertext and existing functions worked on the assumption that object hierarchy
had been flattened.
BUG=578341
R=dmazzoni@chromium.org
TESTED=NVDA console, browser tests

Committed: https://crrev.com/31a53111bcf69efa7ff1481a0efd19b89a3b7cad
Cr-Commit-Position: refs/heads/master@{#370788}
==========

[commit-bot: I haz the power, 2016-01-21 21:25:37]

Patchset 10 (id:??) landed as
https://crrev.com/31a53111bcf69efa7ff1481a0efd19b89a3b7cad
Cr-Commit-Position: refs/heads/master@{#370788}