[webkit-reviews] review requested: [Bug 178404] Web Inspector: [PARITY] Styles Redesign: Add color gradient, bezier curve, and spring inline widgets : [Attachment 324430] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 20 12:43:09 PDT 2017


Nikita Vasilyev <nvasilyev at apple.com> has asked  for review:
Bug 178404: Web Inspector: [PARITY] Styles Redesign: Add color gradient, bezier
curve, and spring inline widgets
https://bugs.webkit.org/show_bug.cgi?id=178404

Attachment 324430: Patch

https://bugs.webkit.org/attachment.cgi?id=324430&action=review




--- Comment #8 from Nikita Vasilyev <nvasilyev at apple.com> ---
Created attachment 324430

  --> https://bugs.webkit.org/attachment.cgi?id=324430&action=review

Patch

(In reply to Matt Baker from comment #4)
> Comment on attachment 324071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324071&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:295
> > +		 let value = event.data && event.data.value &&
event.data.value.toString();
> 
> InlineSwatch always includes event data when the value changes, so that
> check can be removed. I think it's also safe to assume that event.data
> implies event.data.toString(). With the exception of
> Color.prototype.toString (which can throw), all the InlineSwatch value
> objects (Color, CubicBezier, Spring, and the Gradient subclasses) will
> always return a string.
> 
> It's not as concise, but you could rewrite this as:
> 
> if (!event.data.value)
>     return;
> 
> try {
>     let value = event.data.value.toString();
>     console.assert(value, "Expected value string.", event.data.value);
>     if (!value)
>	  return;
>     this._handleValueChange();
> } catch (e) {
>     console.error("Could not convert value to string.", e);
> }

The old styled sidebar didn't have try/catch, see
CSSStyleDeclarationTextEditor.propotype._inlineSwatchValueChanged:

    let value = event.data && event.data.value && event.data.value.toString();

We rarely use try/catch and it seems unnecessary here. I think it's better to
replace it with console.error.


More information about the webkit-reviews mailing list