Polish non-testing area. Up next: testing polish

carlos-zamora · carlos-zamora · commit 4571d42ba55e · 2020-01-30T14:00:25.000-08:00
diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp
@@ -1105,7 +1105,6 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w
     }
 }
 
-
 // Method Description:
 // - Helper method for GetWordEnd(). Get the COORD for the beginning of the next READABLE word
 // Arguments:
diff --git a/src/cascadia/TerminalControl/XamlUiaTextRange.cpp b/src/cascadia/TerminalControl/XamlUiaTextRange.cpp
@@ -5,6 +5,10 @@
 #include "XamlUiaTextRange.h"
 #include "UiaTextRange.hpp"
 
+// the same as COR_E_NOTSUPPORTED
+// we don't want to import the CLR headers to get it
+#define XAML_E_NOT_SUPPORTED 0x80131515L
+
 namespace UIA
 {
     using ::ITextRangeProvider;
@@ -24,18 +28,6 @@ namespace XamlAutomation
 
 namespace winrt::Microsoft::Terminal::TerminalControl::implementation
 {
-    // EmptyObject is our equivalent of UIAutomationCore::UiaGetReservedNotSupportedValue()
-    // This retrieves a value that is interpreted as "not supported".
-    class EmptyObject : public winrt::implements<EmptyObject, IInspectable>
-    {
-    public:
-        static IInspectable GetInstance()
-        {
-            static auto empty = make_self<EmptyObject>();
-            return *empty;
-        }
-    };
-
     XamlAutomation::ITextRangeProvider XamlUiaTextRange::Clone() const
     {
         UIA::ITextRangeProvider* pReturn;
@@ -100,9 +92,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         }
         else
         {
-            // We _need_ to return an empty object here.
+            // We _need_ to return XAML_E_NOT_SUPPORTED here.
             // Returning nullptr is an improper implementation of it being unsupported.
-            return EmptyObject::GetInstance();
+            winrt::throw_hresult(XAML_E_NOT_SUPPORTED);
         }
     }
 
diff --git a/src/inc/LibraryIncludes.h b/src/inc/LibraryIncludes.h
@@ -70,7 +70,10 @@
 #include <CppCoreCheck/Warnings.h>
 
 // Chromium Numerics (safe math)
+#pragma warning(push)
+#pragma warning(disable:4100) // unreferenced parameter
 #include <base/numerics/safe_math.h>
+#pragma warning(pop)
 
 // IntSafe
 #define ENABLE_INTSAFE_SIGNED_FUNCTIONS
diff --git a/src/types/ScreenInfoUiaProviderBase.cpp b/src/types/ScreenInfoUiaProviderBase.cpp
@@ -352,11 +352,11 @@ IFACEMETHODIMP ScreenInfoUiaProviderBase::GetVisibleRanges(_Outptr_result_mayben
     }
 
     // stuff each visible line in the safearray
-    for (SHORT i = 0; i < rowCount; ++i)
+    for (short i = 0; i < rowCount; ++i)
     {
         // end is exclusive so add 1
-        const COORD start { viewport.Left(), viewport.Top() + i };
-        const COORD end { start.X, start.Y + 1 };
+        const COORD start{ viewport.Left(), viewport.Top() + i };
+        const COORD end{ start.X, start.Y + 1 };
 
         HRESULT hr = S_OK;
         WRL::ComPtr<UiaTextRangeBase> range;
diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp
@@ -239,8 +239,8 @@ IFACEMETHODIMP UiaTextRangeBase::Compare(_In_opt_ ITextRangeProvider* pRange, _O
     if (other)
     {
         *pRetVal = (_start == other->GetEndpoint(TextPatternRangeEndpoint_Start) &&
-                      _end == other->GetEndpoint(TextPatternRangeEndpoint_End) &&
-                      IsDegenerate() == other->IsDegenerate());
+                    _end == other->GetEndpoint(TextPatternRangeEndpoint_End) &&
+                    IsDegenerate() == other->IsDegenerate());
     }
     // TODO GitHub #1914: Re-attach Tracing to UIA Tree
     // tracing
@@ -314,14 +314,15 @@ IFACEMETHODIMP UiaTextRangeBase::ExpandToEnclosingUnit(_In_ TextUnit unit)
         else if (unit <= TextUnit::TextUnit_Word)
         {
             // expand to word
-            _start = buffer.GetWordStart(_start, _wordDelimiters, /*accessibilityMode*/ true);
-            _end = buffer.GetWordEnd(_start, _wordDelimiters, /*accessibilityMode*/ true);
+            _start = buffer.GetWordStart(_start, _wordDelimiters, true);
+            _end = buffer.GetWordEnd(_start, _wordDelimiters, true);
         }
         else if (unit <= TextUnit::TextUnit_Line)
         {
             // expand to line
             _start.X = 0;
-            _end.X = base::ClampAdd(_start.Y, 1);
+            _end.X = 0;
+            _end.Y = base::ClampAdd(_start.Y, 1);
         }
         else
         {
@@ -411,12 +412,11 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_
         // _end is exclusive, let's be inclusive so we don't have to think about it anymore for bounding rects
         bufferSize.DecrementInBounds(endAnchor, true);
 
-        if (IsDegenerate())
-        {
-            _getBoundingRect(_start, _start, coords);
-        }
-        else if (bufferSize.CompareInBounds(_start, viewportEnd, true) > 0 || bufferSize.CompareInBounds(_end, viewportOrigin, true) < 0)
+        if (IsDegenerate() || bufferSize.CompareInBounds(_start, viewportEnd, true) > 0 || bufferSize.CompareInBounds(_end, viewportOrigin, true) < 0)
         {
+            // An empty array is returned for a degenerate (empty) text range or for a text range
+            // reference: https://docs.microsoft.com/en-us/windows/win32/api/uiautomationclient/nf-uiautomationclient-iuiautomationtextrange-getboundingrectangles
+
             // Remember, start cannot be past end, so
             //   if start is past the viewport end,
             //   or end is past the viewport origin
@@ -515,8 +515,8 @@ IFACEMETHODIMP UiaTextRangeBase::GetText(_In_ int maxLength, _Out_ BSTR* pRetVal
             // if _end is at 0, we ignore that row because _end is exclusive
             const auto& buffer = _pData->GetTextBuffer();
             const SHORT totalRowsInRange = (_end.X == buffer.GetSize().Left()) ?
-                                                      _end.Y - _start.Y :
-                                                      _end.Y - _start.Y + static_cast<SHORT>(1);
+                                               _end.Y - _start.Y :
+                                               _end.Y - _start.Y + static_cast<SHORT>(1);
             const SHORT lastRowInRange = _start.Y + totalRowsInRange - static_cast<SHORT>(1);
 
             SHORT currentScreenInfoRow = 0;
@@ -811,11 +811,11 @@ IFACEMETHODIMP UiaTextRangeBase::ScrollIntoView(_In_ BOOL alignToTop)
         const auto oldViewport = _pData->GetViewport().ToInclusive();
         const auto viewportHeight = _getViewportHeight(oldViewport);
         // range rows
-        const auto startScreenInfoRow = _start.Y;
-        const auto endScreenInfoRow = _end.Y;
+        const base::ClampedNumeric<short> startScreenInfoRow = _start.Y;
+        const base::ClampedNumeric<short> endScreenInfoRow = _end.Y;
         // screen buffer rows
-        const auto topRow = 0;
-        const auto bottomRow = _pData->GetTextBuffer().TotalRowCount() - 1;
+        const base::ClampedNumeric<short> topRow = 0;
+        const base::ClampedNumeric<short> bottomRow = _pData->GetTextBuffer().TotalRowCount() - 1;
 
         SMALL_RECT newViewport = oldViewport;
 
@@ -827,15 +827,15 @@ IFACEMETHODIMP UiaTextRangeBase::ScrollIntoView(_In_ BOOL alignToTop)
             if (startScreenInfoRow + viewportHeight <= bottomRow)
             {
                 // we can align to the top
-                newViewport.Top = gsl::narrow<SHORT>(startScreenInfoRow);
-                newViewport.Bottom = gsl::narrow<SHORT>(startScreenInfoRow + viewportHeight - 1);
+                newViewport.Top = startScreenInfoRow;
+                newViewport.Bottom = startScreenInfoRow + viewportHeight - 1;
             }
             else
             {
                 // we can align to the top so we'll just move the viewport
                 // to the bottom of the screen buffer
-                newViewport.Bottom = gsl::narrow<SHORT>(bottomRow);
-                newViewport.Top = gsl::narrow<SHORT>(bottomRow - viewportHeight + 1);
+                newViewport.Bottom = bottomRow;
+                newViewport.Top = bottomRow - viewportHeight + 1;
             }
         }
         else
@@ -845,20 +845,20 @@ IFACEMETHODIMP UiaTextRangeBase::ScrollIntoView(_In_ BOOL alignToTop)
             if (static_cast<unsigned int>(endScreenInfoRow) >= viewportHeight)
             {
                 // we can align to bottom
-                newViewport.Bottom = gsl::narrow<SHORT>(endScreenInfoRow);
-                newViewport.Top = gsl::narrow<SHORT>(endScreenInfoRow - viewportHeight + 1);
+                newViewport.Bottom = endScreenInfoRow;
+                newViewport.Top = endScreenInfoRow - viewportHeight + 1;
             }
             else
             {
                 // we can't align to bottom so we'll move the viewport to
                 // the top of the screen buffer
-                newViewport.Top = gsl::narrow<SHORT>(topRow);
-                newViewport.Bottom = gsl::narrow<SHORT>(topRow + viewportHeight - 1);
+                newViewport.Top = topRow;
+                newViewport.Bottom = topRow + viewportHeight - 1;
             }
         }
 
-        FAIL_FAST_IF(!(newViewport.Top >= gsl::narrow<SHORT>(topRow)));
-        FAIL_FAST_IF(!(newViewport.Bottom <= gsl::narrow<SHORT>(bottomRow)));
+        FAIL_FAST_IF(!(newViewport.Top >= topRow));
+        FAIL_FAST_IF(!(newViewport.Bottom <= bottomRow));
         FAIL_FAST_IF(!(_getViewportHeight(oldViewport) == _getViewportHeight(newViewport)));
 
         _ChangeViewport(newViewport);
@@ -938,30 +938,31 @@ void UiaTextRangeBase::_getBoundingRect(_In_ const COORD startAnchor, _In_ const
     // startAnchor is converted to the viewport coordinate space
     auto startCoord = startAnchor;
     viewport.ConvertToOrigin(&startCoord);
-    topLeft.x = startCoord.X * currentFontSize.X;
-    topLeft.y = startCoord.Y * currentFontSize.Y;
+    topLeft.x = base::ClampMul<long, long>(startCoord.X, currentFontSize.X);
+    topLeft.y = base::ClampMul<long, long>(startCoord.Y, currentFontSize.Y);
 
     if (IsDegenerate())
     {
-        bottomRight.x = (startCoord.X) * currentFontSize.X;
-        bottomRight.y = (startCoord.Y + 1) * currentFontSize.Y;
+        // An empty array is returned for a degenerate (empty) text range or for a text range
+        // reference: https://docs.microsoft.com/en-us/windows/win32/api/uiautomationclient/nf-uiautomationclient-iuiautomationtextrange-getboundingrectangles
+        return;
     }
     else
     {
         // endAnchor is converted to the viewport coordinate space
         auto endCoord = endAnchor;
         viewport.ConvertToOrigin(&endCoord);
-        bottomRight.x = (endCoord.X + 1) * currentFontSize.X;
-        bottomRight.y = (endCoord.Y + 1) * currentFontSize.Y;
+        bottomRight.x = base::ClampMul<long, long>(endCoord.X, currentFontSize.X);
+        bottomRight.y = base::ClampMul<long, long>(base::ClampAdd(endCoord.Y, 1), currentFontSize.Y);
     }
 
     // convert the coords to be relative to the screen instead of
     // the client window
     _TranslatePointToScreen(&topLeft);
     _TranslatePointToScreen(&bottomRight);
 
-    const LONG width = bottomRight.x - topLeft.x;
-    const LONG height = bottomRight.y - topLeft.y;
+    const long width = base::ClampSub(bottomRight.x, topLeft.x);
+    const long height = base::ClampSub(bottomRight.y, topLeft.y);
 
     // insert the coords
     coords.push_back(topLeft.x);
diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp
@@ -179,12 +179,14 @@ bool Viewport::IsInBounds(const Viewport& other) const noexcept
 // - Determines if the given coordinate position lies within this viewport.
 // Arguments:
 // - pos - Coordinate position
-// - allowBottomExclusive - if true, allow the X position to be on the BottomExclusive boundary
+// - allowEndExclusive - if true, allow the EndExclusive COORD as a valid position.
+//                        Used in accessibility to signify that the exclusive end
+//                        includes the last COORD in a given viewport.
 // Return Value:
 // - True if it lies inside the viewport. False otherwise.
-bool Viewport::IsInBounds(const COORD& pos, bool allowBottomExclusive) const noexcept
+bool Viewport::IsInBounds(const COORD& pos, bool allowEndExclusive) const noexcept
 {
-    if (allowBottomExclusive && pos.X == Left() && pos.Y == BottomExclusive())
+    if (allowEndExclusive && pos == EndExclusive())
     {
         return true;
     }
@@ -274,12 +276,14 @@ bool Viewport::MoveInBounds(const ptrdiff_t move, COORD& pos) const noexcept
 // - Increments the given coordinate within the bounds of this viewport.
 // Arguments:
 // - pos - Coordinate position that will be incremented, if it can be.
-// - allowBottomExclusive - if true, allow the X position to be on the BottomExclusive boundary
+// - allowEndExclusive - if true, allow the EndExclusive COORD as a valid position.
+//                        Used in accessibility to signify that the exclusive end
+//                        includes the last COORD in a given viewport.
 // Return Value:
 // - True if it could be incremented. False if it would move outside.
-bool Viewport::IncrementInBounds(COORD& pos, bool allowBottomExclusive) const noexcept
+bool Viewport::IncrementInBounds(COORD& pos, bool allowEndExclusive) const noexcept
 {
-    return WalkInBounds(pos, { XWalk::LeftToRight, YWalk::TopToBottom }, allowBottomExclusive);
+    return WalkInBounds(pos, { XWalk::LeftToRight, YWalk::TopToBottom }, allowEndExclusive);
 }
 
 // Method Description:
@@ -299,12 +303,14 @@ bool Viewport::IncrementInBoundsCircular(COORD& pos) const noexcept
 // - Decrements the given coordinate within the bounds of this viewport.
 // Arguments:
 // - pos - Coordinate position that will be incremented, if it can be.
-// - allowBottomExclusive - if true, allow the X position to be on the BottomExclusive boundary
+// - allowEndExclusive - if true, allow the EndExclusive COORD as a valid position.
+//                        Used in accessibility to signify that the exclusive end
+//                        includes the last COORD in a given viewport.
 // Return Value:
 // - True if it could be incremented. False if it would move outside.
-bool Viewport::DecrementInBounds(COORD& pos, bool allowBottomExclusive) const noexcept
+bool Viewport::DecrementInBounds(COORD& pos, bool allowEndExclusive) const noexcept
 {
-    return WalkInBounds(pos, { XWalk::RightToLeft, YWalk::BottomToTop }, allowBottomExclusive);
+    return WalkInBounds(pos, { XWalk::RightToLeft, YWalk::BottomToTop }, allowEndExclusive);
 }
 
 // Method Description:
@@ -325,19 +331,21 @@ bool Viewport::DecrementInBoundsCircular(COORD& pos) const noexcept
 // Arguments:
 // - first- The first coordinate position
 // - second - The second coordinate position
-// - allowBottomExclusive - if true, allow the COORD's Y value to be BottomExclusive when X = Left
+// - allowEndExclusive - if true, allow the EndExclusive COORD as a valid position.
+//                        Used in accessibility to signify that the exclusive end
+//                        includes the last COORD in a given viewport.
 // Return Value:
 // -  Negative if First is to the left of the Second.
 // -  0 if First and Second are the same coordinate.
 // -  Positive if First is to the right of the Second.
 // -  This is so you can do s_CompareCoords(first, second) <= 0 for "first is left or the same as second".
 //    (the < looks like a left arrow :D)
 // -  The magnitude of the result is the distance between the two coordinates when typing characters into the buffer (left to right, top to bottom)
-int Viewport::CompareInBounds(const COORD& first, const COORD& second, bool allowBottomExclusive) const noexcept
+int Viewport::CompareInBounds(const COORD& first, const COORD& second, bool allowEndExclusive) const noexcept
 {
     // Assert that our coordinates are within the expected boundaries
-    FAIL_FAST_IF(!IsInBounds(first, allowBottomExclusive));
-    FAIL_FAST_IF(!IsInBounds(second, allowBottomExclusive));
+    FAIL_FAST_IF(!IsInBounds(first, allowEndExclusive));
+    FAIL_FAST_IF(!IsInBounds(second, allowEndExclusive));
 
     // First set the distance vertically
     //   If first is on row 4 and second is on row 6, first will be -2 rows behind second * an 80 character row would be -160.
@@ -364,13 +372,15 @@ int Viewport::CompareInBounds(const COORD& first, const COORD& second, bool allo
 // Arguments:
 // - pos - Coordinate position that will be adjusted, if it can be.
 // - dir - Walking direction specifying which direction to go when reaching the end of a row/column
-// - allowBottomExclusive - if true, allow the X position to be on the BottomExclusive boundary
+// - allowEndExclusive - if true, allow the EndExclusive COORD as a valid position.
+//                        Used in accessibility to signify that the exclusive end
+//                        includes the last COORD in a given viewport.
 // Return Value:
 // - True if it could be adjusted as specified and remain in bounds. False if it would move outside.
-bool Viewport::WalkInBounds(COORD& pos, const WalkDir dir, bool allowBottomExclusive) const noexcept
+bool Viewport::WalkInBounds(COORD& pos, const WalkDir dir, bool allowEndExclusive) const noexcept
 {
     auto copy = pos;
-    if (WalkInBoundsCircular(copy, dir, allowBottomExclusive))
+    if (WalkInBoundsCircular(copy, dir, allowEndExclusive))
     {
         pos = copy;
         return true;
@@ -388,19 +398,21 @@ bool Viewport::WalkInBounds(COORD& pos, const WalkDir dir, bool allowBottomExclu
 // Arguments:
 // - pos - Coordinate position that will be adjusted.
 // - dir - Walking direction specifying which direction to go when reaching the end of a row/column
-// - allowBottomExclusive - if true, allow the X position to be on the BottomExclusive boundary
+// - allowEndExclusive - if true, allow the EndExclusive COORD as a valid position.
+//                        Used in accessibility to signify that the exclusive end
+//                        includes the last COORD in a given viewport.
 // Return Value:
 // - True if it could be adjusted inside the viewport.
 // - False if it rolled over from the final corner back to the initial corner
 //   for the specified walk direction.
-bool Viewport::WalkInBoundsCircular(COORD& pos, const WalkDir dir, bool allowBottomExclusive) const noexcept
+bool Viewport::WalkInBoundsCircular(COORD& pos, const WalkDir dir, bool allowEndExclusive) const noexcept
 {
     // Assert that the position given fits inside this viewport.
-    FAIL_FAST_IF(!IsInBounds(pos, allowBottomExclusive));
+    FAIL_FAST_IF(!IsInBounds(pos, allowEndExclusive));
 
     if (dir.x == XWalk::LeftToRight)
     {
-        if (allowBottomExclusive && pos.X == Left() && pos.Y == BottomExclusive())
+        if (allowEndExclusive && pos.X == Left() && pos.Y == BottomExclusive())
         {
             pos.Y = Top();
             return false;
@@ -412,7 +424,7 @@ bool Viewport::WalkInBoundsCircular(COORD& pos, const WalkDir dir, bool allowBot
             if (dir.y == YWalk::TopToBottom)
             {
                 pos.Y++;
-                if (allowBottomExclusive && pos.Y == BottomExclusive())
+                if (allowEndExclusive && pos.Y == BottomExclusive())
                 {
                     return true;
                 }

Original file line number	Diff line number	Diff line change
`@@ -1105,7 +1105,6 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w`
`1105`	`1105`	`}`
`1106`	`1106`	`}`
`1107`	`1107`
`1108`		`-`
`1109`	`1108`	`// Method Description:`
`1110`	`1109`	`// - Helper method for GetWordEnd(). Get the COORD for the beginning of the next READABLE word`
`1111`	`1110`	`// Arguments:`
Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,10 @@`
`5`	`5`	`#include "XamlUiaTextRange.h"`
`6`	`6`	`#include "UiaTextRange.hpp"`
`7`	`7`
	`8`	`+// the same as COR_E_NOTSUPPORTED`
	`9`	`+// we don't want to import the CLR headers to get it`
	`10`	`+#define XAML_E_NOT_SUPPORTED 0x80131515L`
	`11`	`+`
`8`	`12`	`namespace UIA`
`9`	`13`	`{`
`10`	`14`	`using ::ITextRangeProvider;`
`@@ -24,18 +28,6 @@ namespace XamlAutomation`
`24`	`28`
`25`	`29`	`namespace winrt::Microsoft::Terminal::TerminalControl::implementation`
`26`	`30`	`{`
`27`		`- // EmptyObject is our equivalent of UIAutomationCore::UiaGetReservedNotSupportedValue()`
`28`		`- // This retrieves a value that is interpreted as "not supported".`
`29`		`- class EmptyObject : public winrt::implements<EmptyObject, IInspectable>`
`30`		`- {`
`31`		`- public:`
`32`		`- static IInspectable GetInstance()`
`33`		`- {`
`34`		`- static auto empty = make_self<EmptyObject>();`
`35`		`- return *empty;`
`36`		`- }`
`37`		`- };`
`38`		`-`
`39`	`31`	`XamlAutomation::ITextRangeProvider XamlUiaTextRange::Clone() const`
`40`	`32`	`{`
`41`	`33`	`UIA::ITextRangeProvider* pReturn;`
`@@ -100,9 +92,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation`
`100`	`92`	`}`
`101`	`93`	`else`
`102`	`94`	`{`
`103`		`- // We _need_ to return an empty object here.`
	`95`	`+ // We _need_ to return XAML_E_NOT_SUPPORTED here.`
`104`	`96`	`// Returning nullptr is an improper implementation of it being unsupported.`
`105`		`- return EmptyObject::GetInstance();`
	`97`	`+ winrt::throw_hresult(XAML_E_NOT_SUPPORTED);`
`106`	`98`	`}`
`107`	`99`	`}`
`108`	`100`
Original file line number	Diff line number	Diff line change
`@@ -352,11 +352,11 @@ IFACEMETHODIMP ScreenInfoUiaProviderBase::GetVisibleRanges(_Outptr_result_mayben`
`352`	`352`	`}`
`353`	`353`
`354`	`354`	`// stuff each visible line in the safearray`
`355`		`- for (SHORT i = 0; i < rowCount; ++i)`
	`355`	`+ for (short i = 0; i < rowCount; ++i)`
`356`	`356`	`{`
`357`	`357`	`// end is exclusive so add 1`
`358`		`- const COORD start { viewport.Left(), viewport.Top() + i };`
`359`		`- const COORD end { start.X, start.Y + 1 };`
	`358`	`+ const COORD start{ viewport.Left(), viewport.Top() + i };`
	`359`	`+ const COORD end{ start.X, start.Y + 1 };`
`360`	`360`
`361`	`361`	`HRESULT hr = S_OK;`
`362`	`362`	`WRL::ComPtr<UiaTextRangeBase> range;`