Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744423002:
Handle large rects better. (Closed)
|
Created:
3 years, 9 months ago by Peter Mayo Modified:
3 years, 8 months ago CC:
chromium-reviews, cc-bugs_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle large rects better.
Handling rects that are essentially infinite in one or two directions
is not symmetric because we use an assymetric representation for
many. We have to be careful not to overflow our representation(s)
around these to keep as much meaningful state as possible.
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2744423002
Cr-Commit-Position: refs/heads/master@{#460848}
Committed: https://chromium.googlesource.com/chromium/src/+/9711f036f202f53ae0bfa6bb6195c3e61ecae274
Patch Set 1 : Handle large rects better. #
Total comments: 3
Patch Set 2 : Followup fixes #
Total comments: 3
Patch Set 3 : rebase #Patch Set 4 : Add ARM code branch #
Total comments: 1
Patch Set 5 : typo #Patch Set 6 : typo #Patch Set 7 : comments, pointers over refs, and tweaks #Patch Set 8 : Rebased, Use HasHeight, Symmetric test #
Total comments: 2
Patch Set 9 : Self comments #Patch Set 10 : TestPass and rebase #Patch Set 11 : Fix borderline unit test particular result #
Total comments: 1
Patch Set 12 : Rebased on size patch #
Total comments: 23
Patch Set 13 : Typos. Rebased. #
Total comments: 11
Patch Set 14 : Review Feedback #
Total comments: 7
Patch Set 15 : Rebased #Patch Set 16 : Nits #
Total comments: 2
Patch Set 17 : Add comment and change constant reference. #
Depends on Patchset: Messages
Total messages: 49 (27 generated)
Description was changed from ========== Handle large rects better. Handling rects that are essentially infinite in one or two directions is not symmetric because we use an assymetric representation for many. We have to be careful not to overflow our representation(s) around these to keep as much meaningful state as possible. BUG=None ========== to ========== Handle large rects better. Handling rects that are essentially infinite in one or two directions is not symmetric because we use an assymetric representation for many. We have to be careful not to overflow our representation(s) around these to keep as much meaningful state as possible. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Patchset #1 (id:1) has been deleted
add SKiRecttoRect https://codereview.chromium.org/2744423002/diff/20001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:167: if (rdx >= std::numeric_limits<int>::max() / 2) { Maybe just on equality. https://codereview.chromium.org/2744423002/diff/20001/ui/gfx/geometry/rect_co... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2744423002/diff/20001/ui/gfx/geometry/rect_co... ui/gfx/geometry/rect_conversions.cc:21: int height = base::SaturatedSubtraction(max_y, min_y); move into set from range in header
danakj@chromium.org changed reviewers: + danakj@chromium.org
Thanks, I'll wait for you to publish again after addressing the feedback we noted.
Published again. https://codereview.chromium.org/2744423002/diff/20001/ui/gfx/geometry/rect_co... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2744423002/diff/20001/ui/gfx/geometry/rect_co... ui/gfx/geometry/rect_conversions.cc:93: return Rect(min_x, min_y, max_x - min_x, max_y - min_y); Oops - missed this in the previous CL. https://codereview.chromium.org/2744423002/diff/40001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/40001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:78: if (span_loss == 0) { This condition neatly summarizes that we have to punt. It leverages the speed and accuracy of base::SaturatedSubtraction. https://codereview.chromium.org/2744423002/diff/40001/ui/gfx/geometry/rect_un... File ui/gfx/geometry/rect_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/40001/ui/gfx/geometry/rect_un... ui/gfx/geometry/rect_unittest.cc:500: {static_cast<float>(min_int), static_cast<float>(min_int), max_int * 2.f, ICK - git cl upload insisted this should be reformatted this way. https://codereview.chromium.org/2744423002/diff/40001/ui/gfx/platform_font_li... File ui/gfx/platform_font_linux_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/40001/ui/gfx/platform_font_li... ui/gfx/platform_font_linux_unittest.cc:73: PlatformFontLinux::ReloadDefaultFont(); Oops; this snuck in from https://codereview.chromium.org/2760463003/ It's landing, will disappear on the next rebase.
Rebased - PTAL
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2744423002/diff/80001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/80001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:63: // This is the per-axis heuristic for picking the cwmost useful origin and Typo
Comments below are handled, and rebased on top of the size handling CL. https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:16: // in the enclosing int rect. Remove comment - Not relevant any more. https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:81: ToFlooredInt(rect.height())); This should use the improved heuristic too I suppose.
On 2017/03/23 00:01:31, Peter Mayo wrote: > Comments below are handled, and rebased on top of the size handling CL. > > https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... > File ui/gfx/geometry/rect_conversions.cc (right): > > https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... > ui/gfx/geometry/rect_conversions.cc:16: // in the enclosing int rect. > Remove comment - Not relevant any more. > > https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... > ui/gfx/geometry/rect_conversions.cc:81: ToFlooredInt(rect.height())); > This should use the improved heuristic too I suppose. So I'm debugging the unittest failures that this causes now. In particular, one asserts that the visible rect in layer space of a node turn by 90.0 degrees is empty - but this CL fixes that based on the numerical error, the image of the root screen rect projected to the plane of the node is {origin_ = {x_ = -1.11544431e+18, y_ = -118.30127}, size_ = {width_ = 4.4617775e+18, height_ = 273.205078}}, which before this did not intersect with ((0,0), (100, 100)) but now it does. Ergo we are relying on the seemingly buggy behavior in the structure of places I don't understand. (ComputeTargetRectInLocalSpace, LayerVisibleRect in draw_property_utils.cc) The image of the layer rect through the forwards transform is likely quite small, and may have triggered the trivial dimension code (100 being a very small portion of 4.4617775e+18)
On 2017/03/24 00:45:45, Peter Mayo wrote: > On 2017/03/23 00:01:31, Peter Mayo wrote: > > Comments below are handled, and rebased on top of the size handling CL. > > > > > https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... > > File ui/gfx/geometry/rect_conversions.cc (right): > > > > > https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... > > ui/gfx/geometry/rect_conversions.cc:16: // in the enclosing int rect. > > Remove comment - Not relevant any more. > > > > > https://codereview.chromium.org/2744423002/diff/160001/ui/gfx/geometry/rect_c... > > ui/gfx/geometry/rect_conversions.cc:81: ToFlooredInt(rect.height())); > > This should use the improved heuristic too I suppose. > > So I'm debugging the unittest failures that this causes now. In particular, one LayerTreeHostCommonTest_LayerFullyContainedWithinClipInTargetSpace
On Thu, Mar 23, 2017 at 8:45 PM, <petermayo@chromium.org> wrote: > On 2017/03/23 00:01:31, Peter Mayo wrote: > > Comments below are handled, and rebased on top of the size handling CL. > > > > > https://codereview.chromium.org/2744423002/diff/160001/ui/ > gfx/geometry/rect_conversions.cc > > File ui/gfx/geometry/rect_conversions.cc (right): > > > > > https://codereview.chromium.org/2744423002/diff/160001/ui/ > gfx/geometry/rect_conversions.cc#newcode16 > > ui/gfx/geometry/rect_conversions.cc:16: // in the enclosing int rect. > > Remove comment - Not relevant any more. > > > > > https://codereview.chromium.org/2744423002/diff/160001/ui/ > gfx/geometry/rect_conversions.cc#newcode81 > > ui/gfx/geometry/rect_conversions.cc:81: ToFlooredInt(rect.height())); > > This should use the improved heuristic too I suppose. > > So I'm debugging the unittest failures that this causes now. In > particular, one > asserts that the visible rect in layer space of a node turn by 90.0 > degrees is > empty - but this CL fixes that based on the numerical error, the image of > the > root screen rect projected to the plane of the node is {origin_ = {x_ = > -1.11544431e+18, y_ = -118.30127}, size_ = {width_ = 4.4617775e+18, height_ > = 273.205078}}, which before this did not intersect with ((0,0), (100, > 100)) but > now it does. > > Ergo we are relying on the seemingly buggy behavior in the structure of > places I > don't understand. (ComputeTargetRectInLocalSpace, LayerVisibleRect in > draw_property_utils.cc) > > The image of the layer rect through the forwards transform is likely quite > small, and may have triggered the trivial dimension code (100 being a very > small > portion of 4.4617775e+18) > I would suggest going thru git history to see what CL added the test in the first place, read the CL description and comments, and code changes to determine what it is actually trying to test. That will help you decide what the right change is. -- 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.
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/24 19:53:02, danakj wrote: > ... > > I would suggest going thru git history to see what CL added the test in the > first place, read the CL description and comments, and code changes to > determine what it is actually trying to test. That will help you decide > what the right change is. > Thanks, This was underway when I received this and I've copied you on the out-of-band communication to bring the previous authors up to speed. I'm going to add a reviewer to record responses to my specific suggestion here.
Description was changed from ========== Handle large rects better. Handling rects that are essentially infinite in one or two directions is not symmetric because we use an assymetric representation for many. We have to be careful not to overflow our representation(s) around these to keep as much meaningful state as possible. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Handle large rects better. Handling rects that are essentially infinite in one or two directions is not symmetric because we use an assymetric representation for many. We have to be careful not to overflow our representation(s) around these to keep as much meaningful state as possible. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
petermayo@chromium.org changed reviewers: + jaydasika@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2744423002/diff/220001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/220001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:1129: // the clip, and is only empty so land as the numerical precision of the s/land/long/
cc/trees/layer_tree_host_common_unittest.cc lgtm https://codereview.chromium.org/2744423002/diff/240001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:1131: // way around, and the Projection of the screen space clip into layer space nit : the 'other' way around ?
https://codereview.chromium.org/2744423002/diff/240001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:1129: // the clip, and is only empty so land as the numerical precision of the s/land/long/ Done. https://codereview.chromium.org/2744423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:1131: // way around, and the Projection of the screen space clip into layer space On 2017/03/28 20:11:48, jaydasika wrote: > nit : the 'other' way around ? Done.
Thanks the comments are very good, a few small details to polish but I like the change overall. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:174: SetRect(0, 0, 0, 0); // Throw away empty position. nit: Throws https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:178: int leftCoordinate = std::max(x(), rect.x()); chromium style please. also just "left" "top" etc would work if you want, that isn't ambiguous https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:184: SetRect(0, 0, 0, 0); // Throw away empty position. nit: Throws https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:113: // Safely(heuristically) take min and max to the internal representation. I get what this is talking about but I'm not really sure what the comment is saying exactly. I think SetByBounds() is pretty self explanatory and how its implemented is a detail, I would drop the comment https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:114: void SetByBounds(int left, int right, int top, int bottom); Can you reorder these as left, top, right, bottom? That will match SkRect, and then they are clockwise. I wanted to find one that is most common but SkRect, gfx::Insets, and css margins all do a different ordering. At least we can match one of them here instead of making a 4th ordering. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_c... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:17: int right = r.size().width() ? ToCeiledInt(r.right()) : left; r.width() https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:19: int bottom = r.size().height() ? ToCeiledInt(r.bottom()) : top; r.height() https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_u... File ui/gfx/geometry/rect_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_u... ui/gfx/geometry/rect_unittest.cc:429: #define MAYBE_EXPECT_INT_EQ(a, b) \ This is pretty magical, instead of this, can you just break these cases out of the for loops and don't check the things that don't matter (maybe with a comment why)? https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_u... ui/gfx/geometry/rect_unittest.cc:1057: EXPECT_TRUE(UnionRects(left_minint, right_maxint).Contains(expected)); Can you (also?) verify the width and height exactly, and the x and y are within some range? https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/skrect_conversi... File ui/gfx/skrect_conversion_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/skrect_conversi... ui/gfx/skrect_conversion_unittest.cc:37: SkIRectToRect(SkIRect::MakeLTRB(Limits::min(), Limits::min(), This would be more readable (less line-wrappy) if you put the result into a local Rect var before comparing https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:72: *span = base::SaturatedSubtraction(max, min); can you work in local variables until here until you get your final answer, then put them into *origin and *span before returning? https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:84: // to zero we choose to make sure it is represented precisely. choose it https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:92: // both are big, so keep the center. Both
Addressed, rebased and ready to go? https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:174: SetRect(0, 0, 0, 0); // Throw away empty position. On 2017/03/28 20:40:34, danakj wrote: > nit: Throws Done. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:178: int leftCoordinate = std::max(x(), rect.x()); On 2017/03/28 20:40:34, danakj wrote: > chromium style please. also just "left" "top" etc would work if you want, that > isn't ambiguous right and bottom are ambiguous with the this-> methods. Made them more chromium style anyway. Done. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:184: SetRect(0, 0, 0, 0); // Throw away empty position. On 2017/03/28 20:40:34, danakj wrote: > nit: Throws Done. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:113: // Safely(heuristically) take min and max to the internal representation. On 2017/03/28 20:40:34, danakj wrote: > I get what this is talking about but I'm not really sure what the comment is > saying exactly. I think SetByBounds() is pretty self explanatory and how its > implemented is a detail, I would drop the comment I wanted it there to highlight to devs that there it doesn't work to just pass (top, left, right-left, bottom-top) to SetRect, nor to assert equality after calling this. I'm OK with dropping it, but I think it incrementally improved velocity. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:114: void SetByBounds(int left, int right, int top, int bottom); On 2017/03/28 20:40:34, danakj wrote: > Can you reorder these as left, top, right, bottom? That will match SkRect, and > then they are clockwise. I wanted to find one that is most common but SkRect, > gfx::Insets, and css margins all do a different ordering. At least we can match > one of them here instead of making a 4th ordering. I kind of like Skia's practice of putting the order in the name, tickling in place recognition of the order. It would look out pf place in gfx at the moment, especially the constructor. Just reordered. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_c... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:17: int right = r.size().width() ? ToCeiledInt(r.right()) : left; On 2017/03/28 20:40:35, danakj wrote: > r.width() Done. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:19: int bottom = r.size().height() ? ToCeiledInt(r.bottom()) : top; On 2017/03/28 20:40:35, danakj wrote: > r.height() Done. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_u... File ui/gfx/geometry/rect_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_u... ui/gfx/geometry/rect_unittest.cc:429: #define MAYBE_EXPECT_INT_EQ(a, b) \ On 2017/03/28 20:40:35, danakj wrote: > This is pretty magical, instead of this, can you just break these cases out of > the for loops and don't check the things that don't matter (maybe with a comment > why)? Done. Increased the structure of the tests while I was at it. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/geometry/rect_u... ui/gfx/geometry/rect_unittest.cc:1057: EXPECT_TRUE(UnionRects(left_minint, right_maxint).Contains(expected)); On 2017/03/28 20:40:35, danakj wrote: > Can you (also?) verify the width and height exactly, and the x and y are within > some range? Done. https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/skrect_conversi... File ui/gfx/skrect_conversion_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/240001/ui/gfx/skrect_conversi... ui/gfx/skrect_conversion_unittest.cc:37: SkIRectToRect(SkIRect::MakeLTRB(Limits::min(), Limits::min(), On 2017/03/28 20:40:35, danakj wrote: > This would be more readable (less line-wrappy) if you put the result into a > local Rect var before comparing Done.
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the test changes, they look nicer now! https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:72: *span = base::SaturatedSubtraction(max, min); On 2017/03/28 20:40:35, danakj wrote: > can you work in local variables until here until you get your final answer, then > put them into *origin and *span before returning? There's a few comments here that I don't see addressed, were they missed? https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:84: // to zero we choose to make sure it is represented precisely. On 2017/03/28 20:40:35, danakj wrote: > choose it This also. https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:92: // both are big, so keep the center. On 2017/03/28 20:40:35, danakj wrote: > Both And this https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:113: void SetByBounds(int left, int top, int right, int bottom); I see what you're saying about the comment that was here. Maybe we can just make it a bit more clear if you want, I propose: // Use in place of SetRect() when you know the edges of the rectangle instead of the dimensions, rather than trying to determine the width/height yourself. This safely handles cases where the width/height would overflow. https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect_u... File ui/gfx/geometry/rect_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect_u... ui/gfx/geometry/rect_unittest.cc:513: RectF source(min_int_f, min_int_f, max_int_f * 3.f, max_int * 3.f); nit: did you mean to use max_int_f once and max_int the other? It reads intentional but they are the same, so I suggest max_int_f thru the whole thing. https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect_u... ui/gfx/geometry/rect_unittest.cc:573: RectF source(min_int_f, min_int_f, max_int_f * 3.f, max_int * 3.f); same here
https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:72: *span = base::SaturatedSubtraction(max, min); On 2017/03/28 20:40:35, danakj wrote: > can you work in local variables until here until you get your final answer, then > put them into *origin and *span before returning? origin is only ever set just before returning, and span is never modified once set here. I can definitely see keeping a local var to avoid the readback. Are you really sure you want to see a lcoal var for origin just to have an extra copy once we've decided how to set it? https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:72: *span = base::SaturatedSubtraction(max, min); On 2017/03/29 19:00:47, danakj wrote: > On 2017/03/28 20:40:35, danakj wrote: > > can you work in local variables until here until you get your final answer, > then > > put them into *origin and *span before returning? > > There's a few comments here that I don't see addressed, were they missed? Yes - the comments ended up on a different patchset(260001), so they got missed when I was working my way through the ones on 240001. https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:84: // to zero we choose to make sure it is represented precisely. On 2017/03/29 19:00:47, danakj wrote: > On 2017/03/28 20:40:35, danakj wrote: > > choose it > > This also. I think that might be a misleading way to phrase the comment. Shorter is even better. https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:92: // both are big, so keep the center. On 2017/03/29 19:00:47, danakj wrote: > On 2017/03/28 20:40:35, danakj wrote: > > Both > > And this Done.
LGTM https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/260001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:72: *span = base::SaturatedSubtraction(max, min); On 2017/03/29 20:42:48, Peter Mayo wrote: > On 2017/03/28 20:40:35, danakj wrote: > > can you work in local variables until here until you get your final answer, > then > > put them into *origin and *span before returning? > > origin is only ever set just before returning, and span is never modified once > set here. I can definitely see keeping a local var to avoid the readback. Are > you really sure you want to see a lcoal var for origin just to have an extra > copy once we've decided how to set it? Yeah, I just mean variables that are used/read i.e. span https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:113: void SetByBounds(int left, int top, int right, int bottom); On 2017/03/29 19:00:47, danakj wrote: > I see what you're saying about the comment that was here. Maybe we can just make > it a bit more clear if you want, I propose: > > // Use in place of SetRect() when you know the edges of the rectangle instead of > the dimensions, rather than trying to determine the width/height yourself. This > safely handles cases where the width/height would overflow. Idk if u didn't see this also, so in case you want it. https://codereview.chromium.org/2744423002/diff/320001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/320001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:84: // to zero we choose to represent that one precisely. The other side is good change
Last patch will be up in a few moments, just recompiling & running unittests to make sure I didn't hypercomment. https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:113: void SetByBounds(int left, int top, int right, int bottom); On 2017/03/29 20:50:35, danakj wrote: > On 2017/03/29 19:00:47, danakj wrote: > > I see what you're saying about the comment that was here. Maybe we can just > make > > it a bit more clear if you want, I propose: > > > > // Use in place of SetRect() when you know the edges of the rectangle instead > of > > the dimensions, rather than trying to determine the width/height yourself. > This > > safely handles cases where the width/height would overflow. > > Idk if u didn't see this also, so in case you want it. I didn't. I like it - added. https://codereview.chromium.org/2744423002/diff/320001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2744423002/diff/320001/ui/gfx/geometry/rect.c... ui/gfx/geometry/rect.cc:84: // to zero we choose to represent that one precisely. The other side is On 2017/03/29 20:50:35, danakj wrote: > good change Thank you.
https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect_u... File ui/gfx/geometry/rect_unittest.cc (right): https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect_u... ui/gfx/geometry/rect_unittest.cc:513: RectF source(min_int_f, min_int_f, max_int_f * 3.f, max_int * 3.f); On 2017/03/29 19:00:47, danakj wrote: > nit: did you mean to use max_int_f once and max_int the other? It reads > intentional but they are the same, so I suggest max_int_f thru the whole thing. Done. https://codereview.chromium.org/2744423002/diff/280001/ui/gfx/geometry/rect_u... ui/gfx/geometry/rect_unittest.cc:573: RectF source(min_int_f, min_int_f, max_int_f * 3.f, max_int * 3.f); On 2017/03/29 19:00:47, danakj wrote: > same here Done.
The CQ bit was checked by petermayo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jaydasika@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2744423002/#ps340001 (title: "Add comment and change constant reference.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1490894232674680,
"parent_rev": "bdd654be5032b499f6cf442da2b2fb3edf8eafb0", "commit_rev":
"9711f036f202f53ae0bfa6bb6195c3e61ecae274"}
Message was sent while issue was closed.
Description was changed from ========== Handle large rects better. Handling rects that are essentially infinite in one or two directions is not symmetric because we use an assymetric representation for many. We have to be careful not to overflow our representation(s) around these to keep as much meaningful state as possible. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Handle large rects better. Handling rects that are essentially infinite in one or two directions is not symmetric because we use an assymetric representation for many. We have to be careful not to overflow our representation(s) around these to keep as much meaningful state as possible. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2744423002 Cr-Commit-Position: refs/heads/master@{#460848} Committed: https://chromium.googlesource.com/chromium/src/+/9711f036f202f53ae0bfa6bb6195... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:340001) as https://chromium.googlesource.com/chromium/src/+/9711f036f202f53ae0bfa6bb6195... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
