Add private PPAPI interfaces for PDFium accessibility.

Add private PPAPI interfaces for PDFium accessibility.

This change adds PPAPI interfaces for PDFium accessibility and
implements them in the PDFium engine wrapper code. The approach
to get the accessible text is similar to the existing JSON
interface, which will be deleted once this approach is finished.

Design doc (shared with chromium.org): https://goo.gl/ZJUBsg

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

[dmazzoni, 2016-05-05 17:41:35]

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

[Lei Zhang, 2016-05-09 18:51:35]

There's a lot of red trybots.

[dmazzoni, 2016-05-09 21:07:07]

Sorry about that. Take a look now, I think they'll all be green now.

[Lei Zhang, 2016-05-09 23:58:49]

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2309: return;
Would it be helpful to return false to give the accessibility code some feedback
and let it (maybe?) pass the failure to the user to let them know that a give
PDF does not have the right permissions?

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2326: for (uint32_t page_index = 0; page_index <
pages_.size(); ++page_index) {
size_t instead of uint32_t?

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2345: chars.resize(text_run_info.len);
I think you can combine this with the previous line in C++11 std::vector.

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_page.cc
File pdf/pdfium/pdfium_page.cc (right):

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_page....
pdf/pdfium/pdfium_page.cc:358: bool PDFiumPage::GetTextRunInfo(
Can this be void since it always returns true? The caller doesn't check the
return result either.

[dmazzoni, 2016-05-10 22:09:18]

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2309: return;
On 2016/05/09 23:58:49, Lei Zhang wrote:
> Would it be helpful to return false to give the accessibility code some
feedback
> and let it (maybe?) pass the failure to the user to let them know that a give
> PDF does not have the right permissions?

Good thought.

I moved SetAccessibilityDocInfo up to the top, since that already passes through
the accessibility flags, so we can have that metadata sent either way, then skip
the rest if it's not accessible.

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2326: for (uint32_t page_index = 0; page_index <
pages_.size(); ++page_index) {
On 2016/05/09 23:58:49, Lei Zhang wrote:
> size_t instead of uint32_t?

Done.

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2345: chars.resize(text_run_info.len);
On 2016/05/09 23:58:49, Lei Zhang wrote:
> I think you can combine this with the previous line in C++11 std::vector.

Done.

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_page.cc
File pdf/pdfium/pdfium_page.cc (right):

https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_page....
pdf/pdfium/pdfium_page.cc:358: bool PDFiumPage::GetTextRunInfo(
On 2016/05/09 23:58:49, Lei Zhang wrote:
> Can this be void since it always returns true? The caller doesn't check the
> return result either.

Done.

[raymes, 2016-05-13 05:39:47]

Hey Dominic,

Sorry I've been really slow on this but I'll try to get to it soon.

Thanks,
Raymes

On Wed, 11 May 2016 at 08:09 <dmazzoni@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
> File pdf/pdfium/pdfium_engine.cc (right):
>
>
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
> pdf/pdfium/pdfium_engine.cc:2309: return;
> On 2016/05/09 23:58:49, Lei Zhang wrote:
> > Would it be helpful to return false to give the accessibility code
> some feedback
> > and let it (maybe?) pass the failure to the user to let them know that
> a give
> > PDF does not have the right permissions?
>
> Good thought.
>
> I moved SetAccessibilityDocInfo up to the top, since that already passes
> through the accessibility flags, so we can have that metadata sent
> either way, then skip the rest if it's not accessible.
>
>
>
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
> pdf/pdfium/pdfium_engine.cc:2326: for (uint32_t page_index = 0;
> page_index < pages_.size(); ++page_index) {
> On 2016/05/09 23:58:49, Lei Zhang wrote:
> > size_t instead of uint32_t?
>
> Done.
>
>
>
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
> pdf/pdfium/pdfium_engine.cc:2345: chars.resize(text_run_info.len);
> On 2016/05/09 23:58:49, Lei Zhang wrote:
> > I think you can combine this with the previous line in C++11
> std::vector.
>
> Done.
>
>
>
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_page.cc
> File pdf/pdfium/pdfium_page.cc (right):
>
>
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_page....
> pdf/pdfium/pdfium_page.cc:358: bool PDFiumPage::GetTextRunInfo(
> On 2016/05/09 23:58:49, Lei Zhang wrote:
> > Can this be void since it always returns true? The caller doesn't
> check the
> > return result either.
>
> Done.
>
> https://codereview.chromium.org/1953053002/
>

-- 
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, 2016-05-13 18:07:52]

No problem, thanks! I have plenty of other things to work on in the
meantime. :)


On 2016/05/13 05:39:47, raymes wrote:
> Hey Dominic,
> 
> Sorry I've been really slow on this but I'll try to get to it soon.
> 
> Thanks,
> Raymes
> 
> On Wed, 11 May 2016 at 08:09 <mailto:dmazzoni@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
> > File pdf/pdfium/pdfium_engine.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
> > pdf/pdfium/pdfium_engine.cc:2309: return;
> > On 2016/05/09 23:58:49, Lei Zhang wrote:
> > > Would it be helpful to return false to give the accessibility code
> > some feedback
> > > and let it (maybe?) pass the failure to the user to let them know that
> > a give
> > > PDF does not have the right permissions?
> >
> > Good thought.
> >
> > I moved SetAccessibilityDocInfo up to the top, since that already passes
> > through the accessibility flags, so we can have that metadata sent
> > either way, then skip the rest if it's not accessible.
> >
> >
> >
> >
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
> > pdf/pdfium/pdfium_engine.cc:2326: for (uint32_t page_index = 0;
> > page_index < pages_.size(); ++page_index) {
> > On 2016/05/09 23:58:49, Lei Zhang wrote:
> > > size_t instead of uint32_t?
> >
> > Done.
> >
> >
> >
> >
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_engin...
> > pdf/pdfium/pdfium_engine.cc:2345: chars.resize(text_run_info.len);
> > On 2016/05/09 23:58:49, Lei Zhang wrote:
> > > I think you can combine this with the previous line in C++11
> > std::vector.
> >
> > Done.
> >
> >
> >
> >
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_page.cc
> > File pdf/pdfium/pdfium_page.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1953053002/diff/80001/pdf/pdfium/pdfium_page....
> > pdf/pdfium/pdfium_page.cc:358: bool PDFiumPage::GetTextRunInfo(
> > On 2016/05/09 23:58:49, Lei Zhang wrote:
> > > Can this be void since it always returns true? The caller doesn't
> > check the
> > > return result either.
> >
> > Done.
> >
> > https://codereview.chromium.org/1953053002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[raymes, 2016-05-17 20:38:40]

Sorry for the delay!

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2325:
pp::PDF::SetAccessibilityViewportInfo(GetPluginInstance(), &viewport_info);
Does the viewport info need to be updated as the viewport changes?

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2339: PP_PrivateAccessibilityTextRunInfo
text_run_info;
Could getting all this data be slow for large docs? If so it might hang the
plugin. I'd suggest breaking up the function by posting tasks with a delay. You
can take a look in OutOfProcessInstance::NotifyNumberOfFindResultsChanged for an
example.

An even better approach might be to only send the data that is visible in the
current viewport. That only works if we update on viewport changes though. I'm
not sure if you were planning to do this.

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2359: }
Could this code live in out_of_process_instance.cc (by exposing the necessary
information through the PDFEngine API)? I'd rather not expose the toolbar height
offset to the engine because it's more of a UI detail.

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_page.cc
File pdf/pdfium/pdfium_page.cc (right):

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_page...
pdf/pdfium/pdfium_page.cc:399: *out_bounds = text_run_bounds;
I'll leave this file for thestig@ to review.

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:155: uint32_t page_index,
nit: could this go in PP_PrivateAccessibilityPageInfo?

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:156: struct PP_PrivateAccessibilityPageInfo*
page_info);
I wonder if it's good enough to put all the text runs into the PageInfo (and get
rid of the function which sets the text run info)? Do you think we could end up
with IPCs that are too big?

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:158: void (*SetAccessibilityTextRunInfo)(
nit: no comment

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:163: struct PP_PrivateAccessibilityCharInfo chars[]);
nit: could everything go in PP_PrivateAccessibilityTextRunInfo?

https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppp_pdf_pr...
File ppapi/proxy/ppp_pdf_proxy.h (right):

https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppp_pdf_pr...
ppapi/proxy/ppp_pdf_proxy.h:8: #include <vector>
nit: not needed

[raymes, 2016-05-17 20:40:12]

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2325:
pp::PDF::SetAccessibilityViewportInfo(GetPluginInstance(), &viewport_info);
On 2016/05/17 20:38:40, raymes wrote:
> Does the viewport info need to be updated as the viewport changes?

Also, is the viewport information important for accessibility (I'm guessing so
since you're sending it but I wanted to double check :)

[dmazzoni, 2016-05-17 20:56:58]

Thanks!

I'll address the rest of your feedback, but I would like your
thoughts on complex C structs. As I commented below, I
originally wanted to put everything in PageInfo but the IPC
macros don't seem to have a way to serialize a C array
inside of a struct.

Is there any reason we have to use a C API? Since this is
private and all of the code involved is C++ anyway could
we just define these structs in ppapi/cpp/private
instead?

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2325:
pp::PDF::SetAccessibilityViewportInfo(GetPluginInstance(), &viewport_info);
On 2016/05/17 20:38:40, raymes wrote:
> Does the viewport info need to be updated as the viewport changes?

Yes, I just haven't implemented that yet.

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2325:
pp::PDF::SetAccessibilityViewportInfo(GetPluginInstance(), &viewport_info);
On 2016/05/17 20:40:12, raymes wrote:
> On 2016/05/17 20:38:40, raymes wrote:
> > Does the viewport info need to be updated as the viewport changes?
> 
> Also, is the viewport information important for accessibility (I'm guessing so
> since you're sending it but I wanted to double check :)

Yes, it's important to expose the on-screen location of all of the text - it's
not just for totally blind users, there are tools using accessibility APIs for
low-vision users, etc.

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2339: PP_PrivateAccessibilityTextRunInfo
text_run_info;
On 2016/05/17 20:38:40, raymes wrote:
> Could getting all this data be slow for large docs? If so it might hang the
> plugin. I'd suggest breaking up the function by posting tasks with a delay.
You
> can take a look in OutOfProcessInstance::NotifyNumberOfFindResultsChanged for
an
> example.

I think I'd prefer to do this, maybe one page at a time.

> An even better approach might be to only send the data that is visible in the
> current viewport. That only works if we update on viewport changes though. I'm
> not sure if you were planning to do this.

Screen readers like to buffer the whole document. I think I'd like to try
posting
tasks and see how that works, at a minimum try to buffer more than just the
visible
portion, if not the whole doc.

https://codereview.chromium.org/1953053002/diff/100001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2359: }
On 2016/05/17 20:38:40, raymes wrote:
> Could this code live in out_of_process_instance.cc (by exposing the necessary
> information through the PDFEngine API)? I'd rather not expose the toolbar
height
> offset to the engine because it's more of a UI detail.

Sure. That's one of the reasons I wanted to stop and get this reviewed before
I did things like updating the viewport.

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:156: struct PP_PrivateAccessibilityPageInfo*
page_info);
On 2016/05/17 20:38:40, raymes wrote:
> I wonder if it's good enough to put all the text runs into the PageInfo (and
get
> rid of the function which sets the text run info)? Do you think we could end
up
> with IPCs that are too big?

Actually it just wasn't clear to me how to do this since this is a C API.
The IPC macros didn't seem to handle a C array of structs inside of a
struct, so it seemed cleaner to just use separate IPCs rather than
write custom Read/Write/Log functions for the IPC serializer.

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:163: struct PP_PrivateAccessibilityCharInfo chars[]);
On 2016/05/17 20:38:40, raymes wrote:
> nit: could everything go in PP_PrivateAccessibilityTextRunInfo?

Same - if this was C++ that's what I'd do.

[raymes, 2016-05-17 21:58:13]

> Is there any reason we have to use a C API? Since this is
> private and all of the code involved is C++ anyway could
> we just define these structs in ppapi/cpp/private
> instead?

You're right. We probably could do something smarter here because we ship the
two things together. There's no need for a stable binary API. I'd be hesitant to
do this right now though as this is just the way the ppapi architecture works
and there might be some subtle implications to trying to speak C++.

Thanks!

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:156: struct PP_PrivateAccessibilityPageInfo*
page_info);
On 2016/05/17 20:56:58, dmazzoni wrote:
> On 2016/05/17 20:38:40, raymes wrote:
> > I wonder if it's good enough to put all the text runs into the PageInfo (and
> get
> > rid of the function which sets the text run info)? Do you think we could end
> up
> > with IPCs that are too big?
> 
> Actually it just wasn't clear to me how to do this since this is a C API.
> The IPC macros didn't seem to handle a C array of structs inside of a
> struct, so it seemed cleaner to just use separate IPCs rather than
> write custom Read/Write/Log functions for the IPC serializer.

