I noticed that output-parser.js can rewrite url()s. In particular _appendURL adds quotes, even if there were none before.

Comment on attachment 8651953 [details] [diff] [review] preserve spelling of urls in output-parser.js Review of attachment 8651953 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/test/browser_outputparser.js @@ +135,5 @@ > > target.innerHTML = ""; > } > + > +function testParseURL(doc, parser) { Can you add an info in here, so it's easy to follow where the test is when reading logs: info("Testing that URL parsing preserve authored quotes"); @@ +143,5 @@ > + urlClass: "test-urlclass" > + }); > + > + let target = doc.querySelector("div"); > + ok(target, "captain, we have the div"); This part of the test isn't really about testing that the test div exists. Could you remove this assertion? @@ +146,5 @@ > + let target = doc.querySelector("div"); > + ok(target, "captain, we have the div"); > + target.appendChild(frag); > + > + is(target.innerHTML, url, "URL correctly parsed"); And make sure the comment in this assertion is unique so it's easy to know which of the steps failed when reading logs: is(target.innerHTML, url, "URL with " + quote + " quotes correctly parsed"); ::: toolkit/devtools/output-parser.js @@ +426,5 @@ > this._appendNode("a", { > target: "_blank", > class: options.urlClass, > href: href > + }, matches[2]); With this regexp (and doing some tests locally), I think the closing quote is going to be part of group 2 and therefore part of the <a> link. url("test.png"); will result in <text>url("</text><a>test.png"</a><text>)</text> The closing quote should be part of group 3 instead.

That test case sure was bogus. Here is a better one. This patch also fixes the problem you noticed in the regexp; plus some I found. I considered changing CSSLexer to emit the sort of information we need about URL tokens. However, we'd still want to break the token apart like this, so in the end I decided a regexp was ok, essentially because we know that the token is already valid.

Comment on attachment 8652952 [details] [diff] [review] preserve spelling of urls in output-parser.js Review of attachment 8652952 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/output-parser.js @@ +414,5 @@ > */ > _appendURL: function(match, url, options={}) { > if (options.urlClass) { > + let [, leader, , body, trailer] = > + /^(url\([ \t\r\n\f]*(["']?))(.*?)(\2[ \t\r\n\f]*\))$/i.exec(match); Can you add a comment before this crazy-looking regex explaining what it's about and summarizing what you said about using a regex in your last comment?

I belatedly realized that this doesn't handle bad_url tokens. That makes everything much hairier :( I'm considering three choices. One is to just wrap the entire url() in the <a>. This is simple and won't affect functionality. Another is to punt on bad_url. The final complicated one is to re-lex the bad_url token text, then use the facility in bug 1196896 to get the implied EOF characters to turn it into a good url.

Using a new lexer was easy to write and gets the correct results. But one might reasonably believe it is too complicated compared to some of the simpler approaches suggested earlier. It's worth noting that the CSS lexer doesn't in fact yield bad_url tokens for the new test cases. So, in one of the alternate plans, the approach would have to be to look for regexp failure rather than bad_url.

Comment on attachment 8653673 [details] [diff] [review] preserve spelling of urls in output-parser.js Review of attachment 8653673 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Just a suggestion, but R+ing this anyway. ::: toolkit/devtools/output-parser.js @@ +416,3 @@ > if (options.urlClass) { > + // Maybe the URL is incomplete. Re-lex it and add any needed > + // termination characters so that our regexp will work. This approach looks good to me, not too complex. I'd be nice if we could move this to a helper function (like sanitizeURL or something). Of course this means that this function should also return eofChars. @@ +433,5 @@ > + let [, leader, , body, trailer] = > + /^(url\([ \t\r\n\f]*(["']?))(.*?)(\2[ \t\r\n\f]*\))$/i.exec(match); > + // If the URL needed some completing characters, remove them > + // here so that the result bears a close relationship to the > + // authored text. I wonder if it'd be ok to keep the URL complete here? We wouldn't strictly show style as authored, but it would remove some code complexity and this use case is probably rare.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #7) > I wonder if it'd be ok to keep the URL complete here? We wouldn't strictly > show style as authored, but it would remove some code complexity and this > use case is probably rare. I'll make the change. I don't think it can hurt anything.

Updated per review comments and changes in bug 1196896.

Comment on attachment 8655638 [details] [diff] [review] preserve spelling of urls in output-parser.js Carrying over r+

Updated for requested changes in bug 1196896.

Hi, i think this may need a rebase for fx-team because this failed to apply: adding 1197967 to series file renamed 1197967 -> Bug-1197967---preserve-spelling-of-urls-in-output-.patch applying Bug-1197967---preserve-spelling-of-urls-in-output-.patch unable to find 'browser/devtools/shared/test/browser_outputparser.js' for patching 3 out of 3 hunks FAILED -- saving rejects to file browser/devtools/shared/test/browser_outputparser.js.rej unable to find 'toolkit/devtools/output-parser.js' for patching 1 out of 1 hunks FAILED -- saving rejects to file toolkit/devtools/output-parser.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh Bug-1197967---preserve-spelling-of-urls-in-output-.patch

Rebased.

Comment on attachment 8666694 [details] [diff] [review] preserve spelling of urls in output-parser.js Oh, sorry about that. I tested this rebased version but forgot to attach it to the bug.