PDF accessibility tweaks

PDF accessibility tweaks

1. Allows EnableAccessibility to be called at any time, and doesn't actually
   send the accessibility tree until after the document loads. (In the future
   we could send it while loading, but this is a good start.)
2. The character width wasn't accurate if there was space between
   characters in the same text run. Update it so that a character's width
   includes all of the space between this character and the start of the
   next one, because the purpose of this field is a very compact
   representation of the character bounds within a line, to enable
   visually highlighting the accessible selection.
3. Heuristic: split a text run after whitespace wider than 3 characters.
4. Heuristic: if the font size is missing or obviously bogus, use the
   pixel height of the text run, which is at least in the right ballpark.

BUG=54724
Committed: https://crrev.com/d48d93288ff25bb6e07a67c61af5486a1092ad10
Cr-Commit-Position: refs/heads/master@{#399505}

[dmazzoni, 2016-06-09 19:03:43]

dmazzoni@chromium.org changed reviewers:
+ raymes@chromium.org, thestig@chromium.org

[Lei Zhang, 2016-06-09 22:19:05]

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance.cc
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:698: pp::FloatRect char_bounds =
engine_->GetCharBounds(page_index, char_index);
Can you add a brief description of what this new block of code is doing?

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:701: page_index, char_index + i + 1);
Can you double check and make sure char_index + i + 1 doesn't go out of bounds?

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:1272: if (accessibility_enabled_)
This is a bit confusing, since EnableAccessibility() is the place that sets
accessibility_enabled_ to true.

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance.h
File pdf/out_of_process_instance.h (right):

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.h:355: bool accessibility_loaded_;
Are all 4 combinations of the two a11y variables reachable? Do you think an enum
might better description the state we are in?

[dmazzoni, 2016-06-10 17:44:52]

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance.cc
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:698: pp::FloatRect char_bounds =
engine_->GetCharBounds(page_index, char_index);
On 2016/06/09 22:19:05, Lei Zhang (Slow) wrote:
> Can you add a brief description of what this new block of code is doing?

Done.

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:701: page_index, char_index + i + 1);
On 2016/06/09 22:19:05, Lei Zhang (Slow) wrote:
> Can you double check and make sure char_index + i + 1 doesn't go out of
bounds?

Added a DCHECK.

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:1272: if (accessibility_enabled_)
On 2016/06/09 22:19:05, Lei Zhang (Slow) wrote:
> This is a bit confusing, since EnableAccessibility() is the place that sets
> accessibility_enabled_ to true.

Split into EnableAccessibility() and LoadAccessibility().

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance.h
File pdf/out_of_process_instance.h (right):

https://codereview.chromium.org/2050973003/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.h:355: bool accessibility_loaded_;
On 2016/06/09 22:19:05, Lei Zhang (Slow) wrote:
> Are all 4 combinations of the two a11y variables reachable? Do you think an
enum
> might better description the state we are in?

Good idea, take a look.

[Lei Zhang, 2016-06-10 18:57:43]

lgtm

[dmazzoni, 2016-06-13 18:12:27]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-13 18:12:27]

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

[commit-bot: I haz the power, 2016-06-13 18:13:03]

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

[commit-bot: I haz the power, 2016-06-13 19:37:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-13 19:37:59]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 19:39:28]

Description was changed from

==========
PDF accessibility tweaks

1. Allows EnableAccessibility to be called at any time, and doesn't actually
   send the accessibility tree until after the document loads. (In the future
   we could send it while loading, but this is a good start.)
2. The character width wasn't accurate if there was space between
   characters in the same text run. Update it so that a character's width
   includes all of the space between this character and the start of the
   next one, because the purpose of this field is a very compact
   representation of the character bounds within a line, to enable
   visually highlighting the accessible selection.
3. Heuristic: split a text run after whitespace wider than 3 characters.
4. Heuristic: if the font size is missing or obviously bogus, use the
   pixel height of the text run, which is at least in the right ballpark.

BUG=54724
==========

to

==========
PDF accessibility tweaks

1. Allows EnableAccessibility to be called at any time, and doesn't actually
   send the accessibility tree until after the document loads. (In the future
   we could send it while loading, but this is a good start.)
2. The character width wasn't accurate if there was space between
   characters in the same text run. Update it so that a character's width
   includes all of the space between this character and the start of the
   next one, because the purpose of this field is a very compact
   representation of the character bounds within a line, to enable
   visually highlighting the accessible selection.
3. Heuristic: split a text run after whitespace wider than 3 characters.
4. Heuristic: if the font size is missing or obviously bogus, use the
   pixel height of the text run, which is at least in the right ballpark.

BUG=54724

Committed: https://crrev.com/d48d93288ff25bb6e07a67c61af5486a1092ad10
Cr-Commit-Position: refs/heads/master@{#399505}
==========

[commit-bot: I haz the power, 2016-06-13 19:39:30]

Patchset 3 (id:??) landed as
https://crrev.com/d48d93288ff25bb6e07a67c61af5486a1092ad10
Cr-Commit-Position: refs/heads/master@{#399505}