Ah right. Yeah it is a bit of a headache, I agree. It also means doing more
memory management in the plugin for the complex struct. 

What if the function instead looked like:
SetAccessibilityPageInfo(PageInfo, TextRunInfo[], CharInfo[])

Where each TextRunInfo contains a start_index and end_index into the CharInfo
array?

In my opinion it would be better to consolidate this into one IPC/function call
though as sending the data on a per-page basis seems like a good granularity.

https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:254: 
nit: spaces

[dmazzoni, 2016-05-17 23:49:07]

That sounds good!

On Tue, May 17, 2016 at 2:58 PM <raymes@chromium.org> wrote:

> > Is there any reason we have to use a C API? Since this is
> > private and all of the code involved is C++ anyway could
> > we just define these structs in ppapi/cpp/private
> > instead?
>
> You're right. We probably could do something smarter here because we ship
> the
> two things together. There's no need for a stable binary API. I'd be
> hesitant to
> do this right now though as this is just the way the ppapi architecture
> works
> and there might be some subtle implications to trying to speak C++.
>
> Thanks!
>
>
>
>
>
https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pdf.h
> File ppapi/c/private/ppb_pdf.h (right):
>
>
>
https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
> ppapi/c/private/ppb_pdf.h:156: struct PP_PrivateAccessibilityPageInfo*
> page_info);
> On 2016/05/17 20:56:58, dmazzoni wrote:
> > On 2016/05/17 20:38:40, raymes wrote:
> > > I wonder if it's good enough to put all the text runs into the
> PageInfo (and
> > get
> > > rid of the function which sets the text run info)? Do you think we
> could end
> > up
> > > with IPCs that are too big?
> >
> > Actually it just wasn't clear to me how to do this since this is a C
> API.
> > The IPC macros didn't seem to handle a C array of structs inside of a
> > struct, so it seemed cleaner to just use separate IPCs rather than
> > write custom Read/Write/Log functions for the IPC serializer.
>
> Ah right. Yeah it is a bit of a headache, I agree. It also means doing
> more memory management in the plugin for the complex struct.
>
> What if the function instead looked like:
> SetAccessibilityPageInfo(PageInfo, TextRunInfo[], CharInfo[])
>
> Where each TextRunInfo contains a start_index and end_index into the
> CharInfo array?
>
> In my opinion it would be better to consolidate this into one
> IPC/function call though as sending the data on a per-page basis seems
> like a good granularity.
>
>
>
https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppapi_mess...
> File ppapi/proxy/ppapi_messages.h (right):
>
>
>
https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppapi_mess...
> ppapi/proxy/ppapi_messages.h:254:
> nit: spaces
>
> https://codereview.chromium.org/1953053002/
>
> --
> 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.

