WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
195580
Web Inspector: Styles: remove unused protocol values
https://bugs.webkit.org/show_bug.cgi?id=195580
Summary
Web Inspector: Styles: remove unused protocol values
Devin Rousso
Reported
2019-03-11 16:51:57 PDT
As an example, `CSS.CSSStyle` has a `width` and `height`, which don't appear to be used.
Attachments
Patch
(46.24 KB, patch)
2019-03-12 00:09 PDT
,
Devin Rousso
joepeck
: review-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-03-11 16:52:15 PDT
<
rdar://problem/48789619
>
Devin Rousso
Comment 2
2019-03-12 00:09:16 PDT
Created
attachment 364363
[details]
Patch
Joseph Pecoraro
Comment 3
2019-03-14 22:14:46 PDT
Comment on
attachment 364363
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=364363&action=review
> Source/JavaScriptCore/ChangeLog:20 > + * inspector/protocol/CSS.json: > + - remove `range` property from `SelectorList` > + - remove `CSSStyleAttribute` type > + - rename `CSSStyleSheetHeader` to `CSSStyleSheet` > + - remove `title` and `disabled` properties from `CSSStyleSheet` > + - remove `CSSStyleSheetBody` type > + - remove `sourceURL` and `sourceLine` properties from `CSSRule` > + - remove `ShorthandEntry` type > + - remove `shorthandEntries`, `width`, and `height` properties from `CSSStyle` > + - remove `source`, `sourceURL`, and `sourceLine` properties from `CSSMedia` > + - remove `getStyleSheet` command > + Unify the source location range/url data to be held on the `CSSRule` (range) and `CSSStyleSheet` (url).
It might make sense to split this up into separate patches that are much easier to review. As is there are all kinds of extraneous changes that have to be reviewed together which makes it hard, even though removing some things on their own are easy to review. It would be good to understand why some of these are no longer needed, or what supplants them. Without any explanation it is difficult to review. Each of these had a purpose before, so what are we missing out on by removing them. It could be we can add them back and get a feature, which would be better than removing and adding back.
> LayoutTests/inspector/css/add-rule-expected.txt:8 > PASS: Rule selector should be "body" > -PASS: Rule range should be [0:0,0:4] > +PASS: Rule range should be [0:6,0:6] > PASS: Rule origin should be "inspector"
These do not look right. Previously it looks like it would be 'body' and now it looks like it would be ''
Devin Rousso
Comment 4
2019-03-14 22:34:19 PDT
Comment on
attachment 364363
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=364363&action=review
>> Source/JavaScriptCore/ChangeLog:20 >> + Unify the source location range/url data to be held on the `CSSRule` (range) and `CSSStyleSheet` (url). > > It might make sense to split this up into separate patches that are much easier to review. As is there are all kinds of extraneous changes that have to be reviewed together which makes it hard, even though removing some things on their own are easy to review. > > It would be good to understand why some of these are no longer needed, or what supplants them. Without any explanation it is difficult to review. Each of these had a purpose before, so what are we missing out on by removing them. It could be we can add them back and get a feature, which would be better than removing and adding back.
Most of these either weren't ever used (e.g. `shorthandEntries`, `width`, and `height` properties from `CSSStyle`) or could be replaced/merged (e.g. unnecessary duplication of data) into a single payload (e.g. all of the `sourceURL` and `sourceLine` changes). I don't think splitting it up would be that much better, as much of it is the unification of all of the `sourceURL`/`sourceLine` values into a single value.
>> LayoutTests/inspector/css/add-rule-expected.txt:8 >> PASS: Rule origin should be "inspector" > > These do not look right. Previously it looks like it would be 'body' and now it looks like it would be ''
They are correct, but they deserve a better explanation than I provided :( Previously, we said that the range of the rule was the range of the selector. I don't think that makes a lot of sense, especially when it comes to how Web Inspector uses that range to "go to source location". If there is whitespace between the selector and the rule's body (e.g. the style), "go to source location" would highlight the last line of the selector, rather than the first line of the style. The change in this case is to make it so that the rule's range is now the full range of the style. In this case, it looks like the range is empty because the rule's content in the actual <style> is empty. If we modified `inspeector/css/add-rule.html` to include some text (even plain whitespace), you'd see that the range printed here would reflect that.
Joseph Pecoraro
Comment 5
2019-08-05 13:38:09 PDT
Comment on
attachment 364363
[details]
Patch This really needs to be broken up into smaller patches. r- for now.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug