Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259433005:
DevTools: Add CSSShadowModel and CSSLength (Closed)
|
Created:
4 years, 4 months ago by flandy Modified:
4 years, 4 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add CSSShadowModel and CSSLength
BUG=639095
Committed: https://crrev.com/b713ca179b7b17d741ff7c11717752c9eefb0c32
Cr-Commit-Position: refs/heads/master@{#413655}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Use array for format #
Total comments: 6
Patch Set 4 : Address comments #
Messages
Total messages: 19 (6 generated)
flandy@google.com changed reviewers: + dgozman@chromium.org, lushnikov@chromium.org
Please, take a look.
https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js (right): https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:87: this._spreadRadius = spreadRadius; Don't you want to add "S" to format here? https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:103: this._format = format; Do we intend to switch between formats? https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:169: var shadow = new WebInspector.CSSShadowModel(false, WebInspector.CSSLength.zero(), WebInspector.CSSLength.zero(), If you never create with meaningful parameters, just assign defaults in constructor and remove parameters. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:203: shadow.setOffset(shadow.offsetX(), length); Use fields, not setters. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:225: shadow.setFormat(format); We can set fields here directly, and not expose setters which are not used outside of parser. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:273: return new RegExp("(" + number + ")(" + unit + ")|" + zero, "gi"); All this regexp magic is hard to understand. Can we make it more straightforward? https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:306: if (unit && typeof unit === "string") How could it be not string? https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:309: return new RegExp(units.join("|"), "i"); Let's cache this one. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:318: var lengthRegex = new RegExp("^(?:" + WebInspector.CSSLength.regex().source + ")$", "i"); Let's cache this one as well. Why don't we inline previous function here? https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/common/module.json (right): https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/module.json:15: "CSSShadowModel.js", Does this make sense in common? I'd put it to sdk, as it won't be commonly used.
https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/common/module.json (right): https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/module.json:15: "CSSShadowModel.js", On 2016/08/22 16:56:41, dgozman wrote: > Does this make sense in common? I'd put it to sdk, as it won't be commonly used. This is per our offline discussion; WI.CSSShadowModel is no different to WI.Color and WI.Geometry. The plan is to put shadow editor in ui/ as it doesn't depend on any runtime inspection.
Please take another look. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js (right): https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:87: this._spreadRadius = spreadRadius; On 2016/08/22 16:56:41, dgozman wrote: > Don't you want to add "S" to format here? Yes, if it is not included in the format already, we will want to add it to the format in the correct location. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:103: this._format = format; On 2016/08/22 16:56:41, dgozman wrote: > Do we intend to switch between formats? No. Removed https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:169: var shadow = new WebInspector.CSSShadowModel(false, WebInspector.CSSLength.zero(), WebInspector.CSSLength.zero(), On 2016/08/22 16:56:41, dgozman wrote: > If you never create with meaningful parameters, just assign defaults in > constructor and remove parameters. Done. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:203: shadow.setOffset(shadow.offsetX(), length); On 2016/08/22 16:56:41, dgozman wrote: > Use fields, not setters. Done. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:225: shadow.setFormat(format); On 2016/08/22 16:56:41, dgozman wrote: > We can set fields here directly, and not expose setters which are not used > outside of parser. Setters will still be used by the Shadow Editor to update the model. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:273: return new RegExp("(" + number + ")(" + unit + ")|" + zero, "gi"); On 2016/08/22 16:56:41, dgozman wrote: > All this regexp magic is hard to understand. Can we make it more > straightforward? I've now inlined the regexes. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:306: if (unit && typeof unit === "string") On 2016/08/22 16:56:41, dgozman wrote: > How could it be not string? Removed. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:309: return new RegExp(units.join("|"), "i"); On 2016/08/22 16:56:41, dgozman wrote: > Let's cache this one. Removed. https://codereview.chromium.org/2259433005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:318: var lengthRegex = new RegExp("^(?:" + WebInspector.CSSLength.regex().source + ")$", "i"); On 2016/08/22 16:56:41, dgozman wrote: > Let's cache this one as well. Why don't we inline previous function here? I use WI.CSSLength.Regex in the parse function as well but without the start and end of string anchors. Here I just add anchors. Do you mean to extract it from this function?
This is much better! https://codereview.chromium.org/2259433005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js (right): https://codereview.chromium.org/2259433005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:78: this._format = this._format.substring(0, yIndex + 1) + WebInspector.CSSShadowModel.FormatParts.BlurRadius + this._format.substring(yIndex + 1); If you make this._format an array, this will be splice(). https://codereview.chromium.org/2259433005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:210: return []; Maybe return correct ones to show swatches for them?
https://codereview.chromium.org/2259433005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js (right): https://codereview.chromium.org/2259433005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:78: this._format = this._format.substring(0, yIndex + 1) + WebInspector.CSSShadowModel.FormatParts.BlurRadius + this._format.substring(yIndex + 1); On 2016/08/22 19:57:21, dgozman wrote: > If you make this._format an array, this will be splice(). Done. https://codereview.chromium.org/2259433005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:210: return []; On 2016/08/22 19:57:21, dgozman wrote: > Maybe return correct ones to show swatches for them? I'd rather keep it as all or nothing right now. We can try to make this optimization later, but we also need to deal with property.parsedOk in StylesSidebarPane.
lgtm https://codereview.chromium.org/2259433005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js (right): https://codereview.chromium.org/2259433005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:16: this._format = ["X", "Y"]; Use named constants here. https://codereview.chromium.org/2259433005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:22: WebInspector.CSSShadowModel.FormatParts = { FormatParts -> _Part https://codereview.chromium.org/2259433005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:303: var match = text.match(lengthRegex) || []; I'd rather do if (!match) return null; if > 2 return length(..) return zero
https://codereview.chromium.org/2259433005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js (right): https://codereview.chromium.org/2259433005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:16: this._format = ["X", "Y"]; On 2016/08/22 23:59:04, dgozman wrote: > Use named constants here. Done. https://codereview.chromium.org/2259433005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:22: WebInspector.CSSShadowModel.FormatParts = { On 2016/08/22 23:59:04, dgozman wrote: > FormatParts -> _Part Done. https://codereview.chromium.org/2259433005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/CSSShadowModel.js:303: var match = text.match(lengthRegex) || []; On 2016/08/22 23:59:04, dgozman wrote: > I'd rather do > if (!match) return null; > if > 2 return length(..) > return zero Done.
The CQ bit was checked by flandy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2259433005/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by flandy@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add CSSShadowModel and CSSLength BUG=639095 ========== to ========== DevTools: Add CSSShadowModel and CSSLength BUG=639095 Committed: https://crrev.com/b713ca179b7b17d741ff7c11717752c9eefb0c32 Cr-Commit-Position: refs/heads/master@{#413655} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b713ca179b7b17d741ff7c11717752c9eefb0c32 Cr-Commit-Position: refs/heads/master@{#413655} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