[dmazzoni, 2016-05-26 21:59:14]

Ready for another look, thanks!

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:155: uint32_t page_index,
On 2016/05/17 20:38:40, raymes wrote:
> nit: could this go in PP_PrivateAccessibilityPageInfo?

Done.

https://codereview.chromium.org/1953053002/diff/100001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:156: struct PP_PrivateAccessibilityPageInfo*
page_info);
On 2016/05/17 21:58:13, raymes wrote:
> On 2016/05/17 20:56:58, dmazzoni wrote:
> > On 2016/05/17 20:38:40, raymes wrote:
> > > I wonder if it's good enough to put all the text runs into the PageInfo
(and
> > get
> > > rid of the function which sets the text run info)? Do you think we could
end
> > up
> > > with IPCs that are too big?
> > 
> > Actually it just wasn't clear to me how to do this since this is a C API.
> > The IPC macros didn't seem to handle a C array of structs inside of a
> > struct, so it seemed cleaner to just use separate IPCs rather than
> > write custom Read/Write/Log functions for the IPC serializer.
> 
> Ah right. Yeah it is a bit of a headache, I agree. It also means doing more
> memory management in the plugin for the complex struct. 
> 
> What if the function instead looked like:
> SetAccessibilityPageInfo(PageInfo, TextRunInfo[], CharInfo[])
> 
> Where each TextRunInfo contains a start_index and end_index into the CharInfo
> array?
> 
> In my opinion it would be better to consolidate this into one IPC/function
call
> though as sending the data on a per-page basis seems like a good granularity.

