For as-authored we'll need to be able to optionally parse comments from parseDeclarations; and we'll need to be able to do text rewriting based on parsed declarations.

A few notes on this patch - * While this patch has been generally stable for a while now, it's possible that it will need some updates, depending on how exactly some of the editing corner cases in bug 984880 are resolved. For example this patch implements a property-validity heuristic when parsing comment text; it's possible this would have to change. * rewriteDeclarations takes the extended form of objects constructed by RuleModificationList. However, RuleModificationList itself isn't updated until a later patch. The test case included here should be clear enough; or refer to the relevant patch ("as-authored styles in the rule view") in bug 984880.

Fix up the test so it works.

Comment on attachment 8648836 [details] [diff] [review] make parseDeclarations handle comments; add rewriteDeclarations Review of attachment 8648836 [details] [diff] [review]: ----------------------------------------------------------------- There's a lot of complicated parsing going on here. Great work for this! I don't have the full picture for rewriteDeclarations, but as you said, the test helps understand, so that's fine. I haven't seen anything too complex and haven't detected any huge problems. You said this patch has been stable locally for you for a while, so I trust you've done more tests to ensure it works fine than I can do while reviewing now. I can only suggest that you move the comment parsing heuristic to a separate function so that if we do decide to change it, we can easily. ::: browser/devtools/styleinspector/css-parsing-utils.js @@ +24,5 @@ > + * @param {String} CSS source string > + * @yield {CSSToken} The next CSSToken that is lexed > + * @see CSSToken for details about the returned tokens > + */ > +function* tokenizeCSSWithComments(string) { I think by now, css-tokenizer.js and css-parsing-utils.js should be merged into one file, and maybe some functions should be renamed (tokenizeCSSWithComments and cssTokenizer could possibly just be one function, with a parameter to include or exclude comments). Can you file a bug for later if you agree with this? @@ +134,5 @@ > + // When parsing a comment body, if the left-hand-side is not a > + // valid property name, then drop it and stop parsing. > + if (inComment) { > + try { > + DOMUtils.cssPropertyIsShorthand(lastProp.name); I'm not very familiar with this function, its name seems to suggest it only works with shorthand properties. What if a valid longhand property is passed, will the try/catch behave as expected? Also, I wonder if it'd be better to externalize the valid-declaration-in-comment heuristic to a separate function, so that it's obvious to the reader where is the heuristic implemented and maybe easier to change it later if needed. If we do this, we should probably be passing it the full comment input string so it can decide to walk over token any way it wants. ::: browser/devtools/styleinspector/test/unit/test_parseDeclarations.js @@ +254,4 @@ > }, > + > + // Simple embedded comment test. > + {parseComments: true, nit: can you format these object properties like in previous test cases: // Simple embedded comment test. { parseComments: true, input: ... @@ +270,5 @@ > + offsets: [45, 61]}]}, > + > + // Embedded comment where the parsing heuristic is a bit funny. > + {parseComments: true, > + input: "width: 5; /* background: */ background: red;", It's not obvious to me how the following would be parsed, could you add another test case for it: "width: 5; /* background: yellow */ background: red;" ::: browser/devtools/styleinspector/test/unit/test_rewriteDeclarations.js @@ +80,5 @@ > + instruction: {type: "enable", name: "p", value: false, index: 0}, > + expected: "/* p:v; */" > + }, > + { > + input: "p:v;", This test case is exactly identical to the previous one.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3) > I think by now, css-tokenizer.js and css-parsing-utils.js should be merged > into one file, and maybe some functions should be renamed > (tokenizeCSSWithComments and cssTokenizer could possibly just be one > function, with a parameter to include or exclude comments). > Can you file a bug for later if you agree with this? Will do. > > + DOMUtils.cssPropertyIsShorthand(lastProp.name); > > I'm not very familiar with this function, its name seems to suggest it only > works with shorthand properties. > What if a valid longhand property is passed, will the try/catch behave as > expected? Yeah. This is a bit of a hack. What the call does it just ignore the result -- we're actually only checking to see if the call throws an exception, to see whether the property name is valid at all. Note that this required a separate bug fix, see bug 1187409. I've added that as a dependency of this bug as well. I'll add a comment explaining what is going on here when I pull this out into a separate function. > It's not obvious to me how the following would be parsed, could you add > another test case for it: > > "width: 5; /* background: yellow */ background: red;" I'll add the test. The comment parser has two heuristics -- one is that the property name must be valid; and the other is that the value must end with a ";". So here, the comment should not result in a property. > ::: browser/devtools/styleinspector/test/unit/test_rewriteDeclarations.js > @@ +80,5 @@ > > + instruction: {type: "enable", name: "p", value: false, index: 0}, > > + expected: "/* p:v; */" > > + }, > > + { > > + input: "p:v;", > > This test case is exactly identical to the previous one. Probably a merge mistake on my part but I will check.

(In reply to Tom Tromey :tromey from comment #4) > > It's not obvious to me how the following would be parsed, could you add > > another test case for it: > > > > "width: 5; /* background: yellow */ background: red;" > > I'll add the test. > The comment parser has two heuristics -- one is that the property name > must be valid; and the other is that the value must end with a ";". > So here, the comment should not result in a property. I must be remembering some old version of the heuristic, because now it clearly allows the lack of a ";" and there is even another test for this. Bah. So, thanks for requesting this test.

Updated per review. However I think this one may require some future work after all, because I misremembered the comment-parsing heuristic; and, since we accept declarations in comments without a ";" we're going to need some way to terminate those. Note to self to also test bad_string and bad_url parses in comments. Those should probably just be rejected.

I also realized today that we do not currently handle <!-- .. -->, but should. This will have to be a second style of comment parsing. I'm inclined to reject comments-in-comments, like: <!-- color: red /* border-width: 25px; */ --> I suspect supporting this would make the rewriter unreadable.

So much for this patch being relatively stable :) This version adds handling of newline and indentation insert, semicolon insertion, and fixes the comment bugs pointed out in bug 984880 comment 86. This still doesn't handle HTML comments or other cases of missing terminators. It still needs an update to compute the default indentation (see the FIXME). I think for ease of testing I will make this a parameter to be supplied by the caller; and I will add another bug to break editor.js:detectIndentation out into a new file and make it suitable for use on a string. In fact, some of those cases can't be reliably recognized without some improvements in nsCSSScanner. In particular some forms of unterminated URL are reported as ordinary URLs, and I think finding the missing terminator(s) is pretty much equivalent to writing a new lexer. See test_csslexer.js for at least one example (though another one, I think, is a trailing "\"). Note to self -- maybe we can expose AppendImpliedEOFCharacters.

(In reply to Tom Tromey :tromey from comment #7) > I also realized today that we do not currently handle <!-- .. -->, but > should. Why do you think we should support these invalid comments? Shouldn't we just skip them?

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #9) > (In reply to Tom Tromey :tromey from comment #7) > > I also realized today that we do not currently handle <!-- .. -->, but > > should. > Why do you think we should support these invalid comments? Shouldn't we just > skip them? I saw them as analogous to /* */ comments, which we attempt to parse. If you think skipping them is ok, I will do that instead.

I don't think they're part of the CSS syntax, and in my experience, no-one use them. Skipping them sounds ok to me.

Here is an updated version of the patch. Now it relies on bug 1196896. I've added a lot of things. There are tests for all of them. * Parsing now records any missing terminators, so that rewriting can supply them as needed. E.g., if you have background: url("something then rewriting will turn this into background: url("something"); * If we see a comment like /*! .... */ then we skip the heuristic that checks the property name. This avoids the problem where disabling and then re-enabling an invalid property caused errors. The rewriter now generates comments like this. * Escaped comments could sometimes cause declarations to be incorrect. This is fixed now. There is still one more issue that has to be addressed here, which is the enabling of "very invalid" properties. Suppose you see: /* color: }; */ The current comment-parser will pick this up, but if you enable it, you will break your entire style sheet (I think) -- which would be a problem first because the rule editor knows that edits don't affect the number of rules, and so won't adapt properly; and second because it's just unfriendly to users. Since the same issue arises with editing, I may keep rewriteDeclarations at the roughly current level of intelligence, expand the "enable" instruction to accept new text, and require some higher layer to sanitize the input.

I believe this version is feature-complete. In addition to the things mentioned in comment #8, this: * Ignores HTML comments. * Terminates incomplete declarations using information now exposed on CSSLexer (see bug 1196896). * Splits out the bulk of parseDeclarations to avoid exposing an overly complex API. * Fixes a bug where comment parsing could yield incorrect offsets sometimes. This is handled by the new function parseCommentDeclarations. * Sanitizes bad property values when rewriting. In particular, attempting to set a property to a value that includes an unpaired "}" will now quote the "}". Sanitizations are reported to the caller of rewriteDeclarations; in a later patch the rule view uses this to update properties when such rewriting is done. There are tests for all of these things. You may find non-obvious things in rewriteDeclarations in particular; these should correspond to some particular test case.

One thing from my notes that I forgot to mention is an odd case in comment parsing. Suppose you have this css: div { color: blue /* color: red; */ } Here you might expect to see two declarations, but due to the missing ";" you will only see one. This is fixable with even more hair in parseDeclarations. In particular we could keep track of all the comments when looking for the ";", and then parse them in the "no-;" case in the epilogue. I considered this not worth bothering about, but I'm happy to implement it if you think it's worthwhile.

Comment on attachment 8651885 [details] [diff] [review] make parseDeclarations handle comments; add rewriteDeclarations Review of attachment 8651885 [details] [diff] [review]: ----------------------------------------------------------------- These are really huge functions! But I don't think splitting any further is going to help make the code simpler. I have a few comments below, but I'm going to R+ this anyway, I don't think I need to review this again after you've changed it. ::: browser/devtools/styleinspector/css-parsing-utils.js @@ +62,5 @@ > + if (!token) { > + break; > + } > + if (token.tokenType === "htmlcomment" > + && text.substring(token.startOffset, token.endOffset) === "-->") { Ah, I had no idea html comments were recognized by the lexer as a token. I went to the code and found this: // HTML comment delimiters, either "<!--" or "-->". Note that each // is emitted as a separate token, and the intervening text is lexed // as normal; whereas ordinary CSS comments are lexed as a unit. And it occurred to me that back when <style> tags weren't supported by all browsers, developers might have been inserting html comments to prevent the CSS code from appearing in the page: <style> <!-- body {color: red} --> </style> Knowing this, it's normal that the lexer knows about html comments and continues to emit token for things inside html comments. In fact, I just tested this code in firefox dev-edition, and the text is red. So html comments should indeed be ignored completely and the content should be parsed as normal css code. @@ +195,2 @@ > */ > +function parseDeclarationsWorker(inputString, parseComments, This name might confuse someone into thinking this runs in a worker. I know it's hard, but it'd be better to find another name for this function if it doesn't run in a worker. (btw, performance intensive things can run in workers if that helps, we have a handy loader for this: toolkit\devtools\shared\worker.js). @@ +203,5 @@ > > + let declarations = [{name: "", value: "", priority: "", > + terminator: "", > + offsets: [undefined, undefined], > + colonOffsets: false}]; Can you move the construction of this template object to a small helper function just above? So that you can call it again later when you push a new element in the declarations array. function getEmptyDeclaration() { return {name: "", value: "", priority: "", terminator: "", offsets: [undefined, undefined], colonOffsets: false}; } let declarations = [getEmptyDeclaration()]; ... and later in the code ... declarations.push(getEmptyDeclaration()); There are chances that we need to add/remove/change the properties in this objects in the future, so this will help. @@ +288,5 @@ > + token.endOffset); > + > + // Insert the new declarations just before the final element. > + let lastDecl = declarations.pop(); > + Array.prototype.push.apply(declarations, newDecls); Why does declarations.push(newDecls) not work here? @@ +325,5 @@ > return declarations; > } > > /** > + * Returns an array of CSS declarations given an string. s/an string/a string @@ +364,5 @@ > +/** > + * A helper function for rewriteDeclarations that computes the > + * indentation of some text. > + * @param {string} string the input text > + * @param {integer} offset the offset at which to compute the indentation Js integer type is Number, so @param {Number} offset The offset at which to compute the indentation @@ +387,5 @@ > +} > + > +/** > + * A helper function for rewriteDeclarations that modifies a property > + * value to ensure it is"lexically safe" for insertion into a style sheet. s/is"lexically safe"/is "lexically safe" @@ +499,5 @@ > + // invalid, so this function must check for that. > + let maybeTerminateDecl = function(index) { > + if (index >= 0 && index < declarations.length > + // No need to rewrite declarations in comments. > + && !("commentOffsets" in declarations[index])) { Can you reverse this condition and early return instead? This avoids having to indent the whole function body. if (index < 0 || index >= declarations.length || ("commentOffsets" in declarations[index])) { return; } @@ +504,5 @@ > + let termDecl = declarations[index]; > + let endIndex = termDecl.offsets[1]; > + // Due to an oddity of the lexer, we might have gotten a bit of > + // extra whitespace in a trailing bad_url token -- so be sure to > + // skip that as well. Couldn't this be handled by parseDeclarationsWorker instead? It doesn't feel right to have to deal with lexer oddities here whereas the lexer isn't used in this function, it's hidden in parseDeclarationsWorker. @@ +559,5 @@ > + copyOffset = decl.offsets[1]; > + if (instruction.value) { > + // Enable it. First see if the comment start can be deleted. > + let commentStart = decl.commentOffsets[0]; > + if (/^\/\*!?[ \r\n\t\f]*$/.test(result.substring(commentStart))) { I don't have a strong opinion on this, but most modules that use regexps do define them as constants at the top instead, so might be a good thing to do here too (and in other places in this file). ::: browser/devtools/styleinspector/test/unit/test_escapeCSSComment.js @@ +35,5 @@ > + ++i; > + do_print("Test #" + i); > + > + let escaped = escapeCSSComment(test.input); > + do_check_eq(escaped, test.expected); You can use 'equal' instead. @@ +37,5 @@ > + > + let escaped = escapeCSSComment(test.input); > + do_check_eq(escaped, test.expected); > + let unescaped = _unescapeCSSComment(escaped); > + do_check_eq(unescaped, test.input); You can use 'equal' instead. ::: browser/devtools/styleinspector/test/unit/test_parseDeclarations.js @@ +389,5 @@ > + deepEqual(output, test.expected); > + } > +} > + > +function run_test() { Maybe move run_test up, so it's just after the TEST_DATA array, before run_basic_tests. ::: browser/devtools/styleinspector/test/unit/test_rewriteDeclarations.js @@ +364,5 @@ > +function run_test() { > + let i = 0; > + for (let test of TEST_DATA) { > + ++i; > + do_print("Test #" + i); Just a thought, no need to do this unless you're revisiting this patch: add 'desc' properties to the test objects which are strings (that contain the line comments you added in the TEST_DATA array) and print them instead. @@ +367,5 @@ > + ++i; > + do_print("Test #" + i); > + let {changed, text} = rewriteDeclarations(test.input, test.instruction, > + "\t"); > + do_check_eq(text, test.expected); I recently learned that you can use 'equal', and 'ok' in xpcshell tests now.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #16) > These are really huge functions! But I don't think splitting any further is > going to help make the code simpler. I wasn't too concerned about it, but while fixing yet another buglet today, I realized it really is out of hand. I am thinking I will turn the rewriter into an object with a bunch of methods, to split things up more clearly.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #16) > > + // Insert the new declarations just before the final element. > > + let lastDecl = declarations.pop(); > > + Array.prototype.push.apply(declarations, newDecls); > > Why does declarations.push(newDecls) not work here? There may be multiple declarations and we want to append them all.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #16) > > + let termDecl = declarations[index]; > > + let endIndex = termDecl.offsets[1]; > > + // Due to an oddity of the lexer, we might have gotten a bit of > > + // extra whitespace in a trailing bad_url token -- so be sure to > > + // skip that as well. > > Couldn't this be handled by parseDeclarationsWorker instead? It doesn't feel > right to have to deal with lexer oddities here whereas the lexer isn't used > in this function, it's hidden in parseDeclarationsWorker. The background here is that CSSLexer will cause trailing whitespace to be treated as part of a url token, when the token is unterminated. E.g., "url(something " is a single token. This comes up in the rewriter when termination is needed; for example when enabling a commented-out property like /* background: url(something.jpg */ The heuristic here is based on the idea that the whitespace is probably not what the user intended. I suppose this could be handled in parseDeclaration, say by editing the text before adding it to "current". However, in this situation it is mildly hard to notice the bad url token (CSSLexer often hands out "url" tokens instead), and, because the situation only matters when rewriting, I considered it better to put the hack here.

Updated for review comments. I replied to some of the comments in other comments in the bug. This version changes rewriteDeclarations to use a helper object. I think this cleans things up somewhat -- at least it isn't a 200 line function any more -- but it's easy to revert if you liked the earlier approach. I'm afraid that due to the extensive refactoring I'm going to ask you to review it again.

Comment on attachment 8654326 [details] [diff] [review] make parseDeclarations handle comments; add rewriteDeclarations Much later I realized that this refactoring is just a short step from just letting the rewriter replace RuleModificationList when dealing with an authored-aware server. So, I'm rescinding this review request so I can implement that idea first.

Refactored a bit more so that RuleRewriter has the same API as RuleModificationList. I think this is somewhat simpler to follow now as the rewriting is split up into several functions.

Comment on attachment 8655642 [details] [diff] [review] make parseDeclarations handle comments; add rewriteDeclarations This has the refactoring I mentioned in an earlier comment; and I think addresses all previous review comments.

(In reply to Tom Tromey :tromey from comment #18) > (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #16) > > > > + // Insert the new declarations just before the final element. > > > + let lastDecl = declarations.pop(); > > > + Array.prototype.push.apply(declarations, newDecls); > > > > Why does declarations.push(newDecls) not work here? > > There may be multiple declarations and we want to append them all. Right, sorry I missed this. But then you could do: declarations = declarations.concat(newDecls); or declarations = [...declarations, ...newDecls]; I find these easier to read than applying a prototype function, this always messes with my brain.

Comment on attachment 8655642 [details] [diff] [review] make parseDeclarations handle comments; add rewriteDeclarations Review of attachment 8655642 [details] [diff] [review]: ----------------------------------------------------------------- Ok, giving this another round of review. I'm going to assume that only css-parsing-utils.js has changed though (I haven't tried to interdiff the patches, I don't trust splinter for this). I've made a bunch of comments, all of them are minor, and so I'm going to R+ this again. This patch is complex, it really takes time to read these functions are be comfortable with what each of them does, especially with all the edge cases, indentation, comments, etc... But as I said earlier, that's a complexity we need to deal with for this project, and so this code is needed. So I have no problem with this. Also, the various refactorings you've done do help. I haven't seen really big functions so maintenance will be easier if there are problems in the future. I want to insist on the fact that, since this is complex, it's kind of hard to see the entry point and how consumer code would use this all. So I made a comment about adding a usage example in the RuleRewriter. You could also elaborate a bit more at the top of the css-parsing-utils file with a comment that explains what are the interesting things to use in this module, and what are the main (current) consumers of it. More importantly, putting myself in the shoes of someone having to work on this in the future and having never seen it before, reading tests would help a lot. So test_rewriteDeclarations.js is really good in that respect. ::: browser/devtools/styleinspector/css-parsing-utils.js @@ +37,5 @@ > + * input string > + * @return {String} the escaped result > + */ > +function escapeCSSComment(inputString) { > + let result = inputString.replace(/\/(\\*)\*/g, "/\\$1*"); I was going to suggest that you also moved the regexps in escapeCSSComment and unescapeCSSComment to const at the top, but I think it would hurt readability here, especially because you're using replace, so let's keep them here. @@ +74,5 @@ > + * to decide whether a given bit of comment text should be parsed as a > + * declaration. > + * > + * @param {String} name the property name that has been parsed > + * @return {boolean} true if the property should be parsed, false if nit: s/boolean/Boolean @@ +108,5 @@ > +function parseCommentDeclarations(commentText, startOffset, endOffset) { > + let commentOverride = false; > + if (commentText === "") { > + return []; > + } else if (commentText[0] === "!") { Can you make this a const? else if (commentText[0] === COMMENT_PARSING_HEURISTIC_BYPASS_CHAR) @@ +194,5 @@ > + * > + * The return value and arguments are like parseDeclarations, with > + * these additional arguments. > + * > + * @param {boolean} inComment s/boolean/Boolean @@ +199,5 @@ > + * If true, assume that this call is parsing some text > + * which came from a comment in another declaration. > + * In this case some heuristics are used to avoid parsing > + * text which isn't obviously a series of declarations. > + * @param {boolean} commentOverride s/boolean/Boolean @@ +216,3 @@ > > + let declarations = [getEmptyDeclaration()]; > + let lastProp = declarations[declarations.length - 1]; declarations has to be an array with just one item at this stage, so you could replace this line with: let lastProp = declarations[0]; @@ +223,5 @@ > + if (!token) { > + break; > + } > + > + // Ignore HTML comments. Maybe this comment should read: Skip HTML comment tokens (the comment content will be parsed tough). @@ +290,5 @@ > + > + // Insert the new declarations just before the final element. > + let lastDecl = declarations.pop(); > + Array.prototype.push.apply(declarations, newDecls); > + declarations.push(lastDecl); let lastDecl = declarations.pop(); declarations = [...declarations, ...newDecls, lastDecl]; @@ +371,5 @@ > + * as @see RuleModificationList. > + * > + * @param {StyleRuleFront} rule The style rule to use. > + * @param {String} inputString The CSS source text to parse and modify. > + * @return {Object} an object that can be used to rewrite the input text. Can you add a usage example in this comment. No need to do something long, but at least something that describes how to instantiate and make use of the main methods of this class. * Example * let ruleRewriter = new RuleRewriter(ruleFront, "color:red;); * ruleRewriter.setPropertyEnabled(0, "color", false); * ruleRewriter.apply().then(() => {}); @@ +398,5 @@ > + * @param {Number} index The index of the property to modify > + */ > + completeInitialization: function(index) { > + if (index < 0) { > + throw new Error("invalid index"); throw new Error("Invalid index " + index + ". Expected positive integer") @@ +400,5 @@ > + completeInitialization: function(index) { > + if (index < 0) { > + throw new Error("invalid index"); > + } > + this.index = index; Not sure if setting this property is needed. Maybe I've missed some spots, but when I see |this.index| being used, I also see |this.completeInitialization(index)| being called, so the caller could use |index| instead of |this.index| and you could drop this. @@ +419,5 @@ > + * @param {string} string the input text > + * @param {Number} offset the offset at which to compute the indentation > + * @return {string} the indentation at the indicated position > + */ > + getIndentation: function(string, offset) { What's the relationship between this and the indentation detection module you created in another bug? Sorry I'm a bit lost here with regards to the order these things are going to be checked-in, but shouldn't you be using the new module here instead of having to write this code? @@ +555,5 @@ > + } > + }, > + > + /** > + * Sanitize the given property value given and return the Santinize the given property value and return the ... @@ +589,5 @@ > + > + /** > + * Enable or disable a declaration > + * > + * @param {number} index index of the property in the rule. s/number/Number There may be other occurrences of this throughout this file. This is really a minor remark, feel free to ignore unless you're revisiting this patch anyway. I think this is the way we're documenting types in other places of the code, by using the constructor name (capitalized) @@ +591,5 @@ > + * Enable or disable a declaration > + * > + * @param {number} index index of the property in the rule. > + * @param {string} name current name of the property > + * @param {boolean} value true if the property should be enabled; s/value/isEnabled This describes better what this param does. Also because value, along with name, could be confused with a declaration (name:value;)

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #25) > I'm going to assume that only css-parsing-utils.js has changed though (I > haven't tried to interdiff the patches, I don't trust splinter for this). Yes, I think that is correct; aside of course from the (generally minor) changes I made to the test cases while updating the code. > > + getIndentation: function(string, offset) { > > What's the relationship between this and the indentation detection module > you created in another bug? Sorry I'm a bit lost here with regards to the > order these things are going to be checked-in, but shouldn't you be using > the new module here instead of having to write this code? I'm sorry about the confusion here. I've added a comment to getIndentation to try to clarify it a bit. The idea here is that the first choice of indentation for the rewriter is to use the indentation of another declaration in the same rule. I think this is more likely to give good results in general than the heuristic that looks at the text of the style sheet. However, this can fail if there aren't any other declarations in the rule. In this case we fall back to using the default indentation, which is computed using the indentation.js code.

Updated per review.

This update changes parseDeclarations to attribute whitespace after a ":" to the colon's end offset. Whitespace here isn't considered as part of the value; and this change makes it so that it is preserved over edits without requiring hacks in the rule view.

This version has a few changes: * Fix a bug when parsing declarations in a comment. Previously, any text with a ";" would cause a nameless declaration to be created; but this is the wrong thing to do in a comment. This was found by the existing test suite (parsing user agent styles of all things) but I've also added a unit test for this. * Change RuleRewriter.createProperty to automatically call getDefaultIndentation. This is in response to https://bugzilla.mozilla.org/show_bug.cgi?id=984880#c120 * Updates required by changes requested in bug 1196896.

Comment on attachment 8662626 [details] [diff] [review] make parseDeclarations handle comments; add rewriteDeclarations I was unsure if my recent changes were significant enough to warrant re-review; so erring on the side of caution. The changes are described in comment #28 and comment #29.

Comment on attachment 8662626 [details] [diff] [review] make parseDeclarations handle comments; add rewriteDeclarations Review of attachment 8662626 [details] [diff] [review]: ----------------------------------------------------------------- Bugzilla isn't being very helpful with interdiffs here, but I didn't see anything in the code worth commenting on related to what you described in comment 28 and comment 29. This still seems good to me. I particularly like the volume of unit tests you added.

Updated for the big devtools renaming.

I noticed a no-op call to completeCopying that was left over from an earlier approach. This removes it.

RuleRewriter.apply was missing a "return", which caused some style inspector tests to fail with e10s.