Bug 208175 - IsolatedObject support for ProgressIndicator and Meter.
Summary: IsolatedObject support for ProgressIndicator and Meter.
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: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-24 20:02 PST by Andres Gonzalez
Modified: 2020-02-25 08:29 PST (History)
11 users (show)

See Also:


Attachments
Patch (10.57 KB, patch)
2020-02-24 20:15 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (10.44 KB, patch)
2020-02-25 06:02 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2020-02-24 20:02:07 PST
IsolatedObject support for ProgressIndicator and Meter.
Comment 1 Andres Gonzalez 2020-02-24 20:15:02 PST
Created attachment 391621 [details]
Patch
Comment 2 Darin Adler 2020-02-24 22:05:37 PST
Comment on attachment 391621 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:97
> +    if (!gaugeRegionValue.isEmpty()) {
> +        StringBuilder builder;
> +        builder.append(description);
> +        if (builder.length())
> +            builder.appendLiteral(", ");
> +        builder.append(gaugeRegionValue);
> +        description = builder.toString();
> +    }

Here’s a more efficient way to write this:

    if (!gaugeRegionValue.isEmpty())
        description = makeString(description, description.isEmpty() ? "" : ", ", gaugeRegionValue);

Or we could use the + operator if you prefer that to makeString.

    if (!gaugeRegionValue.isEmpty())
        description = description + (description.isEmpty() ? "" : ", ") + gaugeRegionValue;
Comment 3 Andres Gonzalez 2020-02-25 06:02:47 PST
Created attachment 391644 [details]
Patch
Comment 4 Andres Gonzalez 2020-02-25 06:12:41 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 391621 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391621&action=review
> 
> > Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:97
> > +    if (!gaugeRegionValue.isEmpty()) {
> > +        StringBuilder builder;
> > +        builder.append(description);
> > +        if (builder.length())
> > +            builder.appendLiteral(", ");
> > +        builder.append(gaugeRegionValue);
> > +        description = builder.toString();
> > +    }
> 
> Here’s a more efficient way to write this:
> 
>     if (!gaugeRegionValue.isEmpty())
>         description = makeString(description, description.isEmpty() ? "" :
> ", ", gaugeRegionValue);
> 
> Or we could use the + operator if you prefer that to makeString.
> 
>     if (!gaugeRegionValue.isEmpty())
>         description = description + (description.isEmpty() ? "" : ", ") +
> gaugeRegionValue;

Thanks very much, Darin. Changed it to:

    if (!gaugeRegionValue.isEmpty())
        description = description.isEmpty() ? gaugeRegionValue : description + ", " + gaugeRegionValue;

So that we don't call operator+ unnecessarily when description.isEmpty. I assumed that the StringBuilder implementation was more efficient than the String concatenation and so I kept it. Good to know that that is not the case. Thanks again.
Comment 5 WebKit Commit Bot 2020-02-25 08:28:56 PST
Comment on attachment 391644 [details]
Patch

Clearing flags on attachment: 391644

Committed r257356: <https://trac.webkit.org/changeset/257356>
Comment 6 WebKit Commit Bot 2020-02-25 08:28:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2020-02-25 08:29:13 PST
<rdar://problem/59765193>