Closed
Bug 1345237
Opened 7 years ago
Closed 7 years ago
Change all the selectionrange/start/end stuff in editor state and text control frame to use uint32_t
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file)
47.09 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
Spec says all that stuff is unsigned. We currently use int32_t...
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
MozReview-Commit-ID: 15tqFjexx4q
Attachment #8845225 -
Flags: review?(michael)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
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
Attachment #8845225 -
Flags: review?(michael) → review+
Assignee | ||
Comment 3•7 years ago
|
||
> 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
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bzbarsky)
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f8c3eb6aa697
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•