Bug 209966 - REGRESSION(r259401): [GTK] Check surroundingRange is not null
Summary: REGRESSION(r259401): [GTK] Check surroundingRange is not null
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Diego Pino
URL:
Keywords: InRadar
Depends on:
Blocks: 209723
  Show dependency treegraph
 
Reported: 2020-04-03 08:19 PDT by Diego Pino
Modified: 2020-04-07 13:50 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.14 KB, patch)
2020-04-03 08:20 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (2.12 KB, patch)
2020-04-03 09:00 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (1.69 KB, patch)
2020-04-03 09:17 PDT, Diego Pino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Pino 2020-04-03 08:19:57 PDT
REGRESSION(r259401): [GTK] Check surroundingRange is not null
Comment 1 Diego Pino 2020-04-03 08:20:29 PDT
Created attachment 395377 [details]
Patch
Comment 2 Philippe Normand 2020-04-03 08:34:21 PDT
Comment on attachment 395377 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/glib/WebPageGLib.cpp:116
> +            if (surroundingRange != nullptr)

We usually don't do explicit checks like this. Can you remove the `!= nullptr` part?

> Source/WebKit/WebProcess/WebPage/glib/WebPageGLib.cpp:121
> +            if (surroundingRange != nullptr)

Ditto
Comment 3 Diego Pino 2020-04-03 09:00:51 PDT
Created attachment 395382 [details]
Patch
Comment 4 Darin Adler 2020-04-03 09:04:39 PDT
Comment on attachment 395382 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/glib/WebPageGLib.cpp:117
>              surroundingRange->setEnd(compositionRange->startPosition());
>              clonedRange->setStart(compositionRange->endPosition());
> -            postLayoutData.surroundingContext = plainText(*surroundingRange) + plainText(clonedRange);
> +            if (surroundingRange)
> +                postLayoutData.surroundingContext = plainText(*surroundingRange) + plainText(clonedRange);

This change isn’t needed. As you can see, surroundingRange is already used above and so it can’t be null by the time it gets here.

> Source/WebKit/WebProcess/WebPage/glib/WebPageGLib.cpp:122
> -            postLayoutData.surroundingContext = plainText(*surroundingRange);
> +            if (surroundingRange)
> +                postLayoutData.surroundingContext = plainText(*surroundingRange);

This change looks OK. Sorry for missing it. To preserve historical behavior, could instead write:

    postLayoutData.surroundingContext = surroundingRange ? plainText(*surroundingRange) : emptyString();

The old code set the string to empty string, rather than leaving it with its previous value.
Comment 5 Diego Pino 2020-04-03 09:17:05 PDT
Created attachment 395383 [details]
Patch
Comment 6 EWS 2020-04-03 09:54:17 PDT
Committed r259468: <https://trac.webkit.org/changeset/259468>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395383 [details].
Comment 7 Radar WebKit Bug Importer 2020-04-03 09:55:14 PDT
<rdar://problem/61263510>