Done.

https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:254: 
On 2016/05/17 21:58:13, raymes wrote:
> nit: spaces

Done.

https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppp_pdf_pr...
File ppapi/proxy/ppp_pdf_proxy.h (right):

https://codereview.chromium.org/1953053002/diff/100001/ppapi/proxy/ppp_pdf_pr...
ppapi/proxy/ppp_pdf_proxy.h:8: #include <vector>
On 2016/05/17 20:38:40, raymes wrote:
> nit: not needed

Done.

[raymes, 2016-05-30 04:17:38]

lg with a few comments. Defer to thestig for pdfium/ code.

Thanks!

https://codereview.chromium.org/1953053002/diff/120001/pdf/out_of_process_ins...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/1953053002/diff/120001/pdf/out_of_process_ins...
pdf/out_of_process_instance.cc:684: uint32_t char_index = 0;
int (that's what the argument type is anyhow)?

https://codereview.chromium.org/1953053002/diff/120001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1953053002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2311: CHECK(page_index >= 0 && page_index <
static_cast<int>(pages_.size()));
nit: DCHECK?

https://codereview.chromium.org/1953053002/diff/120001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/1953053002/diff/120001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:73: double char_width;
Would it be any more performant to split these two things into parallel arrays.
e.g. would it be useful to directly have a uint32_t array of characters without
having to unpack them on the renderer side?

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/pdf_resour...
File ppapi/proxy/pdf_resource.cc (right):

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/pdf_resour...
ppapi/proxy/pdf_resource.cc:185: std::vector<PP_PrivateAccessibilityTextRunInfo>
text_run_vector;
You should be able to initialize the vector with the data:
std::vector<...> text_run_vector(text_runs, text_runs +
page_info->text_run_count);

