Spec says all that stuff is unsigned. We currently use int32_t...

MozReview-Commit-ID: 15tqFjexx4q

Comment on attachment 8845225 [details] [diff] [review] Propagate uint32_t deeper into the editor state and text control frame code Review of attachment 8845225 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsTextEditorState.cpp @@ +1850,5 @@ > void > nsTextEditorState::SetRangeText(const nsAString& aReplacement, uint32_t aStart, > uint32_t aEnd, SelectionMode aSelectMode, > + ErrorResult& aRv, > + Maybe<uint32_t> aSelectionStart, This won't pass the static analysis :). Maybe<T> contains an `alignas(T)` member, and thus isn't safe to pass by parameter, due to ABI not being required to respect alignas(T) fields. Right now it is marked as MOZ_NON_PARAM, and once bug 1339537 lands it will also be rejected due to the alignas member. (See bug 1287006 comment 26 for a more detailed explanation). Try passing these fields by const reference instead :). ::: dom/html/nsTextEditorState.h @@ +338,5 @@ > // otherwise they're the start and end of our selection range. > void SetRangeText(const nsAString& aReplacement, uint32_t aStart, > uint32_t aEnd, mozilla::dom::SelectionMode aSelectMode, > + mozilla::ErrorResult& aRv, > + mozilla::Maybe<uint32_t> aSelectionStart = mozilla::Nothing(), Again, pass by const reference here. ::: testing/web-platform/tests/html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html @@ +143,5 @@ > + input.setSelectionRange(Math.pow(2,32) - 2, Math.pow(2,32) - 1); > + assert_equals(input.selectionStart, input.value.length, > + "element.selectionStart should be value.length"); > + assert_equals(input.selectionEnd, input.value.length, > + "element.selectionStart should be value.length"); typo: selectionEnd @@ +151,5 @@ > + input.setSelectionRange(Math.pow(2,31), Math.pow(2,32) - 1); > + assert_equals(input.selectionStart, input.value.length, > + "element.selectionStart should be value.length"); > + assert_equals(input.selectionEnd, input.value.length, > + "element.selectionStart should be value.length"); typo: selectionEnd @@ +278,5 @@ > + textarea.setSelectionRange(Math.pow(2,32) - 2, Math.pow(2,32) - 1); > + assert_equals(textarea.selectionStart, textarea.value.length, > + "element.selectionStart should be value.length"); > + assert_equals(textarea.selectionEnd, textarea.value.length, > + "element.selectionStart should be value.length"); Typo: selectionEnd @@ +286,5 @@ > + textarea.setSelectionRange(Math.pow(2,31), Math.pow(2,32) - 1); > + assert_equals(textarea.selectionStart, textarea.value.length, > + "element.selectionStart should be value.length"); > + assert_equals(textarea.selectionEnd, textarea.value.length, > + "element.selectionStart should be value.length"); typo: selectionEnd

> This won't pass the static analysis :). Gah. I wish we'd just run that on try automatically instead of it being a separate platform. :( Thanks for catching this! > typo: selectionEnd mmm, copy/paste. Fixed.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea659450b0da Propagate uint32_t deeper into the editor state and text control frame code. r=mystor

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=82842964&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/ac6e090b6365

The issue is the patches for bug 1343037.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c3eb6aa697 Propagate uint32_t deeper into the editor state and text control frame code. r=mystor