Add GetScrollOffset function to PPB_View

Add GetScrollOffset function to PPB_View

This adds a function to PPB_View which allows plugins to know the scroll offset
of the page when they are in view. This is useful for OOP PDF which uses the
scroll offset of the window it is contained in to determine the document's
scroll location. A web page can send scroll location via postMessage but
this is slow. Sending the offset directly via view messages is much faster
and seems reasonable.

We don't send the scroll offset in the cases where the plugin is off screen
to avoid any more additional IPC traffic than what currently exists.

BUG=303491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278236

[raymes, 2014-06-11 06:54:53]

Hey, could you give me your thoughts as to whether this is reasonable? If it's
ok I'll send a note to pepper-dev to get wider feedback before submitting (it
probably doesn't warrant a full API document).

[dmichael (off chromium), 2014-06-11 20:24:15]

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl
File ppapi/api/ppb_view.idl (right):

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl#n...
ppapi/api/ppb_view.idl:16: M37 = 1.2
It might be best to use channel=dev to start out, while you experiment with it?

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl#n...
ppapi/api/ppb_view.idl:200: * plugin. This value will only be valid if the
module instance is visible
Can you get the same information from the position you get from GetRect combined
with the visible part from GetClipRect?

E.g.:
PP_Rect rect = {};
view->GetRect(view_resource, &rect);
PP_Rect clip_rect = {};
view->GetClipRect(view_resource, &clip_rect);
PP_Point scroll_offset = {rect.point.x + clip_rect.point.x,
                          rect.point.y + clip_rect.point.y};
^^^ Would that you give you the same information? Or am I missing something?

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl#n...
ppapi/api/ppb_view.idl:207: * of the scroll offset.
in CSS pixels?

[raymes, 2014-06-12 01:06:15]

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl
File ppapi/api/ppb_view.idl (right):

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl#n...
ppapi/api/ppb_view.idl:16: M37 = 1.2
On 2014/06/11 20:24:14, dmichael wrote:
> It might be best to use channel=dev to start out, while you experiment with
it?

Done.

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl#n...
ppapi/api/ppb_view.idl:200: * plugin. This value will only be valid if the
module instance is visible
I think this works except for the cases where the plugin instance is position:
fixed (as in pdf) and it doesn't move with the rest of the window contents :(

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl#n...
ppapi/api/ppb_view.idl:207: * of the scroll offset.
On 2014/06/11 20:24:14, dmichael wrote:
> in CSS pixels?

Done.

[darin (slow to review), 2014-06-12 03:45:29]

Can you elaborate on what makes this method of communicating scroll offset so
much faster than postMessage?

[raymes, 2014-06-12 04:47:46]

> Can you elaborate on what makes this method of communicating scroll offset
> so
> much faster than postMessage?

I haven't spent any time profiling the performance but my guess is as follows:
postMessage requires the scroll offset going through JavaScript. In
other words, When a user scrolls the web page's onscroll handler must
first be called, then postMessage will get called from JS. Then we
convert the v8 value into a var and transmit it over IPC.

This is contrasted with the sending a PP_Point directly in an IPC message.

The performance difference between the two approaches is definitely
noticeable even on high end machines. I can definitely dig into the
details of exactly where the extra cost is coming from if you want
more details before moving ahead with this?


On Thu, Jun 12, 2014 at 1:45 PM,  <darin@chromium.org> wrote:
> Can you elaborate on what makes this method of communicating scroll offset
> so
> much faster than postMessage?
>
> https://codereview.chromium.org/329033003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[raymes, 2014-06-16 02:52:31]

Friendly ping :)

