New issue
Advanced search Search tips

Issue 860535 link

Starred by 1 user
Status: Fixed
Owner:
brat...@opera.com
Closed: Jul 27
Cc:
pdr@chromium.org
f...@opera.com
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Forbid implicit conversion from IntRect to FloatRect, IntPoint to FloatPoint

Project Member Reported by brat...@opera.com, Jul 5 
After noticing some accidental conversions via FloatRect that were completely hidden for the one reading the source, it seems best to remove the implicit conversion. Since float is not a subset of int so there is a risk of dataloss when converting. Most of the time it works fine though so there is no critical issue with that.

A larger issue with the auto conversions is that they contribute to the confusion about what types should be used which may make code use the wrong types or the wrong APIs.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 9 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f844435c6086446e8104b631fdfc4649af4ac696

commit f844435c6086446e8104b631fdfc4649af4ac696
Author: Daniel Bratell <bratell@opera.com>
Date: Mon Jul 09 10:21:58 2018

Forbid implicit conversions from IntRect to FloatRect

This is a followup from removing some accidental conversions via FloatRect. Since
FloatRect is not a perfect superset of IntRect, converting from IntRect to FloatRect is
potentially lossy, and conversions that happen by accident hide that APIs are not used
as intended.

A few new helper functions that use IntRect were added to make the change smaller, but
there is potential for future further cleaning up of what types are used where, and how.

Behaviour has been kept where it's not obviously a mistake. Truncating or snapping to
integers is sometimes beneficial.

R=fs@opera.com, pdr@chromium.org

Bug:  860535 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I733b4273b847654f22bae726255cfb75ba8023bb
Reviewed-on: https://chromium-review.googlesource.com/1124454
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#573263}
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/clipboard/data_transfer.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/editing/finder/find_in_page_coordinates.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/inspector/inspector_dom_snapshot_agent.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/layout/layout_box_model_object.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/layout/shapes/shape.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/block_painter_test.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/box_painter_base.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/clip_path_clipper_test.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/image_painter.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/list_marker_painter.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/nine_piece_image_painter.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/theme_painter_default.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/paint/theme_painter_mac.mm
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/core/svg/graphics/svg_image.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/platform/geometry/float_quad.h
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/platform/geometry/float_rect.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/platform/geometry/float_rect.h
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/platform/geometry/float_rounded_rect.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/platform/geometry/float_rounded_rect.h
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/platform/graphics/graphics_context.cc
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/platform/graphics/graphics_context.h
[modify] https://crrev.com/f844435c6086446e8104b631fdfc4649af4ac696/third_party/blink/renderer/platform/graphics/image.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5

commit 7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5
Author: Daniel Bratell <bratell@opera.com>
Date: Tue Jul 10 10:16:11 2018

Forbid implicit conversions from IntPoint to FloatPoint

This is a followup from an analogous change for IntRect and FloatRect which was triggered
by some unintended conversions happening.
Since FloatPoint is not a perfect superset of IntPoint, converting from IntPoint to
FloatPoint is potentially lossy, and conversions that happen by accident hide that APIs
are not used as intended.

Behaviour has been kept where it's not obviously a mistake. Truncating or snapping to
integers is sometimes beneficial.

R=fs@opera.com, pdr@chromium.org

Bug:  860535 

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I84e72a890dbddc2a9f40bb25a000f7eec75f6ffa
Reviewed-on: https://chromium-review.googlesource.com/1126396
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#573675}
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/editing/finder/find_in_page_coordinates.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/exported/web_plugin_container_impl.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/exported/web_remote_frame_impl.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/frame/rotation_viewport_anchor.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/frame/visual_viewport_test.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/input/event_handler.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/input/mouse_event_manager.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/inspector/inspector_highlight.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/inspector/inspector_trace_events.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/layout/layout_tree_as_text.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/layout/line/inline_text_box.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/page/drag_controller_test.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/page/touch_adjustment.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/paint/html_canvas_painter.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/paint/link_highlight_impl.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/paint/list_marker_painter.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/paint/theme_painter_mac.mm
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/core/paint/video_painter.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/platform/geometry/float_point.h
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/platform/graphics/filters/fe_convolve_matrix.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/platform/graphics/graphics_context.cc
[modify] https://crrev.com/7ef85471df8874dcda4eadb3c5e7b1c9dbd686a5/third_party/blink/renderer/platform/graphics/paint/raster_invalidator_test.cc

Comment 3 by brat...@opera.com, Jul 27

Status: Fixed (was: Assigned)
It is now forbidden.
►

Sign in to add a comment