Bug 38578

*** Bug 16528 has been marked as a duplicate of this bug. ***

*** Bug 31068 has been marked as a duplicate of this bug. ***

Created attachment 62573 [details] [PATCH] A preliminary implementation

Comment on attachment 62573 [details] [PATCH] A preliminary implementation Some minor comments. I didn't look too intently at the parsing this pass. > +++ b/LayoutTests/inspector/styles-edit-rewrite-expected.txt > +.foo { > + color: green !important; > + border: solid rgb(0, 255, 0); > +border-left-width: 3px !important; > +border-color: rgb(0, 255, 0); > +} I would expect leading whitespace for these last two. > diff --git a/LayoutTests/inspector/styles-edit-rewrite.html b/LayoutTests/inspector/styles-edit-rewrite.html > + if (resource.url.indexOf("styles-edit-rewrite.css") != -1) { Use !== or === wherever possible. In this case, !== is appropriate. > +++ b/WebCore/inspector/front-end/CSSStyleModel.js > + _updateStyles: function(styles) > + { > + if (styles.inlineStyle && !this._idToOriginalStyle[styles.inlineStyle.id]) > + this._idToOriginalStyle[styles.inlineStyle.id] = WebInspector.CSSStyleDeclaration.parseStyle(styles.inlineStyle); > + > + var cssRules = styles.matchedCSSRules; > + for (var i = 0; i < cssRules.length; ++i) { > + var rule = cssRules[i]; > + if (rule.style.id && !this._idToOriginalStyle[rule.style.id]) > + this._idToOriginalStyle[rule.style.id] = WebInspector.CSSStyleDeclaration.parseStyle(rule.style); // only for bound styles > + } Typically we use the `in` syntax here instead of checking if the "slot" in the lookup table is undefined. We normally do: if (rule.style.id && !(rule.style.id in this._idToOriginalStyle)) This way, if we ever want to store values like "", 0, null, undefined, false, etc it would still be possible to store and retrieve those values. For example: var obj = { x:false }; obj["x"]; //=> false "x" in obj; //=> true I see you use this approach later on. Is there an advantage to not using this approach here? > +WebInspector.CSSStyleModel.StyleSheetDelta.prototype = { > + update: function(changedProperties, newStyle) > + { > ... > + var uniqueProperties = {}; > + for (var i = 0; i < changedProperties.length; ++i) { > + changedPropertiesSet[changedProperties[i]] = true; > + uniqueProperties[changedProperties[i]] = true; > + } > + for (var i = 0; i < newStyle.length; ++i) > + uniqueProperties[newStyle[i]] = true; > + for (var i = 0; i < originalStyle.length; ++i) > + uniqueProperties[originalStyle[i]] = true; > + for (var i = 0; i < changedProperties.length; ++i) > + uniqueProperties[originalStyle[i]] = true; I don't understand this loop here. I think it should be removed. I think it should be covered by the top loop shown here. > + getAssociatedShorthand: function(explicitLonghand) > + { > + if (explicitLonghand.charAt(0) === "-") > + return ""; > + var lastDashIdx = explicitLonghand.lastIndexOf("-"); > + if (lastDashIdx > 0) > + return explicitLonghand.substring(0, lastDashIdx); > + return ""; > + }, We typically do not shorten / abbreviate variable names. "lastDashIdx" => "lastDashIndex". > + if (propertyDelta.newValue === propertyDelta.oldValue && propertyDelta.newPriority === propertyDelta.oldPriority && !propertyDelta.disabled) { Might be useful to make this a function. > + patchRuleBody: function(originalBody, propertyDiffs, tokenizer) > + var currentPropertyType = ""; > + var whitespaceHandling = "normal"; currentPropertyType gets three values: "", deleted, changed whitespaceHandling gets three values: normal, copy, and skip. It might be worth changing these from strings to integer constants. I don't know how optimized JavaScript is in these string comparison cases, but having all caps constants would make this more readable to me. > + console.log(propertyName + ":" + propertyDiffs[propertyName].newValue + newPrioritySuffix(propertyDiffs[propertyName]));' Is this intended? > + getPatchedStyleSheet: function(styleSheetDelta, originalData, tokenizer) > + { > + function sorter(leftEntry, rightEntry) > + { > + return leftEntry.originalStyle.bodyStartOffset < rightEntry.originalStyle.bodyStartOffset; > + } In JavaScript, sort functions should return integer values. Typically -1, 0, and 1. Booleans don't work so well, for example: js> [4, 8, 1, 9].sort(function(a,b) { return a < b; }); [4, 8, 1, 9] The common solution is to just change "<" to "-", like so: js> [4, 8, 1, 9].sort(function(a,b) { return a - b; }); [1, 4, 8, 9]

The whole point for this feature to me is that I can copy/paste whatever is changed into my CSS editor. So, please... DON'T give me a diff with + and - characters in there. That would not help much. Just give me a summary of the CSS rules that I edited with all its styling. So, if I have this originally: .myclass { left: 10; width: 5; } And I edit it in the browser and add this: height: 1px; Then I want to see this in the diffing overview: .myclass { left: 10; width: 5; heigt: px; } Perhaps you can use a different color for the added line but now I can just copy/paste this rule and replace my older version in one go. No carefully figuring out what has changed, removing + characters etc. THAT would help me a lot. Cheers.

> > THAT would help me a lot. What you describe is the end goal. This change is just a single step in achieving it. Wait and see what we offer here!

(In reply to comment #4) > (From update of attachment 62573 [details]) > Some minor comments. I didn't look too intently at the parsing this pass. > > > +++ b/LayoutTests/inspector/styles-edit-rewrite-expected.txt > > +.foo { > > + color: green !important; > > + border: solid rgb(0, 255, 0); > > +border-left-width: 3px !important; > > +border-color: rgb(0, 255, 0); > > +} > > I would expect leading whitespace for these last two. This output is generated by the patching algorithm. It has no way to learn how many spaces to prefix user-added properties. Do you have a good way to fix this? > > > > diff --git a/LayoutTests/inspector/styles-edit-rewrite.html b/LayoutTests/inspector/styles-edit-rewrite.html > > + if (resource.url.indexOf("styles-edit-rewrite.css") != -1) { > > Use !== or === wherever possible. In this case, !== is appropriate. Fixed. > > > +++ b/WebCore/inspector/front-end/CSSStyleModel.js > > + _updateStyles: function(styles) > > + { > > + if (styles.inlineStyle && !this._idToOriginalStyle[styles.inlineStyle.id]) > > + this._idToOriginalStyle[styles.inlineStyle.id] = WebInspector.CSSStyleDeclaration.parseStyle(styles.inlineStyle); > > + > > + var cssRules = styles.matchedCSSRules; > > + for (var i = 0; i < cssRules.length; ++i) { > > + var rule = cssRules[i]; > > + if (rule.style.id && !this._idToOriginalStyle[rule.style.id]) > > + this._idToOriginalStyle[rule.style.id] = WebInspector.CSSStyleDeclaration.parseStyle(rule.style); // only for bound styles > > + } > > Typically we use the `in` syntax here instead of checking if the > "slot" in the lookup table is undefined. We normally do: > > if (rule.style.id && !(rule.style.id in this._idToOriginalStyle)) > > This way, if we ever want to store values like "", 0, null, undefined, > false, etc it would still be possible to store and retrieve those > values. For example: > > var obj = { x:false }; > obj["x"]; //=> false > "x" in obj; //=> true > > I see you use this approach later on. Is there an advantage to not > using this approach here? We explicitly use 0 as an invalid id. That's why I'm using a simple ! without "in" here. Might as well use "in" for consistency. > > +WebInspector.CSSStyleModel.StyleSheetDelta.prototype = { > > + update: function(changedProperties, newStyle) > > + { > > ... > > + var uniqueProperties = {}; > > + for (var i = 0; i < changedProperties.length; ++i) { > > + changedPropertiesSet[changedProperties[i]] = true; > > + uniqueProperties[changedProperties[i]] = true; > > + } > > + for (var i = 0; i < newStyle.length; ++i) > > + uniqueProperties[newStyle[i]] = true; > > + for (var i = 0; i < originalStyle.length; ++i) > > + uniqueProperties[originalStyle[i]] = true; > > + for (var i = 0; i < changedProperties.length; ++i) > > + uniqueProperties[originalStyle[i]] = true; > > I don't understand this loop here. I think it should be removed. I > think it should be covered by the top loop shown here. Ugh, you've found a bug :) > > > + getAssociatedShorthand: function(explicitLonghand) > > + { > > + if (explicitLonghand.charAt(0) === "-") > > + return ""; > > + var lastDashIdx = explicitLonghand.lastIndexOf("-"); > > + if (lastDashIdx > 0) > > + return explicitLonghand.substring(0, lastDashIdx); > > + return ""; > > + }, > > We typically do not shorten / abbreviate variable names. > "lastDashIdx" => "lastDashIndex". Fixed > > > + if (propertyDelta.newValue === propertyDelta.oldValue && propertyDelta.newPriority === propertyDelta.oldPriority && !propertyDelta.disabled) { > > Might be useful to make this a function. Done > > + patchRuleBody: function(originalBody, propertyDiffs, tokenizer) > > + var currentPropertyType = ""; > > + var whitespaceHandling = "normal"; > > currentPropertyType gets three values: "", deleted, changed > whitespaceHandling gets three values: normal, copy, and skip. > > It might be worth changing these from strings to integer constants. > I don't know how optimized JavaScript is in these string comparison > cases, but having all caps constants would make this more readable to me. Introduced local enums. Is there anything better we can use here? > > + console.log(propertyName + ":" + propertyDiffs[propertyName].newValue + newPrioritySuffix(propertyDiffs[propertyName]));' > > Is this intended? No, just logging. This is a preliminary non-r? patch for pfeldman@ to have a look. You appeared to be faster :) > > + getPatchedStyleSheet: function(styleSheetDelta, originalData, tokenizer) > > + { > > + function sorter(leftEntry, rightEntry) > > + { > > + return leftEntry.originalStyle.bodyStartOffset < rightEntry.originalStyle.bodyStartOffset; > > + } > > In JavaScript, sort functions should return integer values. Typically Oops, thanks for catching this...

Created attachment 62663 [details] [PATCH] Preliminary implementation, comments fixed

(In reply to comment #5) > The whole point for this feature to me is that I can copy/paste whatever is changed into my CSS editor. The whole point, as I see it, to save changed CSS. I'm curious, can we skip the copy/paste part? Can we just save changes (at least, in case of CSS file located in file://)? Does Inspector have write access to local files?

Created attachment 63225 [details] [PATCH] Preliminary implementation, factored out EditedStyleSheet

What is the UI for this?

Created attachment 63226 [details] [IMAGE] Screenshot of original and patched external CSS stylesheet sources To the right, the original and modified styles in the Styles sidebar pane are shown.

Created attachment 63722 [details] [PATCH] Introduced changes made by pfeldman offline

Comment on attachment 63722 [details] [PATCH] Introduced changes made by pfeldman offline Review for CSSStyleSheetDelta.js WebCore/inspector/front-end/CSSStyleSheetDelta.js:33 + this._idToStyleEntryMap = idToStyleEntryMap || {}; What is "Entry"? When do you need non-empty idToStyleEntryMap? WebCore/inspector/front-end/CSSStyleSheetDelta.js:41 + result.styleEntryMap[id] = { originalStyle: WebInspector.CSSStyleDeclaration.parseStyle(payloadEntry.originalStylePayload), deltas: payloadEntry.deltas }; What is the structure of a payload? Where is it defined? Are there other entities involved with the 'deltas', etc.? WebCore/inspector/front-end/CSSStyleSheetDelta.js:47 + get styleEntryMap() There is no benefit in encapsulating the map and providing full access to it. Did you mean getEntryForId? What is this entry, again? WebCore/inspector/front-end/CSSStyleSheetDelta.js:57 + var propertyDeltas = styleData.deltas; So StyleSheetDelta contains a map of StyleDatas that contain property deltas? Lets come up with good names for the properties. What does styleData mean by the way? Is this CSSStyleDeclration or Payload or anything else? Is the structure for this 'data' defined in any class? WebCore/inspector/front-end/CSSStyleSheetDelta.js:58 + var originalStyle = styleData.originalStyle; "style" is overloaded. Is this declaration? Or a payload? WebCore/inspector/front-end/CSSStyleSheetDelta.js:82 + function setLonghandDeletedDelta(name, disabled) Now that this code resides in its own class, you can define these as private prototype members. WebCore/inspector/front-end/CSSStyleSheetDelta.js:151 + getAssociatedShorthand: function(explicitLonghand) Describing what requires this heuristics would be a plus. Which ones do you need to distinguish? Maybe we can fix it in the native code and/or expose additional API. WebCore/inspector/front-end/CSSStyleSheetDelta.js:164 + updateDelta: function(originalStyle, newStyle, propertyName, isShorthand, propertyDeltas) should be private WebCore/inspector/front-end/CSSStyleSheetDelta.js:184 + _isPropertyChanged: function(propertyDelta) This should be defined on the property delta class. WebCore/inspector/front-end/CSSStyleSheetDelta.js:202 + maskAbsent: function(value) private? WebCore/inspector/front-end/CSSStyleSheetDelta.js:218 + styleCopy.__proto__ = WebInspector.CSSStyleDeclaration.prototype; no proto assignments please WebCore/inspector/front-end/CSSStyleSheetDelta.js:207 + trackStyle: function(style) What is this used for? Name is confusing. WebCore/inspector/front-end/CSSStyleSheetDelta.js:222 + asPayload: function() This should be defined next to parse. Maybe we should call them 'toPayload' and 'fromPayload' ? WebCore/inspector/front-end/DOMAgent.js:661 + width: this.width, Do you really need to serialize width and height? It used to be a hack for the metrics panel.

*** Bug 47908 has been marked as a duplicate of this bug. ***

Bug 50088 implements the highlighting of stylesheet revision diffs. Bug 50107 implements the ability to activate (switch to) specific modified stylesheet revisions. Any stylesheet revision text contents can be exported by dragging the respective resource tree node into any application that supports DnD of type "text/plain" (e.g., OpenOffice.org Writer). Hence, closing this bug as FIXED.