You may want to test how much overhead the extra copy introduces here on very
large documents. It may be worth optimizing to get rid of it but we don't have
to for the first version.

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/pdf_resour...
ppapi/proxy/pdf_resource.cc:190: std::vector<PP_PrivateAccessibilityCharInfo>
char_vector;
nit: same here

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:2458: struct PP_PrivateAccessibilityViewportInfo /*
viewport_info */)
Do we need "struct" here? (and below)

[dmazzoni, 2016-05-31 21:45:02]

https://codereview.chromium.org/1953053002/diff/120001/pdf/out_of_process_ins...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/1953053002/diff/120001/pdf/out_of_process_ins...
pdf/out_of_process_instance.cc:684: uint32_t char_index = 0;
On 2016/05/30 04:17:37, raymes wrote:
> int (that's what the argument type is anyhow)?

Done.

https://codereview.chromium.org/1953053002/diff/120001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1953053002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:2311: CHECK(page_index >= 0 && page_index <
static_cast<int>(pages_.size()));
On 2016/05/30 04:17:37, raymes wrote:
> nit: DCHECK?

Done.

https://codereview.chromium.org/1953053002/diff/120001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/1953053002/diff/120001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:73: double char_width;
On 2016/05/30 04:17:37, raymes wrote:
> Would it be any more performant to split these two things into parallel
arrays.
> e.g. would it be useful to directly have a uint32_t array of characters
without
> having to unpack them on the renderer side?

I agree there are some advantages to having parallel arrays, but I'm positive
this won't be a bottleneck - the total amount of text on a page is not that
large.

