[webkit-reviews] review granted: [Bug 50565] Web Inspector: Enable CSS property editing name/value-wise (like Firebug does) : [Attachment 76052] [PATCH] Fixed handling of ; /: on Mac

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 50565: Web Inspector: Enable CSS property editing name/value-wise (like
Firebug does)
https://bugs.webkit.org/show_bug.cgi?id=50565

Attachment 76052: [PATCH] Fixed handling of ;/: on Mac
https://bugs.webkit.org/attachment.cgi?id=76052&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76052&action=review

r=me, with some comments below. Pavel might want to comment, since they were
originally his comments. Looks and feels slightly better than the first patch.
Thanks!

> WebCore/ChangeLog:13
> +	   the next editable field (while applying the entire property value.)
Once a rule boundary
> +	   is reached, the editing starts for the next rule selector/previous
style blank property.

Nit: Starting with "Once a rule..." could be a separate paragraph since it is
separate behavior.
And you can simplify this by saying that you now allow for tabbing through all
"editable" styles,
even across selector boundaries.

> WebCore/inspector/front-end/inspector.js:1922
> +// Available config fields:
> +// commitHandler: Function - handles editing "commit" outcome
> +// cancelHandler: Function - handles editing "cancel" outcome
> +// customFinishHandler: Function|undefined - custom finish handler for the
editing session
> +// multiline: true|false - whether the edited element is multiline
> +WebInspector.startEditing = function(element, config, context)

Why couldn't context just be included in config? Or is config for entirely
optional arguments?
This was Pavel's review comment, so I'd like to see what he thinks of your
approach. If context
is required, I would have preferred the following signature:

  WebInspector.startEditing = function(element, context, options)

To have code that uses it look cleanly like:

  WebInspector.startEditing(elem, tagName, {
      multiline: true,
      commitHandler: handler
      ...
  });