New issue
Advanced search Search tips

Issue 638981 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

LayoutRect/LayoutSize/LayoutPoint construction with mixed int and LayoutUnit parameters will implicitly convert LayoutUnit to int

Project Member Reported by wangxianzhu@chromium.org, Aug 18 2016

Issue description

Currently we have implicit conversion from LayoutUnit to int which is lossy, but explicit conversion from int to LayoutUnit.

In many cases, the intent of the lossy conversion is obvious, for example:
  int intValue = layoutUnitValue.

However, in the following statement, the result is likely unexpected by the author:
  LayoutRect r(LayoutUnit(0.5), LayoutUnit(0.5), 0, 0);

As conversion from LayoutUnit to int is implicit, the above statement will match the LayoutRect(int, int, int, int) constructor which is likely not the intention of the author.

LayoutSize, LayoutPoint and any other classes having overloaded constructors and/or methods accepting all LayoutUnit parameters and all int parameters also have the problem. 

I'd like to mark LayoutUnit::operator int() and LayoutUnit::operator unsigned() as deleted to avoid unexpected implicit conversion. The original code where the implicit conversion would need to use LayoutUnit::toInt() or LayoutUnit::toUnsigned() explicitly.

However, in the CL doing the above, because I'm not sure what the actual intent of the LayoutRect constructor with mixed typed parameters, and I don't want to introduce any behavior change in the CL, I'd like to use toInt() and add TODO(this bug) at the sites.
 

Comment 1 by e...@chromium.org, Aug 18 2016

This seems like a great idea, we've been slowly moving towards removing all of the implicit conversions for a few years now.
Sorry for the English grammar errors in the report. Should have reviewed more carefully and fixed them before submitting. I guess you all understood it :)
Cc: schenney@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 22 2016

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

commit 7d87acb1c709a75cc6859bd75960744ee612d7bf
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Mon Aug 22 23:28:17 2016

Disallow cast/implicit conversion from LayoutUnit to int/unsigned

Conversion from LayoutUnit to int/unsigned is lossy. Allowing implicit
conversion also causes unexpected conversion when LayoutUnits and ints
are mixed in invocation of overloaded functions which have both the
all-LayoutUnit and all-int variations: the all-int variation is chosen
instead of the all-LayoutUnit one, which might not the intention of
the author.

BUG= 638981 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2261663002
Cr-Commit-Position: refs/heads/master@{#413578}

[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/editing/Editor.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/events/MouseRelatedEvent.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/events/MouseRelatedEvent.h
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/frame/FrameViewAutoSizeInfo.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/html/forms/ImageInputType.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutButton.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutFieldset.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutFileUploadControl.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutFrameSet.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutInline.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutReplaced.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutRubyBase.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutRubyRun.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutRubyText.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutScrollbar.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutScrollbarPart.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutSlider.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTable.h
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTableCell.h
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutText.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTextControl.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTheme.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutTreeAsText.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/TableLayoutAlgorithmFixed.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/line/EllipsisBox.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/line/InlineBox.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/line/InlineBox.h
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/line/InlineFlowBox.h
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/line/LineBoxList.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/line/RootInlineBox.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/layout/shapes/RasterShape.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/page/FocusController.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/page/SpatialNavigation.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/BoxPainter.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/EllipsisBoxPainter.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/NinePieceImageGrid.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/PaintLayer.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/TableSectionPainter.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/ThemePainterDefault.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/paint/ViewPainter.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/style/ComputedStyle.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/platform/LayoutUnit.h
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/platform/LengthFunctions.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/platform/geometry/IntRect.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/web/RotationViewportAnchor.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/web/WebInputEventConversion.cpp
[modify] https://crrev.com/7d87acb1c709a75cc6859bd75960744ee612d7bf/third_party/WebKit/Source/web/WebPluginContainerImpl.cpp

Status: Fixed (was: Assigned)

Sign in to add a comment