Summary: | Web Inspector: Styles: refactor properties/allProperties/visibleProperties/allVisibleProperties | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2019-01-19 17:43:55 PST
Created attachment 359626 [details]
Patch
Comment on attachment 359626 [details] Patch Attachment 359626 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10815019 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html Created attachment 359630 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 359626 [details]
Patch
Unrelated test failures.
Comment on attachment 359626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359626&action=review r=me, nice refactor Personally, given the fact that any given `WI.CSSStyleDeclaration` is unlikely to have a significantly large number of `WI.CSSProperty`, I'd also be fine with inlining the logic to create `this._enabledProperties` and `this._visibleProperties` inside their respective getters (e.g. don't save the array operation result to a member variable and recalculate it each time, expecting the callers to cache the result). > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:47 > + this._properties = []; NIT: `this._enabledProperties = [];` should also be defined here > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:118 > + this._enabledProperties = properties.filter((property) => property.enabled); > + this._properties = properties; NIT: `this._properties` should come before `this._enabledProperties`. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:224 > + return this._enabledProperties; NIT: I think this should be a lazy creation (like `get visibleProperties`). I realize that `this._enabledProperties` is most likely used shortly after `this._properties` is set`, but that isn't always the case (and may not be in the future), and we should try to be more efficient/lazy where we can. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:308 > + hasEnabledProperties() NIT: I realize that this is a refactoring change, but I personally don't find much use in functions like this. I think it'd be easier/simpler to have the caller do `style.enabledProperties.length`. It's really a not-so-convenient convenience. Comment on attachment 359626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359626&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:224 >> + return this._enabledProperties; > > NIT: I think this should be a lazy creation (like `get visibleProperties`). I realize that `this._enabledProperties` is most likely used shortly after `this._properties` is set`, but that isn't always the case (and may not be in the future), and we should try to be more efficient/lazy where we can. I didn't do lazy creation because enabledProperties are used in `update` method. Created attachment 359817 [details]
Patch
Comment on attachment 359817 [details] Patch Clearing flags on attachment: 359817 Committed r240314: <https://trac.webkit.org/changeset/240314> All reviewed patches have been landed. Closing bug. |