On Thu, Jun 12, 2014 at 2:47 PM, Raymes Khoury <raymes@chromium.org> wrote:
>> Can you elaborate on what makes this method of communicating scroll offset
>> so
>> much faster than postMessage?
>
> I haven't spent any time profiling the performance but my guess is as
follows:
> postMessage requires the scroll offset going through JavaScript. In
> other words, When a user scrolls the web page's onscroll handler must
> first be called, then postMessage will get called from JS. Then we
> convert the v8 value into a var and transmit it over IPC.
>
> This is contrasted with the sending a PP_Point directly in an IPC message.
>
> The performance difference between the two approaches is definitely
> noticeable even on high end machines. I can definitely dig into the
> details of exactly where the extra cost is coming from if you want
> more details before moving ahead with this?
>
>
> On Thu, Jun 12, 2014 at 1:45 PM,  <darin@chromium.org> wrote:
>> Can you elaborate on what makes this method of communicating scroll offset
>> so
>> much faster than postMessage?
>>
>> https://codereview.chromium.org/329033003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-06-16 22:31:56]

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl
File ppapi/api/ppb_view.idl (right):

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl#n...
ppapi/api/ppb_view.idl:200: * plugin. This value will only be valid if the
module instance is visible
On 2014/06/12 01:06:15, raymes wrote:
> I think this works except for the cases where the plugin instance is position:
> fixed (as in pdf) and it doesn't move with the rest of the window contents :(
Oh, you're using position:fixed? I had thought you were doing something else,
where you tell Graphics2D which part of the plugin's drawing area your ImageData
applies to. That way you'd be able to render more than is displayed to make it
possible for a small scroll to happen without roundtripping to the plugin.

Did you move away from that strategy? Does this mean you have to roundtrip to
the plugin for any scroll?

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

https://codereview.chromium.org/329033003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1207: // Only set the
scroll offset if the plugin is visible.
Why have this restriction?

https://codereview.chromium.org/329033003/diff/40001/ppapi/tests/test_view.cc
File ppapi/tests/test_view.cc (right):

https://codereview.chromium.org/329033003/diff/40001/ppapi/tests/test_view.cc...
ppapi/tests/test_view.cc:215: ASSERT_TRUE(last_view_.GetScrollOffset() ==
pp::Point(5, 1));
Does ASSERT_EQ work? Not sure since I added the logging of values...

[raymes, 2014-06-17 00:43:00]

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl
File ppapi/api/ppb_view.idl (right):

https://codereview.chromium.org/329033003/diff/20001/ppapi/api/ppb_view.idl#n...
ppapi/api/ppb_view.idl:200: * plugin. This value will only be valid if the
module instance is visible
Yeah...unfortunately I put that strategy to the side for now. The problem is
that the compositor support really isn't there yet. If you scroll too quickly
with that approach then we get flickering/checkerboarding because there is no
way at the moment for the compositor to wait for the plugin to deliver a frame.

The requestAnimationFrame pepper API (that Brian and Peng proposed) would make
it possible to go back down this route and get really nice scrolling in PDF, but
for now I've gone with a similar approach to what exists with in-process... :(

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

https://codereview.chromium.org/329033003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1207: // Only set the
scroll offset if the plugin is visible.
Originally I had this because I thought we might end up spamming the plugin with
more messages than usual.

But now that I think about it, it will be getting view change messages as the
value of GetRect changes when the window is scrolled anyway, so there won't be
much difference.

I've removed the check, thanks!

https://codereview.chromium.org/329033003/diff/40001/ppapi/tests/test_view.cc
File ppapi/tests/test_view.cc (right):

https://codereview.chromium.org/329033003/diff/40001/ppapi/tests/test_view.cc...
ppapi/tests/test_view.cc:215: ASSERT_TRUE(last_view_.GetScrollOffset() ==
pp::Point(5, 1));
On 2014/06/16 22:31:56, dmichael wrote:
> Does ASSERT_EQ work? Not sure since I added the logging of values...

Done.

[dmichael (off chromium), 2014-06-17 20:26:02]

lgtm

[raymes, 2014-06-18 03:46:49]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-06-18 03:50:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/329033003/60001

[commit-bot: I haz the power, 2014-06-18 10:08:54]

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

[commit-bot: I haz the power, 2014-06-18 10:08:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[raymes, 2014-06-18 23:41:52]

+tsepez for ppapi_messages

[Tom Sepez, 2014-06-19 00:53:58]

Messages LGTM.

[raymes, 2014-06-19 01:35:57]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-06-19 01:39:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/329033003/60001

[commit-bot: I haz the power, 2014-06-19 01:48:19]

Change committed as 278236