Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10704198:
Scale to DIPs in ppb_graphics2d_impl for proper invalidation (Closed)
|
Created:
8 years, 5 months ago by Josh Horwich Modified:
8 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionScale to DIPs in ppb_graphics2d_impl for proper invalidation
If a Pepper plugin uses PaintImageData or Scroll in a pp::Graphics2D context
that has a scale factor, we need to convert the invalidated area to DIPs.
BUG=137163
TEST=Manual - run HiDPI-compatible plugin that uses PaintImageData
TEST=gfx_unittests --gtest_filter="Rect*"
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147952
Patch Set 1 #
Total comments: 11
Patch Set 2 : Make generic ScaleBounds method, handle scroll properly #
Total comments: 4
Patch Set 3 : Separate function for computation, add unit test #Patch Set 4 : Move ScaleBounds back out of gfx::Rect #
Total comments: 12
Patch Set 5 : Tweaks based on review feedback #Patch Set 6 : Skip math in common (scale=1) case #Patch Set 7 : Separate bounds scale from delta scale #
Total comments: 8
Patch Set 8 : Explicitly include ui/gfx/rect #Patch Set 9 : Fix shared_lib build, simplify based on feedback and fix nits #
Total comments: 4
Patch Set 10 : Move things to ConvertToLogicalPixels #Patch Set 11 : Remove dead code #
Total comments: 1
Patch Set 12 : Fix a nit #
Messages
Total messages: 23 (0 generated)
https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:353: // Set |nothing_visible| to false if the change overlaps the visible area. nit: This comment belongs with the if statement below. You probably want a comment here to clarify why you are scaling |op_rect| here. https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:370: bound_instance_->ScrollRect(operation.scroll_dx, operation.scroll_dy, Do you need to scale the scroll dx & dy values as well?
Thank you for the early/fast feedback. https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:353: // Set |nothing_visible| to false if the change overlaps the visible area. On 2012/07/13 21:29:57, Wez wrote: > nit: This comment belongs with the if statement below. You probably want a > comment here to clarify why you are scaling |op_rect| here. Agree with the nit - will fix so comment stays with relevant code. I am potentially going to move this bounds computation somewhere more common (will check with oshima if it's ok to put in gfx::Rect since it might be generally useful, not just for pepper) - in that (newer) version of the patch I do explain the reason for scaling |op_rect|... https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:370: bound_instance_->ScrollRect(operation.scroll_dx, operation.scroll_dy, On 2012/07/13 21:29:57, Wez wrote: > Do you need to scale the scroll dx & dy values as well? Likely I do actually - thank you for noticing this. I will also need to think a bit about what to do if the delta*scale is non-integral (which for Invalidate wasn't a big deal since "rounding outward" is ok). Hmmmm.
https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:355: left = floor(op_rect.x() * scale_); Do we need to consider the impact of introducing float multiplications here, or can we safely assume all our target architectures have H/W FP-multiply? https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:370: bound_instance_->ScrollRect(operation.scroll_dx, operation.scroll_dy, On 2012/07/13 22:01:23, Josh Horwich wrote: > On 2012/07/13 21:29:57, Wez wrote: > > Do you need to scale the scroll dx & dy values as well? > > Likely I do actually - thank you for noticing this. > I will also need to think a bit about what to do if the delta*scale is > non-integral (which for Invalidate wasn't a big deal since "rounding outward" is > ok). Hmmmm. By the time we reach here we've already "executed" the scroll in the back-buffer, so if the scroll dx/dy end up non-integer then you can fall back to InvalidateRect(). If we're expecting to usually see 1x and 2x zooms then in practice we'll never hit that case.
Updated based on feedback, and I split the computation of the bounds rect out since it could be more generally useful (+added relevant unittest). https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:353: // Set |nothing_visible| to false if the change overlaps the visible area. On 2012/07/13 22:01:23, Josh Horwich wrote: > On 2012/07/13 21:29:57, Wez wrote: > > nit: This comment belongs with the if statement below. You probably want a > > comment here to clarify why you are scaling |op_rect| here. > > Agree with the nit - will fix so comment stays with relevant code. > > I am potentially going to move this bounds computation somewhere more common > (will check with oshima if it's ok to put in gfx::Rect since it might be > generally useful, not just for pepper) - in that (newer) version of the patch I > do explain the reason for scaling |op_rect|... Done. https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:355: left = floor(op_rect.x() * scale_); On 2012/07/16 17:41:43, Wez wrote: > Do we need to consider the impact of introducing float multiplications here, or > can we safely assume all our target architectures have H/W FP-multiply? Looking elsewhere (e.g. ui/gfx) I see lots of FP multiply / divides occurring. However, I'll only do any of it if scale_ isn't 1.0 as my later patch does other computations that are only relevant for non-identity scale. https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:370: bound_instance_->ScrollRect(operation.scroll_dx, operation.scroll_dy, On 2012/07/16 17:41:43, Wez wrote: > On 2012/07/13 22:01:23, Josh Horwich wrote: > > On 2012/07/13 21:29:57, Wez wrote: > > > Do you need to scale the scroll dx & dy values as well? > > > > Likely I do actually - thank you for noticing this. > > I will also need to think a bit about what to do if the delta*scale is > > non-integral (which for Invalidate wasn't a big deal since "rounding outward" > is > > ok). Hmmmm. > > By the time we reach here we've already "executed" the scroll in the > back-buffer, so if the scroll dx/dy end up non-integer then you can fall back to > InvalidateRect(). If we're expecting to usually see 1x and 2x zooms then in > practice we'll never hit that case. Actually, I believe we will hit this case reasonably often (when device_scale is 2x, the scale between context and logical pixels/DIPs is .5, so a plugin scrolling by "5 device pixels" would hit this, or scrolling an area that doesn't exactly align on integral 'logical pixels' in origin or size). Anyway, I've added code in the new patchset to fall back to Invalidate in any of those cases.
https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:357: right = ceil((op_rect.x() + op_rect.width()) * scale_); Out of interest, why does Graphics2D even need an explicit scale to be specified? We know the context and view dimensions in pixels, so can't we just scale using those? https://chromiumcodereview.appspot.com/10704198/diff/1/webkit/plugins/ppapi/p... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:370: bound_instance_->ScrollRect(operation.scroll_dx, operation.scroll_dy, On 2012/07/16 23:26:52, Josh Horwich wrote: > On 2012/07/16 17:41:43, Wez wrote: > > On 2012/07/13 22:01:23, Josh Horwich wrote: > > > On 2012/07/13 21:29:57, Wez wrote: > > > > Do you need to scale the scroll dx & dy values as well? > > > > > > Likely I do actually - thank you for noticing this. > > > I will also need to think a bit about what to do if the delta*scale is > > > non-integral (which for Invalidate wasn't a big deal since "rounding > outward" > > is > > > ok). Hmmmm. > > > > By the time we reach here we've already "executed" the scroll in the > > back-buffer, so if the scroll dx/dy end up non-integer then you can fall back > to > > InvalidateRect(). If we're expecting to usually see 1x and 2x zooms then in > > practice we'll never hit that case. > > Actually, I believe we will hit this case reasonably often (when device_scale is > 2x, the scale between context and logical pixels/DIPs is .5, so a plugin > scrolling by "5 device pixels" would hit this, or scrolling an area that doesn't > exactly align on integral 'logical pixels' in origin or size). Anyway, I've > added code in the new patchset to fall back to Invalidate in any of those cases. True; you really need to fix the underlying implementation to use device pixels throughout so that just the interfaces have to do grotty coordinate conversion.
Adding reviewers - please take a look sky for ui (let me know if you think it's ok to add the general ScaleBounds utlity to gfx::Rect, or if I should just do it inline in PPB_Graphics2D_Impl::Flush - for now that's the only user but I think it could be useful eleswhere) brettw for webkit/plugins/pepper
https://chromiumcodereview.appspot.com/10704198/diff/3002/webkit/plugins/ppap... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): https://chromiumcodereview.appspot.com/10704198/diff/3002/webkit/plugins/ppap... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:353: // Convert op_rect from context coordinates to logical pixels, taking care This seems kind of complicated. Can you separate this into a separate function and add a unit test? You'll need to add a new unit test file and add it to the test_shell_tests target in webkit/tools/test_shell/test_shell.gypi (this will be much easier than writing a normal pepper test for this internal feature, you can see there are a few other unit test files using this scheme).
https://chromiumcodereview.appspot.com/10704198/diff/3002/ui/gfx/rect.h File ui/gfx/rect.h (right): https://chromiumcodereview.appspot.com/10704198/diff/3002/ui/gfx/rect.h#newco... ui/gfx/rect.h:51: void ScaleBounds(float scale); rect_base already has Scale(float). Is there a reason that doesn't work?
Please take a look https://chromiumcodereview.appspot.com/10704198/diff/3002/ui/gfx/rect.h File ui/gfx/rect.h (right): https://chromiumcodereview.appspot.com/10704198/diff/3002/ui/gfx/rect.h#newco... ui/gfx/rect.h:51: void ScaleBounds(float scale); On 2012/07/17 22:12:12, sky wrote: > rect_base already has Scale(float). Is there a reason that doesn't work? Yes, that method scales the origin and size independently, and the conversion float->int occurs with a simple static_cast<int> so it will round down. So if scale is 0.5 (typical on a 2x density display like a Mac Retina when converting from device pixels to logical pixels/DIPs) and the rectangle in question has origin (9, 9) and size (10, 10) we'd want the result to be origin (4, 4) and size (6, 6) when calculating invalid rects in the lower-res (logical pixel) coordinate system. Using the existing Scale would result in origin (4, 4) but size (5, 5) and we'd end up not covering the lower-right line of pixels. :( I didn't change Scale itself in fear of opening up a few thousand cans worth of worms. I moved this here since I thought it might be more generally useful for others - but I don't feel strongly that it should be here - if you wish I can move it back into the code that specifically needs it (ppb_graphics_2d_impl.cc in my case) https://chromiumcodereview.appspot.com/10704198/diff/3002/webkit/plugins/ppap... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): https://chromiumcodereview.appspot.com/10704198/diff/3002/webkit/plugins/ppap... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:353: // Convert op_rect from context coordinates to logical pixels, taking care On 2012/07/17 19:27:50, brettw wrote: > This seems kind of complicated. Can you separate this into a separate function > and add a unit test? You'll need to add a new unit test file and add it to the > test_shell_tests target in webkit/tools/test_shell/test_shell.gypi (this will be > much easier than writing a normal pepper test for this internal feature, you can > see there are a few other unit test files using this scheme). Done.
On Tue, Jul 17, 2012 at 3:53 PM, <jhorwich@chromium.org> wrote: > Please take a look > > > > https://chromiumcodereview.appspot.com/10704198/diff/3002/ui/gfx/rect.h > File ui/gfx/rect.h (right): > > https://chromiumcodereview.appspot.com/10704198/diff/3002/ui/gfx/rect.h#newco... > ui/gfx/rect.h:51: void ScaleBounds(float scale); > On 2012/07/17 22:12:12, sky wrote: >> >> rect_base already has Scale(float). Is there a reason that doesn't > > work? > > Yes, that method scales the origin and size independently, and the > conversion float->int occurs with a simple static_cast<int> so it will > round down. So if scale is 0.5 (typical on a 2x density display like a > Mac Retina when converting from device pixels to logical pixels/DIPs) > and the rectangle in question has origin (9, 9) and size (10, 10) we'd > want the result to be origin (4, 4) and size (6, 6) when calculating > invalid rects in the lower-res (logical pixel) coordinate system. Using > the existing Scale would result in origin (4, 4) but size (5, 5) and > we'd end up not covering the lower-right line of pixels. :( > > I didn't change Scale itself in fear of opening up a few thousand cans > worth of worms. > > I moved this here since I thought it might be more generally useful for > others - but I don't feel strongly that it should be here - if you wish > I can move it back into the code that specifically needs it > (ppb_graphics_2d_impl.cc in my case) I would rather see you fix Scale. -Scott > > https://chromiumcodereview.appspot.com/10704198/diff/3002/webkit/plugins/ppap... > File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): > > https://chromiumcodereview.appspot.com/10704198/diff/3002/webkit/plugins/ppap... > webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:353: // Convert op_rect > from context coordinates to logical pixels, taking care > On 2012/07/17 19:27:50, brettw wrote: >> >> This seems kind of complicated. Can you separate this into a separate > > function >> >> and add a unit test? You'll need to add a new unit test file and add > > it to the >> >> test_shell_tests target in webkit/tools/test_shell/test_shell.gypi > > (this will be >> >> much easier than writing a normal pepper test for this internal > > feature, you can >> >> see there are a few other unit test files using this scheme). > > > Done. > > https://chromiumcodereview.appspot.com/10704198/
Please take a look As per an offline discussion with sky, I've moved the bounds-specific computation back to ppb_graphics_2d_impl.cc rather than change gfx::Rect::Scale.
This means you no longer need me as a reviewer. I've removed myself.
My 2c. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:127: // fully contain the original. nit: Consider expressing this in terms of the goal, being that the scaled rectangle fully contains the area of the scaled output "affected by" the original rectangle. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:128: void ScaleRectBounds(gfx::Rect& rect, float scale) { nit: gfx::Rect ScaleRectBounds(const gfx::Rect& ...) http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:368: // be described in logical pixels due to rounding errors typo: missing . http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:376: ScaleMetrics(scale_, op_rect, NULL, NULL); nit: Use ScaleRectBounds explicitly here. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:660: static_cast<int>(*dy / scale) != original_dy) It feels like it'd be cleaner to keep the ScaleRect and ScaleDelta parts in separate functions rather than mix the two like this. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.h (right): http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.h:78: static bool ScaleMetrics(float scale, gfx::Rect& rect, int* dx, int* dy); gfx::Rect& -> gfx::Rect* since it's an in/out-parameter.
Please take a look http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:127: // fully contain the original. On 2012/07/19 01:33:37, Wez wrote: > nit: Consider expressing this in terms of the goal, being that the scaled > rectangle fully contains the area of the scaled output "affected by" the > original rectangle. Done. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:128: void ScaleRectBounds(gfx::Rect& rect, float scale) { On 2012/07/19 01:33:37, Wez wrote: > nit: gfx::Rect ScaleRectBounds(const gfx::Rect& ...) Done. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:368: // be described in logical pixels due to rounding errors On 2012/07/19 01:33:37, Wez wrote: > typo: missing . Done. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:376: ScaleMetrics(scale_, op_rect, NULL, NULL); On 2012/07/19 01:33:37, Wez wrote: > nit: Use ScaleRectBounds explicitly here. Done. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:660: static_cast<int>(*dy / scale) != original_dy) On 2012/07/19 01:33:37, Wez wrote: > It feels like it'd be cleaner to keep the ScaleRect and ScaleDelta parts in > separate functions rather than mix the two like this. Done. I put the ScaleDelta parts back inline above. http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.h (right): http://codereview.chromium.org/10704198/diff/10001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.h:78: static bool ScaleMetrics(float scale, gfx::Rect& rect, int* dx, int* dy); On 2012/07/19 01:33:37, Wez wrote: > gfx::Rect& -> gfx::Rect* since it's an in/out-parameter. Done.
http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:361: scroll = true; Is the float multiplication overhead sufficient to justify a special-case for SCROLL but not for PAINT? Or should the scale_ test be outside this scaling block? http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:369: operation.scroll_dy = delta_scaled.y(); nit: Rather than copy the point back into the operation, why not copy the scroll fields to a gfx::Point |scroll_delta|, and simplify this to scroll_delta = original_delta.Scale(scale_)? http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:371: original_delta == delta_scaled.Scale(inverse_scale)) nit: Put {} around the body of multi-line-condition ifs. http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:646: // fully contain the entire area affected by the original rectangle. nit: Move this comment to the header, since it's a part of the class's public API.
please take a look - updates based on review feedback, and fixed shared_lib build http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:361: scroll = true; On 2012/07/20 00:01:41, Wez wrote: > Is the float multiplication overhead sufficient to justify a special-case for > SCROLL but not for PAINT? Or should the scale_ test be outside this scaling > block? Done. This motivated me to refactor this section a bit so it should be simpler to understand as well (I hope). http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:369: operation.scroll_dy = delta_scaled.y(); On 2012/07/20 00:01:41, Wez wrote: > nit: Rather than copy the point back into the operation, why not copy the scroll > fields to a gfx::Point |scroll_delta|, and simplify this to scroll_delta = > original_delta.Scale(scale_)? Done. http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:371: original_delta == delta_scaled.Scale(inverse_scale)) On 2012/07/20 00:01:41, Wez wrote: > nit: Put {} around the body of multi-line-condition ifs. Done. http://codereview.chromium.org/10704198/diff/21001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:646: // fully contain the entire area affected by the original rectangle. On 2012/07/20 00:01:41, Wez wrote: > nit: Move this comment to the header, since it's a part of the class's public > API. Done.
Looks good - Brett, you want to take a look? http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:369: scroll_valid = false; You could just override operation.type to PAINT and avoid the need for |scroll_valid|.
http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:354: gfx::Point scroll_delta(operation.scroll_dx, operation.scroll_dy); When I suggested a different function, I was hoping to get all of this block out and add a test for it. It makes the flow of this function harder to follow, rather than just the rounding (which I'm less concerned about). Maybe you can just use some in/out params and take Wez's suggestion of converting the type to PAINT. bool ConvertToLogicalPixels(float scale, QueuedOperation::Type* type, gfx::Rect* op_rect, gfx::Point* scroll_delta);
On 2012/07/20 20:56:45, brettw wrote: > http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... > File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): > > http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... > webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:354: gfx::Point > scroll_delta(operation.scroll_dx, operation.scroll_dy); > When I suggested a different function, I was hoping to get all of this block out > and add a test for it. It makes the flow of this function harder to follow, > rather than just the rounding (which I'm less concerned about). > > Maybe you can just use some in/out params and take Wez's suggestion of > converting the type to PAINT. > > bool ConvertToLogicalPixels(float scale, QueuedOperation::Type* type, gfx::Rect* > op_rect, gfx::Point* scroll_delta); That's actually a lot like an earlier version of the patch, actually; apologies for leading you on a goose-chase, Josh...
Please take a look (or given the goose comment, should I say, "please take a gander"?) http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:354: gfx::Point scroll_delta(operation.scroll_dx, operation.scroll_dy); On 2012/07/20 20:56:45, brettw wrote: > When I suggested a different function, I was hoping to get all of this block out > and add a test for it. It makes the flow of this function harder to follow, > rather than just the rounding (which I'm less concerned about). > > Maybe you can just use some in/out params and take Wez's suggestion of > converting the type to PAINT. > > bool ConvertToLogicalPixels(float scale, QueuedOperation::Type* type, gfx::Rect* > op_rect, gfx::Point* scroll_delta); Done. I didn't include the in/out of QueuedOperation::Type since that would mean moving QueuedOperation into the header - but the impl of Flush manages fallback-to-Invalidate using Wez's suggestion. http://codereview.chromium.org/10704198/diff/24001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:369: scroll_valid = false; On 2012/07/20 05:31:16, Wez wrote: > You could just override operation.type to PAINT and avoid the need for > |scroll_valid|. Done.
lgtm http://codereview.chromium.org/10704198/diff/28001/webkit/plugins/ppapi/ppb_g... File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right): http://codereview.chromium.org/10704198/diff/28001/webkit/plugins/ppapi/ppb_g... webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:369: // Conversion requires falling back to InvalidateRect Nit: need period.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jhorwich@chromium.org/10704198/21006
Change committed as 147952 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