The main reason I prefer this design is that I think there's a chance
I'd want to add another per-character field at some point in the
future, and it seems like it'd be a lot easier with this design.

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/pdf_resour...
File ppapi/proxy/pdf_resource.cc (right):

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/pdf_resour...
ppapi/proxy/pdf_resource.cc:185: std::vector<PP_PrivateAccessibilityTextRunInfo>
text_run_vector;
On 2016/05/30 04:17:37, raymes wrote:
> You should be able to initialize the vector with the data:
> std::vector<...> text_run_vector(text_runs, text_runs +
> page_info->text_run_count);
> 
> You may want to test how much overhead the extra copy introduces here on very
> large documents. It may be worth optimizing to get rid of it but we don't have
> to for the first version.

Good idea, done.

Since this is per-page it shouldn't be too bad. I'll definitely
look at some actual numbers when landing the following changes
to finish this feature.

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/pdf_resour...
ppapi/proxy/pdf_resource.cc:190: std::vector<PP_PrivateAccessibilityCharInfo>
char_vector;
On 2016/05/30 04:17:37, raymes wrote:
> nit: same here

Done.

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1953053002/diff/120001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:2458: struct PP_PrivateAccessibilityViewportInfo /*
viewport_info */)
On 2016/05/30 04:17:37, raymes wrote:
> Do we need "struct" here? (and below)

Done.

[raymes, 2016-06-01 00:05:54]

lgtm for everything except pdf/pdfium

Cheers

https://codereview.chromium.org/1953053002/diff/120001/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

https://codereview.chromium.org/1953053002/diff/120001/ppapi/c/private/ppb_pd...
ppapi/c/private/ppb_pdf.h:73: double char_width;
On 2016/05/31 21:45:01, dmazzoni wrote:
> On 2016/05/30 04:17:37, raymes wrote:
> > Would it be any more performant to split these two things into parallel
> arrays.
> > e.g. would it be useful to directly have a uint32_t array of characters
> without
> > having to unpack them on the renderer side?
> 
> I agree there are some advantages to having parallel arrays, but I'm positive
> this won't be a bottleneck - the total amount of text on a page is not that
> large.
> 
> The main reason I prefer this design is that I think there's a chance
> I'd want to add another per-character field at some point in the
> future, and it seems like it'd be a lot easier with this design.

sgtm

[Lei Zhang, 2016-06-01 21:19:40]

Description was changed from

==========
Add private PPAPI interfaces for PDFium accessibility.

This change adds PPAPI interfaces for PDFium accessibility and
implements them in the PDFium engine wrapper code. The approach
to get the accessible text is similar to the existing JSON
interface, which will be deleted once this approach is finished.

Design doc (shared with chromium.org): https://goo.gl/ZJUBsg

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

to

==========
Add private PPAPI interfaces for PDFium accessibility.

This change adds PPAPI interfaces for PDFium accessibility and
implements them in the PDFium engine wrapper code. The approach
to get the accessible text is similar to the existing JSON
interface, which will be deleted once this approach is finished.

Design doc (shared with chromium.org): https://goo.gl/ZJUBsg

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

[Lei Zhang, 2016-06-01 21:19:40]

thestig@chromium.org changed reviewers:
+ tsepez@chromium.org

[Lei Zhang, 2016-06-01 21:26:34]

I'm not done reviewing, but I need to step out for a bit. Here's what I have so
far.

https://codereview.chromium.org/1953053002/diff/140001/pdf/out_of_process_ins...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/1953053002/diff/140001/pdf/out_of_process_ins...
pdf/out_of_process_instance.cc:305: top_toolbar_height_(0),
indentation

https://codereview.chromium.org/1953053002/diff/140001/pdf/out_of_process_ins...
pdf/out_of_process_instance.cc:696: text_run_index++;
We increment |text_run_index| but it's never read.

https://codereview.chromium.org/1953053002/diff/140001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1953053002/diff/140001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:139: IPC_ENUM_TRAITS_MAX_VALUE(PP_PrivateDirection,
PP_PRIVATEDIRECTION_LAST)
+tsepez for IPC.

https://codereview.chromium.org/1953053002/diff/140001/ppapi/thunk/ppb_pdf_api.h
File ppapi/thunk/ppb_pdf_api.h (right):

