Bug 200554

Summary: Web Inspector: Elements: Computed: show shorthand properties in addition to longhand ones
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, mattbaker, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 203668, 205035    
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Patch
none
[Image] After Patch is applied
none
Patch none

Myles C. Maxfield
Reported 2019-08-08 17:40:44 PDT
It would be useful, since I haven't memorized exactly which properties are shorthands and which are longhands. I was recently looking for "border-radius" in the sidebar but didn't find anything.
Attachments
Patch (40.26 KB, patch)
2019-08-23 19:31 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
[Image] After Patch is applied (191.05 KB, image/png)
2019-08-23 19:31 PDT, Devin Rousso 		no flags
Details
Patch (35.78 KB, patch)
2019-10-11 14:31 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
[Image] After Patch is applied (442.04 KB, image/png)
2019-10-11 14:32 PDT, Devin Rousso 		no flags
Details
Patch (30.06 KB, patch)
2019-10-11 19:37 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2019-08-23 19:31:22 PDT
Created attachment 377195 [details] Patch
Devin Rousso
Comment 2 2019-08-23 19:31:43 PDT
Created attachment 377196 [details] [Image] After Patch is applied
Devin Rousso
Comment 3 2019-10-11 14:31:45 PDT
Created attachment 380784 [details] Patch
Devin Rousso
Comment 4 2019-10-11 14:32:00 PDT
Created attachment 380785 [details] [Image] After Patch is applied
Matt Baker
Comment 5 2019-10-11 15:20:21 PDT
(In reply to Devin Rousso from comment #4) > Created attachment 380785 [details] > [Image] After Patch is applied Nice, I like having this as a toggle.
Matt Baker
Comment 6 2019-10-11 17:54:24 PDT
Comment on attachment 380784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380784&action=review r=me, nice work! > Source/WebCore/inspector/InspectorStyleSheet.cpp:544 > + for (auto property : collectProperties(true)) { auto& will prevent copy constructing InspectorStyleProperty objects. > Source/WebCore/inspector/InspectorStyleSheet.cpp:581 > + IMO, separating the return value from the local with a blank line doesn't add anything. > Source/WebCore/inspector/InspectorStyleSheet.cpp:616 > + continue; if (m_style->getPropertyValue(name).isEmpty()) continue; > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:330 > + get isVariable() { return this._isVariable; } Nice. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:391 > + if (this._isShorthand === undefined) { Our usual style is to reduce arrowing with an early return: if (this._isShorthand !== undefined) return this._isShorthand; > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:90 > + this._propertiesComputedStyleSection = new WI.ComputedStyleSection(this); I'd prefer to leave this as `_computedStyleSection`. I feel like it mostly adds churn. > Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:41 > + this._showsShorthandsInsteadOfLonghands = false; Since this is a concept, it should be singular. Reads better too: this._showsShorthandInsteadOfLonghand
Devin Rousso
Comment 7 2019-10-11 19:22:06 PDT
Comment on attachment 380784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380784&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:391 >> + if (this._isShorthand === undefined) { > > Our usual style is to reduce arrowing with an early return: > > if (this._isShorthand !== undefined) > return this._isShorthand; I think this pattern is far more common for lazy-initialization. I also prefer this approach as it keeps the function "cleaner" in that there's only one `return`. >> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:41 >> + this._showsShorthandsInsteadOfLonghands = false; > > Since this is a concept, it should be singular. Reads better too: > > this._showsShorthandInsteadOfLonghand If there was only one property per `WI.ComputedStyleSection`, I'd agree (show the shorthand instead of the longhand), but if there are multiple it makes more sense as a plural (show all shorthands instead of all longhands).
Devin Rousso
Comment 8 2019-10-11 19:37:08 PDT
Created attachment 380809 [details] Patch Slimmed down this patch to avoid a bunch of unnecessary churn
WebKit Commit Bot
Comment 9 2019-10-11 20:40:49 PDT
Comment on attachment 380809 [details] Patch Clearing flags on attachment: 380809 Committed r251038: <https://trac.webkit.org/changeset/251038>
WebKit Commit Bot
Comment 10 2019-10-11 20:40:51 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2019-10-15 14:40:54 PDT
<rdar://problem/56308811>
Note You need to log in before you can comment on or make changes to this bug.