Make the PDF plug-in accessible.

Make the PDF plug-in accessible.

Makes the PDF plug-in accessible on all supported platforms by building an
accessibility tree for the PDF content and grafting it on as a subtree of
the accessibility tree for the embedding page.

It exposes all of the text of the PDF, including full support for computing
the bounding box of any arbitrary text range.  Using heuristics, automatically
groups the text runs into paragraphs and marks some of them as headings if
their font size is larger than the average for the page.

I wrote a browser test for this, to be reviewed in a follow-up changelist.

Currently the coordinates don't update if the user zooms.  I want to fix that
by using a transformation matrix after this change lands -
http://crrev.com/2047873002 - but scrolling works fine.

Future work might include incremental loading and support for links and maybe
even fill-in forms.

Depends on:
http://crrev.com/1953053002
http://crrev.com/2058453002
http://crrev.com/2050973003
http://crrev.com/2055903002

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

[dmazzoni, 2016-06-09 20:48:54]

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

[raymes, 2016-06-14 03:54:27]

A few suggestions, thanks :)

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pdf_accessibility_tree.cc:14:
PdfAccessibilityTree::PdfAccessibilityTree(
It may be better for someone on the accessibility side to review this file.

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pdf_accessibility_tree.cc:180:
renderer_accessibility_->SetPdfInterface(this);
This looks a bit funny. Could we just do this once at creation?

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:365: 
nit: no newline

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:893: if (accessibility &&
LoadPdfInterface()) {
As you can tell, this file is very badly bloated :) We try to keep a lot of
stuff out of it. In this case, I would suggest changing this code to have only:
...
if (accessibility && LoadPdfInterface())
  plugin_pdf_interface_->EnableAccessibility(pp_instance());

The rest of the code can safely live in the pepper_pdf_host. There will be
exactly one of those things per plugin instance. You can create the
PdfAccessibilityTree inside there. You should be able to get access to the
RendererAccessibility through the RenderFrame which you can access from there,
as well as access to the container for the device scale factor (note that it
should be safe to cast to a PepperPluginInstanceImpl inside PepperPdfHost which
is possibly what was causing you frustration).

Does that make sense roughly?

[raymes, 2016-06-14 03:56:48]

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pdf_accessibility_tree.cc:57: // content/renderer or
should it be in components/pdf?
+1 - with my other suggestions it probably makes sense for this to live
alongside pepper_pdf_host?

[dmazzoni, 2016-06-20 23:55:51]

Moved to components/pdf/renderer and ready for
another look!

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pdf_accessibility_tree.cc:14:
PdfAccessibilityTree::PdfAccessibilityTree(
On 2016/06/14 03:54:27, raymes wrote:
> It may be better for someone on the accessibility side to review this file.

Agreed, I'll ask David Tseng to review this.

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pdf_accessibility_tree.cc:57: // content/renderer or
should it be in components/pdf?
On 2016/06/14 03:56:48, raymes wrote:
> +1 - with my other suggestions it probably makes sense for this to live
> alongside pepper_pdf_host?

Done.

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pdf_accessibility_tree.cc:180:
renderer_accessibility_->SetPdfInterface(this);
On 2016/06/14 03:54:27, raymes wrote:
> This looks a bit funny. Could we just do this once at creation?

For now it needs to be done once the accessibility tree is done.
In a follow-up I'll update it so that we can create it early and
update it every time we finish a page.

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:893: if (accessibility &&
LoadPdfInterface()) {
On 2016/06/14 03:54:27, raymes wrote:
> As you can tell, this file is very badly bloated :) We try to keep a lot of
> stuff out of it. In this case, I would suggest changing this code to have
only:
> ...
> if (accessibility && LoadPdfInterface())
>   plugin_pdf_interface_->EnableAccessibility(pp_instance());
> 
> The rest of the code can safely live in the pepper_pdf_host. There will be
> exactly one of those things per plugin instance. You can create the
> PdfAccessibilityTree inside there. You should be able to get access to the
> RendererAccessibility through the RenderFrame which you can access from there,
> as well as access to the container for the device scale factor (note that it
> should be safe to cast to a PepperPluginInstanceImpl inside PepperPdfHost
which
> is possibly what was causing you frustration).
> 
> Does that make sense roughly?

Done.

[David Tseng, 2016-06-21 16:21:25]

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:64: // TODO: use a localized
string. Should this file even be in
nit: TODO(dmazzoni).

The comment seems outdated.

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:99:
heading_font_size_threshold = median_font_size * 1.2 + 1;
Name these constants

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:137: page_index == 0 &&
first_heading ? 1 : 2);
I don't think changing the heading level makes sense based on whether it's
first.

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:152:
inline_text_box_node->AddStringAttribute(ui::AX_ATTR_NAME, chars_utf8);
Can you break up this function? One reasonable way is to separate each type of
node creation into its own method. It would be easier then to reason about the
in line text box specifically and what attributes (e.g. word offsets) we need.

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:163: if (i == text_runs.size()
- 1) {
Btw, are the text runs in some kind of ordering?

[Lei Zhang, 2016-06-21 17:09:17]

https://codereview.chromium.org/2055953002/diff/100001/components/pdf/rendere...
File components/pdf/renderer/pepper_pdf_host.cc (right):

https://codereview.chromium.org/2055953002/diff/100001/components/pdf/rendere...
components/pdf/renderer/pepper_pdf_host.cc:238: pdf_accessibility_tree_.reset(
Are we going to create a new PdfAccessibilityTree on every message?

[dmazzoni, 2016-06-21 17:17:36]

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:64: // TODO: use a localized
string. Should this file even be in
On 2016/06/21 16:21:25, David Tseng wrote:
> nit: TODO(dmazzoni).
> 
> The comment seems outdated.

Done.

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:99:
heading_font_size_threshold = median_font_size * 1.2 + 1;
On 2016/06/21 16:21:25, David Tseng wrote:
> Name these constants

Done.

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:137: page_index == 0 &&
first_heading ? 1 : 2);
On 2016/06/21 16:21:25, David Tseng wrote:
> I don't think changing the heading level makes sense based on whether it's
> first.

Got rid of this heuristic for now

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:152:
inline_text_box_node->AddStringAttribute(ui::AX_ATTR_NAME, chars_utf8);
On 2016/06/21 16:21:25, David Tseng wrote:
> Can you break up this function? One reasonable way is to separate each type of
> node creation into its own method. It would be easier then to reason about the
> in line text box specifically and what attributes (e.g. word offsets) we need.

It's not easy to break up that way because the nodes are so dependent on one
another, the helper function would have too many arguments and wouldn't be any
more clear. Instead I pulled out three helper functions to cut the size of this
function, is this better?

https://codereview.chromium.org/2055953002/diff/80001/components/pdf/renderer...
components/pdf/renderer/pdf_accessibility_tree.cc:163: if (i == text_runs.size()
- 1) {
On 2016/06/21 16:21:25, David Tseng wrote:
> Btw, are the text runs in some kind of ordering?

The characters of the page are in reading order. The text runs just group those
characters into continuous runs that seem to be going the same direction on one
line with the same font size; the order is unchanged.

https://codereview.chromium.org/2055953002/diff/100001/components/pdf/rendere...
File components/pdf/renderer/pepper_pdf_host.cc (right):

https://codereview.chromium.org/2055953002/diff/100001/components/pdf/rendere...
components/pdf/renderer/pepper_pdf_host.cc:238: pdf_accessibility_tree_.reset(
On 2016/06/21 17:09:17, Lei Zhang wrote:
> Are we going to create a new PdfAccessibilityTree on every message?

Oops! No, meant to do that only once.

[raymes, 2016-06-22 00:02:03]

components/pdf lgtm (modulo content/renderer/pepper/pdf_accessibility_tree.*)
content/renderer/pepper lgtm

Could you also please add yourself as a per-file owner for
content/renderer/pepper/pdf_accessibility_tree.cc?

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2055953002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pdf_accessibility_tree.cc:180:
renderer_accessibility_->SetPdfInterface(this);
On 2016/06/20 23:55:51, dmazzoni wrote:
> On 2016/06/14 03:54:27, raymes wrote:
> > This looks a bit funny. Could we just do this once at creation?
> 
> For now it needs to be done once the accessibility tree is done.
> In a follow-up I'll update it so that we can create it early and
> update it every time we finish a page.

I see, the call site limits this to be calling for the last page only ( I
thought it would be called for every page). That seems reasonable.

[raymes, 2016-06-22 00:02:47]

On 2016/06/22 00:02:03, raymes wrote:
> components/pdf lgtm (modulo content/renderer/pepper/pdf_accessibility_tree.*)
> content/renderer/pepper lgtm
> 
> Could you also please add yourself as a per-file owner for
> content/renderer/pepper/pdf_accessibility_tree.cc?


Sorry - I meant components/pdf/renderer/pdf_accessibility_tree.* (now that it's
moved)

[Lei Zhang, 2016-06-22 00:12:57]

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:59: CHECK(zoom_ > 0);
Are these CHECKs going to stay as CHECKs or become DCHECKS? Prefer CHECK_GT().

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:78: uint32_t char_index = 0;
Put there closer to where it's used.

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:158: static_text = "";
static_text.clear() ?

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:291: return node != NULL;
nullptr here and below

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.h (right):

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.h:31:
PdfAccessibilityTree(content::RenderView* render_view);
explicit

[dmazzoni, 2016-06-22 05:47:32]

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.cc (right):

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:59: CHECK(zoom_ > 0);
On 2016/06/22 00:12:56, Lei Zhang (Slow) wrote:
> Are these CHECKs going to stay as CHECKs or become DCHECKS? Prefer CHECK_GT().

I know that CHECKs are generally discouraged, but my thought is that if there
is a bug somewhere, I'd much rather have a crash log point to a check failure
than a more difficult-to-detect error later. Once this hits stable channel and
seems solid we could replace it with DCHECK.

Switched to CHECK_GT, etc. here and elsewhere.

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:78: uint32_t char_index = 0;
On 2016/06/22 00:12:56, Lei Zhang (Slow) wrote:
> Put there closer to where it's used.

Done.

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:158: static_text = "";
On 2016/06/22 00:12:56, Lei Zhang (Slow) wrote:
> static_text.clear() ?

Done.

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.cc:291: return node != NULL;
On 2016/06/22 00:12:56, Lei Zhang (Slow) wrote:
> nullptr here and below

Done.

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
File components/pdf/renderer/pdf_accessibility_tree.h (right):

https://codereview.chromium.org/2055953002/diff/160001/components/pdf/rendere...
components/pdf/renderer/pdf_accessibility_tree.h:31:
PdfAccessibilityTree(content::RenderView* render_view);
On 2016/06/22 00:12:56, Lei Zhang (Slow) wrote:
> explicit

Done.

[dmazzoni, 2016-06-22 05:49:19]

dmazzoni@chromium.org changed reviewers:
+ jochen@chromium.org

[dmazzoni, 2016-06-22 05:49:19]

+jochen for owners review

[jochen (gone - plz use gerrit), 2016-06-22 13:18:43]

which part exactly do you want me to review?

[dmazzoni, 2016-06-22 16:01:30]

@jochen Sorry, I should have specified! I need
an owners review of these files:

  content/common/*
  content/public/renderer/*
  content/renderer/render_*

The main change is creating a new public interface
RenderAccessibility (so that we can access it
from components/) and renaming
RendererAccessibility -> RenderAccessibilityImpl.

[jochen (gone - plz use gerrit), 2016-06-24 13:37:47]

lgtm

[dmazzoni, 2016-06-24 18:39:18]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-24 18:39:18]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2055953002/#ps180001
(title: "Address more feedback")

[commit-bot: I haz the power, 2016-06-24 18:39:52]

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, 2016-06-24 20:00:44]

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

[commit-bot: I haz the power, 2016-06-24 20:00:46]

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

[dmazzoni, 2016-06-26 20:58:47]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-06-26 20:59:01]

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

[dmazzoni, 2016-06-26 20:59:36]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2016-06-27 05:22:17]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-27 05:22:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2055953002/#ps200001
(title: "Update name of disabled Android test")

[commit-bot: I haz the power, 2016-06-27 05:22:31]

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, 2016-06-27 06:35:57]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-27 06:38:41]

Description was changed from

==========
Make the PDF plug-in accessible.

Makes the PDF plug-in accessible on all supported platforms by building an
accessibility tree for the PDF content and grafting it on as a subtree of
the accessibility tree for the embedding page.

It exposes all of the text of the PDF, including full support for computing
the bounding box of any arbitrary text range.  Using heuristics, automatically
groups the text runs into paragraphs and marks some of them as headings if
their font size is larger than the average for the page.

I wrote a browser test for this, to be reviewed in a follow-up changelist.

Currently the coordinates don't update if the user zooms.  I want to fix that
by using a transformation matrix after this change lands -
http://crrev.com/2047873002 - but scrolling works fine.

Future work might include incremental loading and support for links and maybe
even fill-in forms.

Depends on:
http://crrev.com/1953053002
http://crrev.com/2058453002
http://crrev.com/2050973003
http://crrev.com/2055903002

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

to

==========
Make the PDF plug-in accessible.

Makes the PDF plug-in accessible on all supported platforms by building an
accessibility tree for the PDF content and grafting it on as a subtree of
the accessibility tree for the embedding page.

It exposes all of the text of the PDF, including full support for computing
the bounding box of any arbitrary text range.  Using heuristics, automatically
groups the text runs into paragraphs and marks some of them as headings if
their font size is larger than the average for the page.

I wrote a browser test for this, to be reviewed in a follow-up changelist.

Currently the coordinates don't update if the user zooms.  I want to fix that
by using a transformation matrix after this change lands -
http://crrev.com/2047873002 - but scrolling works fine.

Future work might include incremental loading and support for links and maybe
even fill-in forms.

Depends on:
http://crrev.com/1953053002
http://crrev.com/2058453002
http://crrev.com/2050973003
http://crrev.com/2055903002

BUG=54724

Committed: https://crrev.com/8e6fe4d789f430554f47df5085fa04a6808221c7
Cr-Commit-Position: refs/heads/master@{#402122}
==========

[commit-bot: I haz the power, 2016-06-27 06:38:43]

Patchset 11 (id:??) landed as
https://crrev.com/8e6fe4d789f430554f47df5085fa04a6808221c7
Cr-Commit-Position: refs/heads/master@{#402122}