https://codereview.chromium.org/1953053002/diff/140001/ppapi/thunk/ppb_pdf_ap...
ppapi/thunk/ppb_pdf_api.h:38: = 0;
Is this the "git cl format" output? If not, can you run it and see if the
formatting ends up more like line 35/36?

[Tom Sepez, 2016-06-01 21:29:49]

IPC LGTM

[Lei Zhang, 2016-06-01 22:50:43]

lgtm aside from previous comments.

[dmazzoni, 2016-06-01 22:56:43]

https://codereview.chromium.org/1953053002/diff/140001/pdf/out_of_process_ins...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/1953053002/diff/140001/pdf/out_of_process_ins...
pdf/out_of_process_instance.cc:305: top_toolbar_height_(0),
On 2016/06/01 21:26:33, Lei Zhang wrote:
> indentation

Done.

https://codereview.chromium.org/1953053002/diff/140001/pdf/out_of_process_ins...
pdf/out_of_process_instance.cc:696: text_run_index++;
On 2016/06/01 21:26:33, Lei Zhang wrote:
> We increment |text_run_index| but it's never read.

Done.

https://codereview.chromium.org/1953053002/diff/140001/ppapi/thunk/ppb_pdf_api.h
File ppapi/thunk/ppb_pdf_api.h (right):

https://codereview.chromium.org/1953053002/diff/140001/ppapi/thunk/ppb_pdf_ap...
ppapi/thunk/ppb_pdf_api.h:38: = 0;
On 2016/06/01 21:26:33, Lei Zhang wrote:
> Is this the "git cl format" output? If not, can you run it and see if the
> formatting ends up more like line 35/36?

Fixed, and I applied "git cl format" elsewhere, except for a
few places where "git cl format" didn't seem to do a very good
job like this:

> const PPP_Pdf ppp_private = {
>    &GetLinkAtPosition, &Transform, &GetPrintPresetOptionsFromDocument,
>    &EnableAccessibility,
> };

[dmazzoni, 2016-06-01 22:57:20]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-01 22:57:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, tsepez@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1953053002/#ps160001
(title: "Address feedback and run git cl format")

[commit-bot: I haz the power, 2016-06-01 22:58:33]

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

[commit-bot: I haz the power, 2016-06-02 00:30:11]

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

[commit-bot: I haz the power, 2016-06-02 00:30:13]

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

[dmazzoni, 2016-06-02 04:37:11]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-06-02 04:37:25]

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

[commit-bot: I haz the power, 2016-06-02 05:47:30]

Description was changed from

==========
Add private PPAPI interfaces for PDFium accessibility.

This change adds PPAPI interfaces for PDFium accessibility and
implements them in the PDFium engine wrapper code. The approach
to get the accessible text is similar to the existing JSON
interface, which will be deleted once this approach is finished.

Design doc (shared with chromium.org): https://goo.gl/ZJUBsg

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

to

==========
Add private PPAPI interfaces for PDFium accessibility.

This change adds PPAPI interfaces for PDFium accessibility and
implements them in the PDFium engine wrapper code. The approach
to get the accessible text is similar to the existing JSON
interface, which will be deleted once this approach is finished.

Design doc (shared with chromium.org): https://goo.gl/ZJUBsg

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

[commit-bot: I haz the power, 2016-06-02 05:47:31]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-02 05:49:30]

Description was changed from

==========
Add private PPAPI interfaces for PDFium accessibility.

This change adds PPAPI interfaces for PDFium accessibility and
implements them in the PDFium engine wrapper code. The approach
to get the accessible text is similar to the existing JSON
interface, which will be deleted once this approach is finished.

Design doc (shared with chromium.org): https://goo.gl/ZJUBsg

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

to

==========
Add private PPAPI interfaces for PDFium accessibility.

This change adds PPAPI interfaces for PDFium accessibility and
implements them in the PDFium engine wrapper code. The approach
to get the accessible text is similar to the existing JSON
interface, which will be deleted once this approach is finished.

Design doc (shared with chromium.org): https://goo.gl/ZJUBsg

BUG=54724

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

[commit-bot: I haz the power, 2016-06-02 05:49:31]

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