Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23478003:
Check and canonicalize CSS selectors before registering PageStateMatchers. (Closed)
|
Created:
7 years, 3 months ago by Jeffrey Yasskin Modified:
7 years, 3 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCheck and canonicalize CSS selectors before registering PageStateMatchers.
Checking the selectors here gives error messages when a
developer has a typo instead of just failing to match anything.
Canonicalizing the selectors improves compatibility with the upcoming
Blink watchSelectors API, which will send back generated selector
strings instead of exactly what the user set.
I also added support in our testing infrastructure to assert that a thrown exception's message matches a regular expression.
BUG=172011
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220487
Patch Set 1 #
Total comments: 17
Patch Set 2 : Use WebKit::canonicalizeSelector() instead of CSSStyleRule.selectorText #
Total comments: 22
Patch Set 3 : Fix problems kalman noticed #
Total comments: 3
Messages
Total messages: 15 (0 generated)
The Blink change this helps with is https://codereview.chromium.org/18371008/. +Adam and Elliott to see if they prefer a new Blink API for this, and if so, how they want to spell it. https://codereview.chromium.org/23478003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc (right): https://codereview.chromium.org/23478003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:123: " var psm = new PageStateMatcher(\n" I ought to be able to run these tests entirely on the renderer side, but I don't see a simple way to import the chrome.declarativeContent definitions. https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/test_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/test_custom_bindings.js:238: if (e !== failureException && message !== undefined) This is a drive-by fix to improve test failure messages. It was counting a failed assertThrows() with a message as two failures.
"Adam and Elliott to see if they prefer a new Blink API for this" as you want to submit it now and see later, or you want them to think about this CL and get back to you? https://codereview.chromium.org/23478003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc (right): https://codereview.chromium.org/23478003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:123: " var psm = new PageStateMatcher(\n" On 2013/08/26 22:35:19, Jeffrey Yasskin wrote: > I ought to be able to run these tests entirely on the renderer side, but I don't > see a simple way to import the chrome.declarativeContent definitions. Given we have no way to spin up a fake webkit instance with content (at least yet), which the functionality heavily depends on (.. it's CSS), it would be a lot of work for this. Incidentally there's still something to be said for writing actual extensions here. It's quite hard to read JS strings embedded in C++ like this. Fine for a few lines of code, but this is getting meaty. https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/declarative_content_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:41: document.body.appendChild(div); Modifying the page's DOM / exposing our internals to the page makes me nervous, though a lot less nervous than if this were actually a web page (as opposed to an extension background page). I could rattle off a whole bunch of worrying things with the former. In any case it really saddens me that this is the only way to canonicalize CSS; it's our platform; indeed, building this as an API call into blink sounds better. What are the issues with that? https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:64: if ('css' in this) { small point, but hasOwnProperty is probably better here since we don't look up the prototype chain for function calls. https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/test_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/test_custom_bindings.js:238: if (e !== failureException && message !== undefined) On 2013/08/26 22:35:19, Jeffrey Yasskin wrote: > This is a drive-by fix to improve test failure messages. It was counting a > failed assertThrows() with a message as two failures. All this !== comparing to failureException is rather pointless because failureException is a non-empty string.
https://codereview.chromium.org/23478003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc (right): https://codereview.chromium.org/23478003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:123: " var psm = new PageStateMatcher(\n" On 2013/08/27 01:46:14, kalman wrote: > Incidentally there's still something to be said for writing actual extensions > here. It's quite hard to read JS strings embedded in C++ like this. Fine for a > few lines of code, but this is getting meaty. There's something to be said, but it tends to take me a lot longer to understand tests that are split between C++ code and a separate extension directory, and this avoids that. Reading the JS code doesn't seem that much harder: only quotes get damaged, and there aren't many of those in this test. https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/declarative_content_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:41: document.body.appendChild(div); On 2013/08/27 01:46:14, kalman wrote: > Modifying the page's DOM / exposing our internals to the page makes me nervous, > though a lot less nervous than if this were actually a web page (as opposed to > an extension background page). I could rattle off a whole bunch of worrying > things with the former. Note that this only exposes anything to the same extension that used declarativeContent.PageStateMatcher. > In any case it really saddens me that this is the only > way to canonicalize CSS; it's our platform; indeed, building this as an API call > into blink sounds better. What are the issues with that? Adding a C++ call to Blink just requires getting approval from those owners, which they're reasonable about. Adding a true JS interface would be really hard, of course. I'll send them a CL adding the C++ function. https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:64: if ('css' in this) { On 2013/08/27 01:46:14, kalman wrote: > small point, but hasOwnProperty is probably better here since we don't look up > the prototype chain for function calls. Sure (haven't updated yet, but will). Where's the function call you're talking about?
https://codereview.chromium.org/23478003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc (right): https://codereview.chromium.org/23478003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:123: " var psm = new PageStateMatcher(\n" On 2013/08/27 19:09:48, Jeffrey Yasskin wrote: > On 2013/08/27 01:46:14, kalman wrote: > > Incidentally there's still something to be said for writing actual extensions > > here. It's quite hard to read JS strings embedded in C++ like this. Fine for a > > few lines of code, but this is getting meaty. > > There's something to be said, but it tends to take me a lot longer to understand > tests that are split between C++ code and a separate extension directory, and > this avoids that. Reading the JS code doesn't seem that much harder: only quotes > get damaged, and there aren't many of those in this test. I do wish there was a better pattern for this. Perhaps if we had links to the various file of importance that would help, would allow easier source code navigation (e.g. gf in vim). https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/declarative_content_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:41: document.body.appendChild(div); On 2013/08/27 19:09:48, Jeffrey Yasskin wrote: > On 2013/08/27 01:46:14, kalman wrote: > > Modifying the page's DOM / exposing our internals to the page makes me > nervous, > > though a lot less nervous than if this were actually a web page (as opposed to > > an extension background page). I could rattle off a whole bunch of worrying > > things with the former. > > Note that this only exposes anything to the same extension that used > declarativeContent.PageStateMatcher. Yeah I know, but extensions still to unwittingly break themselves (I've fixed enough such problems to be well aware of that). In particular libraries. I can totally imagine some CSS library which compiles CSS style rules as they're added and replaces them with something else (e.g. LESS, SASS, whatever). If that ends up running in an extension context (e.g. say a popup) it might break things. > > > In any case it really saddens me that this is the only > > way to canonicalize CSS; it's our platform; indeed, building this as an API > call > > into blink sounds better. What are the issues with that? > > Adding a C++ call to Blink just requires getting approval from those owners, > which they're reasonable about. Adding a true JS interface would be really hard, > of course. > > I'll send them a CL adding the C++ function. Cool. https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:64: if ('css' in this) { On 2013/08/27 19:09:48, Jeffrey Yasskin wrote: > On 2013/08/27 01:46:14, kalman wrote: > > small point, but hasOwnProperty is probably better here since we don't look up > > the prototype chain for function calls. > > Sure (haven't updated yet, but will). Where's the function call you're talking > about? I mean, for all extension function calls (chrome.browserAction.whatever). We don't look up the prototype.
https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/declarative_content_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:41: document.body.appendChild(div); On 2013/08/27 19:09:48, Jeffrey Yasskin wrote: > Adding a C++ call to Blink just requires getting approval from those owners, > which they're reasonable about. Adding a true JS interface would be really hard, > of course. > > I'll send them a CL adding the C++ function. Done: https://codereview.chromium.org/23659002/ https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:64: if ('css' in this) { On 2013/08/27 01:46:14, kalman wrote: > small point, but hasOwnProperty is probably better here since we don't look up > the prototype chain for function calls. Done. https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/test_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/test_custom_bindings.js:238: if (e !== failureException && message !== undefined) On 2013/08/27 01:46:14, kalman wrote: > On 2013/08/26 22:35:19, Jeffrey Yasskin wrote: > > This is a drive-by fix to improve test failure messages. It was counting a > > failed assertThrows() with a message as two failures. > > All this !== comparing to failureException is rather pointless because > failureException is a non-empty string. Do you object to the !==, or is it ok to be consistent with the extra character? I wound up extending this function much more, so that I can match error messages with RegExps instead of exactly. Is there a way to say "RegExp" in jsonschema, or do I really have to use 'any' or the much more verbose "object with test() function"? https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1071: "schema_utils", Want to pick a better name for this? https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... File chrome/renderer/extensions/schema_utils_native_handler.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/schema_utils_native_handler.cc:27: v8::Local<v8::Value> arg_string; There's an implicit HandleScope around native function calls, right?
https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/test_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/test_custom_bindings.js:238: if (e !== failureException && message !== undefined) On 2013/08/29 03:39:39, Jeffrey Yasskin wrote: > On 2013/08/27 01:46:14, kalman wrote: > > On 2013/08/26 22:35:19, Jeffrey Yasskin wrote: > > > This is a drive-by fix to improve test failure messages. It was counting a > > > failed assertThrows() with a message as two failures. > > > > All this !== comparing to failureException is rather pointless because > > failureException is a non-empty string. > > Do you object to the !==, or is it ok to be consistent with the extra character? Yeah, use != rather than !==. This file is inconsistent. > > I wound up extending this function much more, so that I can match error messages > with RegExps instead of exactly. Is there a way to say "RegExp" in jsonschema, > or do I really have to use 'any' or the much more verbose "object with test() > function"? "isInstanceOf": "RegExp" should do it. https://codereview.chromium.org/23478003/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:147: " /compound selector.*: div input$/);\n" invalid types too? https://codereview.chromium.org/23478003/diff/18001/chrome/common/extensions/... File chrome/common/extensions/api/test.json (right): https://codereview.chromium.org/23478003/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/test.json:233: {"type": "any", "name": "message", "optional": true, instead: {"choices": [ {"type": "string"}, {"isInstanceOf": "RegExp"} ], "name": "message", "optional": true } i.e. choices. https://codereview.chromium.org/23478003/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/test.json:235: "A string or RegExp that must match the exception's message"} indentation here looks weird; it looks like we don't stick to 80 characters in schema files. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1071: "schema_utils", On 2013/08/29 03:39:39, Jeffrey Yasskin wrote: > Want to pick a better name for this? Sure. This is specifically for supporting declarative content, so declarativeContentNatives. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... File chrome/renderer/extensions/schema_utils_native_handler.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/schema_utils_native_handler.cc:27: v8::Local<v8::Value> arg_string; On 2013/08/29 03:39:39, Jeffrey Yasskin wrote: > There's an implicit HandleScope around native function calls, right? Yep. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/schema_utils_native_handler.cc:33: args.GetReturnValue().Set(v8::Undefined()); I imagine that setting a return value after throwing an exception is meaningless. (also you probably want to early-return from this method here in this case) https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/schema_utils_native_handler.cc:34: } I would think that you'd rather throw a schema validation error if this isn't a string, rather than coercing things into a string. I believe that ToString() calls the ToString method of the object, which is indeed a problem. TBH you might find it easier to do all type validation within the JS code. This method would then become much simpler: CHECK(args.Length() == 1 && args[0]->IsString()); WebString input_selector = WebKit::WebScriptBindings::toWebString(args[0].As<v8::String>()); ... https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... File chrome/renderer/extensions/schema_utils_native_handler.h (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/schema_utils_native_handler.h:19: // Blink-canonicalized form. If the selector is invalid, returns an empty so many people have asked me to change " " to " " that it's stuck. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/resources... File chrome/renderer/resources/extensions/declarative_content_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/resources... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:40: function canonicalizeCssSelectors(selectors) { you may also want to validate that selectors is an array. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/resources... File chrome/renderer/resources/extensions/test_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/resources... chrome/renderer/resources/extensions/test_custom_bindings.js:240: message.constructor.name == 'RegExp') { any reason why you're not using "message instanceof RegExp" here?
Thanks for teaching me javascript. :) https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/test_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/test_custom_bindings.js:238: if (e !== failureException && message !== undefined) On 2013/08/29 15:20:25, kalman wrote: > On 2013/08/29 03:39:39, Jeffrey Yasskin wrote: > > Do you object to the !==, or is it ok to be consistent with the extra > character? > > Yeah, use != rather than !==. This file is inconsistent. Done. https://codereview.chromium.org/23478003/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:147: " /compound selector.*: div input$/);\n" On 2013/08/29 15:20:25, kalman wrote: > invalid types too? I'm not sure what you mean here. The actual message is the same, but the important part of it is that "div input" is not a compound selector. (It's a "complex selector": http://dev.w3.org/csswg/selectors4/#structure) I'm going to put that URL in the docs when I update them, so users shouldn't be too confused by the error message, as long as they read the docs. https://codereview.chromium.org/23478003/diff/18001/chrome/common/extensions/... File chrome/common/extensions/api/test.json (right): https://codereview.chromium.org/23478003/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/test.json:233: {"type": "any", "name": "message", "optional": true, On 2013/08/29 15:20:25, kalman wrote: > instead: > > {"choices": [ {"type": "string"}, {"isInstanceOf": "RegExp"} ], "name": > "message", "optional": true } > > i.e. choices. More "isInstanceOf". Thanks! I still had to specify type:object, and that meant I needed to relax a restriction in model.py. https://codereview.chromium.org/23478003/diff/18001/chrome/common/extensions/... chrome/common/extensions/api/test.json:235: "A string or RegExp that must match the exception's message"} On 2013/08/29 15:20:25, kalman wrote: > indentation here looks weird; it looks like we don't stick to 80 characters in > schema files. 'k. I've removed the description again now that the machine-readable data captures the restrictions. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:1071: "schema_utils", On 2013/08/29 15:20:25, kalman wrote: > On 2013/08/29 03:39:39, Jeffrey Yasskin wrote: > > Want to pick a better name for this? > > Sure. This is specifically for supporting declarative content, so > declarativeContentNatives. Decided on css_natives over IM. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... File chrome/renderer/extensions/schema_utils_native_handler.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/schema_utils_native_handler.cc:33: args.GetReturnValue().Set(v8::Undefined()); On 2013/08/29 15:20:25, kalman wrote: > I imagine that setting a return value after throwing an exception is > meaningless. Yeah, must be, since forgetting to return didn't break anything. > (also you probably want to early-return from this method here in this case) Oops. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/schema_utils_native_handler.cc:34: } On 2013/08/29 15:20:25, kalman wrote: > I would think that you'd rather throw a schema validation error if this isn't a > string, rather than coercing things into a string. I believe that ToString() > calls the ToString method of the object, which is indeed a problem. > > TBH you might find it easier to do all type validation within the JS code. This > method would then become much simpler: > > CHECK(args.Length() == 1 && args[0]->IsString()); > WebString input_selector = > WebKit::WebScriptBindings::toWebString(args[0].As<v8::String>()); > ... Sure, done. The schema validator seems to check all this anyway. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... File chrome/renderer/extensions/schema_utils_native_handler.h (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/extension... chrome/renderer/extensions/schema_utils_native_handler.h:19: // Blink-canonicalized form. If the selector is invalid, returns an empty On 2013/08/29 15:20:25, kalman wrote: > so many people have asked me to change " " to " " that it's stuck. Heh. Done. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/resources... File chrome/renderer/resources/extensions/declarative_content_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/resources... chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:40: function canonicalizeCssSelectors(selectors) { On 2013/08/29 15:20:25, kalman wrote: > you may also want to validate that selectors is an array. The schema validator gets that. I've added a test. https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/resources... File chrome/renderer/resources/extensions/test_custom_bindings.js (right): https://codereview.chromium.org/23478003/diff/18001/chrome/renderer/resources... chrome/renderer/resources/extensions/test_custom_bindings.js:240: message.constructor.name == 'RegExp') { On 2013/08/29 15:20:25, kalman wrote: > any reason why you're not using "message instanceof RegExp" here? Because I don't know JavaScript. Done.
lgtm https://codereview.chromium.org/23478003/diff/18001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc (right): https://codereview.chromium.org/23478003/diff/18001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:147: " /compound selector.*: div input$/);\n" On 2013/08/29 21:46:43, Jeffrey Yasskin wrote: > On 2013/08/29 15:20:25, kalman wrote: > > invalid types too? > > I'm not sure what you mean here. The actual message is the same, but the > important part of it is that "div input" is not a compound selector. (It's a > "complex selector": http://dev.w3.org/csswg/selectors4/#structure) > > I'm going to put that URL in the docs when I update them, so users shouldn't be > too confused by the error message, as long as they read the docs. Oh I meant like rather than passing in a string pass in an object. But if this goes through schema validation (I wasn't sure) then it's pointless to do so. https://codereview.chromium.org/23478003/diff/28001/chrome/renderer/extension... File chrome/renderer/extensions/css_native_handler.cc (right): https://codereview.chromium.org/23478003/diff/28001/chrome/renderer/extension... chrome/renderer/extensions/css_native_handler.cc:29: WebString output_selector = WebKit::canonicalizeSelector( I presume from reading the JS that this returns an empty string if it's invalid CSS. maybe you should comment that.
lgtm
https://codereview.chromium.org/23478003/diff/28001/chrome/renderer/extension... File chrome/renderer/extensions/css_native_handler.cc (right): https://codereview.chromium.org/23478003/diff/28001/chrome/renderer/extension... chrome/renderer/extensions/css_native_handler.cc:29: WebString output_selector = WebKit::canonicalizeSelector( On 2013/08/29 22:24:04, kalman wrote: > I presume from reading the JS that this returns an empty string if it's invalid > CSS. maybe you should comment that. It's commented in third_party/WebKit/public/web/WebSelector.h and in css_native_handler.h. Do I need it in a third place?
https://codereview.chromium.org/23478003/diff/28001/chrome/renderer/extension... File chrome/renderer/extensions/css_native_handler.cc (right): https://codereview.chromium.org/23478003/diff/28001/chrome/renderer/extension... chrome/renderer/extensions/css_native_handler.cc:29: WebString output_selector = WebKit::canonicalizeSelector( On 2013/08/29 23:18:49, Jeffrey Yasskin wrote: > On 2013/08/29 22:24:04, kalman wrote: > > I presume from reading the JS that this returns an empty string if it's > invalid > > CSS. maybe you should comment that. > > It's commented in third_party/WebKit/public/web/WebSelector.h and in > css_native_handler.h. Do I need it in a third place? I didn't see it in the header file and :O webkit has a comment?
(as in I didn't think to look in the header file, I'm sure that's fine)
On 2013/08/29 23:24:29, kalman wrote: > https://codereview.chromium.org/23478003/diff/28001/chrome/renderer/extension... > File chrome/renderer/extensions/css_native_handler.cc (right): > > https://codereview.chromium.org/23478003/diff/28001/chrome/renderer/extension... > chrome/renderer/extensions/css_native_handler.cc:29: WebString output_selector = > WebKit::canonicalizeSelector( > On 2013/08/29 23:18:49, Jeffrey Yasskin wrote: > > On 2013/08/29 22:24:04, kalman wrote: > > > I presume from reading the JS that this returns an empty string if it's > > invalid > > > CSS. maybe you should comment that. > > > > It's commented in third_party/WebKit/public/web/WebSelector.h and in > > css_native_handler.h. Do I need it in a third place? > > I didn't see it in the header file and :O webkit has a comment? Well, Blink has started accepting comments. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/23478003/28001
Message was sent while issue was closed.
Change committed as 220487 |
