Bug 198806 - Web Inspector: REGRESSION(r246178): extra spaces added in at-rules when formatting CSS
Summary: Web Inspector: REGRESSION(r246178): extra spaces added in at-rules when forma...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
: 198807 (view as bug list)
Depends on: 197829
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-12 14:31 PDT by Devin Rousso
Modified: 2019-06-13 23:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.30 KB, patch)
2019-06-13 00:06 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (22.88 KB, patch)
2019-06-13 18:38 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.97 MB, application/zip)
2019-06-13 22:21 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-06-12 14:31:12 PDT
.pagination .page-numbers:not(.current, .dots):hover {
  --- becomes ---
.pagination .page-numbers:not(.current, .dots) :hover {
Comment 1 Devin Rousso 2019-06-13 00:00:09 PDT
*** Bug 198807 has been marked as a duplicate of this bug. ***
Comment 2 Devin Rousso 2019-06-13 00:06:47 PDT
Created attachment 372026 [details]
Patch

This could use a better explanation, but I'm too tired right now, so I'll do it later :(
Comment 3 Joseph Pecoraro 2019-06-13 12:29:47 PDT
Comment on attachment 372026 [details]
Patch

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

rs=me

> Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js:85
> +                let re = new RegExp(`^\\s*@[a-zA-Z][-a-zA-Z]+${suffix}`);

Seems like we may be building these regexes over and over again, which could be bad for performance.

> Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js:86
> +                return re.test(this._builder.currentLine);

What if this isn't on the same line?

    @media
    (...)
    { ... }

> Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js:89
> +            let inSelector = (suffix = "") => {

`suffix` is unused.
Comment 4 Devin Rousso 2019-06-13 17:14:12 PDT
Comment on attachment 372026 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js:86
>> +                return re.test(this._builder.currentLine);
> 
> What if this isn't on the same line?
> 
>     @media
>     (...)
>     { ... }

`currentLine` refers to the last line of the `FormatterContentBuilder`, so basically everything up to the character `c` after the last `appendNewline`.
Comment 5 Devin Rousso 2019-06-13 18:38:04 PDT
Created attachment 372092 [details]
Patch
Comment 6 Build Bot 2019-06-13 22:21:40 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-06-13 22:21:42 PDT Comment hidden (obsolete)
Comment 8 WebKit Commit Bot 2019-06-13 23:52:13 PDT
Comment on attachment 372092 [details]
Patch

Clearing flags on attachment: 372092

Committed r246430: <https://trac.webkit.org/changeset/246430>
Comment 9 WebKit Commit Bot 2019-06-13 23:52:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-06-13 23:53:21 PDT
<rdar://problem/51737451>