Bug 237951 - [selection] Change form's selection attribute to unsigned long
Summary: [selection] Change form's selection attribute to unsigned long
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zsun
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-16 02:47 PDT by zsun
Modified: 2022-03-23 09:18 PDT (History)
10 users (show)

See Also:


Attachments
Patch (28.92 KB, patch)
2022-03-16 07:35 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (28.82 KB, patch)
2022-03-17 04:01 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (28.82 KB, patch)
2022-03-17 04:20 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (28.75 KB, patch)
2022-03-17 04:30 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (28.48 KB, patch)
2022-03-17 04:55 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (28.04 KB, patch)
2022-03-17 06:00 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (29.12 KB, patch)
2022-03-17 07:12 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (22.91 KB, patch)
2022-03-23 08:09 PDT, zsun
darin: review-
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zsun 2022-03-16 02:47:21 PDT
We should use unsigned long for the type of selectionStart and selectionEnd.
Comment 1 Radar WebKit Bug Importer 2022-03-16 05:40:18 PDT
<rdar://problem/90364891>
Comment 2 zsun 2022-03-16 07:35:44 PDT
Created attachment 454832 [details]
Patch
Comment 3 zsun 2022-03-17 04:01:43 PDT
Created attachment 454954 [details]
Patch
Comment 4 zsun 2022-03-17 04:20:29 PDT
Created attachment 454955 [details]
Patch
Comment 5 zsun 2022-03-17 04:30:39 PDT
Created attachment 454957 [details]
Patch
Comment 6 zsun 2022-03-17 04:55:45 PDT
Created attachment 454960 [details]
Patch
Comment 7 zsun 2022-03-17 06:00:16 PDT
Created attachment 454963 [details]
Patch
Comment 8 zsun 2022-03-17 07:12:00 PDT
Created attachment 454968 [details]
Patch
Comment 9 zsun 2022-03-21 11:56:49 PDT
Any chance for a review? Thank you.
Comment 10 Chris Dumez 2022-03-21 12:15:37 PDT
Comment on attachment 454968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454968&action=review

r=me

> Source/WebCore/html/HTMLTextFormControlElement.h:183
> +    bool m_hasCachedSelection { false };

I think personally I would have gone with a `std::optional<std::pair<unsigned, unsigned>>` but this is fine too.
Comment 11 EWS 2022-03-22 02:44:37 PDT
Committed r291609 (248702@main): <https://commits.webkit.org/248702@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454968 [details].
Comment 12 zsun 2022-03-23 08:09:45 PDT
Reopening to attach new patch.
Comment 13 zsun 2022-03-23 08:09:49 PDT
Created attachment 455500 [details]
Patch
Comment 14 Darin Adler 2022-03-23 08:44:17 PDT
Comment on attachment 455500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455500&action=review

Looks pretty good. One question.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:327
>          if (!isTextField())
> -            return;
> +            return false;

Is it OK here that we already did the cacheSelection operation? The old code would not have done that. What tests cover this change in behavior?
Comment 15 Darin Adler 2022-03-23 08:45:11 PDT
Comment on attachment 455500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455500&action=review

> Source/WebCore/ChangeLog:986
> +        [selection] Fire Select event when selection extent or direction changes
> +        https://bugs.webkit.org/show_bug.cgi?id=238142

Looks like this new patch to review got attached to the wrong bug.
Comment 16 zsun 2022-03-23 09:06:23 PDT
(In reply to Darin Adler from comment #15)
> Comment on attachment 455500 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455500&action=review
> 
> > Source/WebCore/ChangeLog:986
> > +        [selection] Fire Select event when selection extent or direction changes
> > +        https://bugs.webkit.org/show_bug.cgi?id=238142
> 
> Looks like this new patch to review got attached to the wrong bug.

Ah...I was careless. Just changed the status of the patch to obsolete.
Comment 17 zsun 2022-03-23 09:07:39 PDT
(In reply to Darin Adler from comment #14)
> Comment on attachment 455500 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455500&action=review
> 
> Looks pretty good. One question.
> 
> > Source/WebCore/html/HTMLTextFormControlElement.cpp:327
> >          if (!isTextField())
> > -            return;
> > +            return false;
> 
> Is it OK here that we already did the cacheSelection operation? The old code
> would not have done that. What tests cover this change in behavior?

Let me check on this.