Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
477193003:
Refactor setIcon to allow for more general use of imageData (Closed)
|
Created:
6 years, 4 months ago by gpdavis Modified:
6 years, 4 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRefactor setIcon to allow for more general use of imageData.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291119
Patch Set 1 #
Total comments: 11
Patch Set 2 : Added page action binding, cleaned up setIcon logic #
Total comments: 20
Patch Set 3 : Minor changes, reworked conversion logic #
Total comments: 10
Patch Set 4 : Moar minor changes #
Messages
Total messages: 33 (0 generated)
Should I create a bug for this? Passed BrowserActionApi browser tests (y)
Sorry I didn't get time to finish this review, but that's a couple of things to do there. https://codereview.chromium.org/477193003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/extension_action/extension_action_api.cc (right): https://codereview.chromium.org/477193003/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/extension_action/extension_action_api.cc:684: IPC::Message pickle(binary_data.c_str(), binary_data.length()); You should only need to do this Binary -> std::string conversion when you're reading/writing from the database. The API can still use Binary, I hope? https://codereview.chromium.org/477193003/diff/40001/chrome/renderer/resource... File chrome/renderer/resources/extensions/browser_action_custom_bindings.js (right): https://codereview.chromium.org/477193003/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/browser_action_custom_bindings.js:13: binding.registerCustomHook(function(bindingsAPI) { What about page_action_custom_bindings.js? https://codereview.chromium.org/477193003/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/40001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:52: function setIcon(details, actionType, callback) { It looks like the only reason we need to thread through |actionType| is to have a nicer error message. Let's just forget about that nicer error message (it's pretty clear from the context - you only have one or the other) - and delete it. Simpler everywhere. https://codereview.chromium.org/477193003/diff/40001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:55: callback(details); This code is such a nightmare. Sheesh. Such a nest of braces :( Let's fix it, starting here: if ('iconIndex' in details) { callback(details); return; } if ('imageData' in details) { var isEmpty = true; for (var i = 0; i < iconSizes.length; i++) { var sizeKey = iconSizes[i].toString(); if (sizeKey in details.imageData) { verifyImageData(..); isEmpty = false; } } if (isEmpty) { // If details.imageData isnot a dictionary with keys... var...; verifyImageData(..); } callback(SetIconCommon(details)); return; } ... etc. https://codereview.chromium.org/477193003/diff/40001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:57: if (typeof details.imageData == 'object') { Checks like these are so unnecessary, it's exactly what the schema validation will have sorted out.
https://codereview.chromium.org/477193003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/extension_action/extension_action_api.cc (right): https://codereview.chromium.org/477193003/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/extension_action/extension_action_api.cc:684: IPC::Message pickle(binary_data.c_str(), binary_data.length()); On 2014/08/19 00:04:43, kalman wrote: > You should only need to do this Binary -> std::string conversion when you're > reading/writing from the database. The API can still use Binary, I hope? You don't think we should keep it this way so it will be serializable once it's expanded for use in declarativeContent, and perhaps other places? It should work here without conversion, if you'd prefer it that way. https://codereview.chromium.org/477193003/diff/40001/chrome/renderer/resource... File chrome/renderer/resources/extensions/browser_action_custom_bindings.js (right): https://codereview.chromium.org/477193003/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/browser_action_custom_bindings.js:13: binding.registerCustomHook(function(bindingsAPI) { On 2014/08/19 00:04:43, kalman wrote: > What about page_action_custom_bindings.js? Good point. Done. https://codereview.chromium.org/477193003/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/40001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:52: function setIcon(details, actionType, callback) { On 2014/08/19 00:04:43, kalman wrote: > It looks like the only reason we need to thread through |actionType| is to have > a nicer error message. Let's just forget about that nicer error message (it's > pretty clear from the context - you only have one or the other) - and delete it. > Simpler everywhere. Done. https://codereview.chromium.org/477193003/diff/40001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:55: callback(details); On 2014/08/19 00:04:43, kalman wrote: > This code is such a nightmare. Sheesh. Such a nest of braces :( > > Let's fix it, starting here: > > if ('iconIndex' in details) { > callback(details); > return; > } > > if ('imageData' in details) { > var isEmpty = true; > for (var i = 0; i < iconSizes.length; i++) { > var sizeKey = iconSizes[i].toString(); > if (sizeKey in details.imageData) { > verifyImageData(..); > isEmpty = false; > } > } > > if (isEmpty) { > // If details.imageData isnot a dictionary with keys... > var...; > verifyImageData(..); > } > > callback(SetIconCommon(details)); > return; > } > > ... etc. Done. https://codereview.chromium.org/477193003/diff/40001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:57: if (typeof details.imageData == 'object') { On 2014/08/19 00:04:43, kalman wrote: > Checks like these are so unnecessary, it's exactly what the schema validation > will have sorted out. Done.
Closer :) https://codereview.chromium.org/477193003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/extension_action/extension_action_api.cc (right): https://codereview.chromium.org/477193003/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/extension_action/extension_action_api.cc:684: IPC::Message pickle(binary_data.c_str(), binary_data.length()); On 2014/08/19 00:54:15, gpdavis wrote: > On 2014/08/19 00:04:43, kalman wrote: > > You should only need to do this Binary -> std::string conversion when you're > > reading/writing from the database. The API can still use Binary, I hope? > > You don't think we should keep it this way so it will be serializable once it's > expanded for use in declarativeContent, and perhaps other places? It should > work here without conversion, if you'd prefer it that way. There are 2 things here: (1) The serialization format over IPC. (2) The serialization format when reading/writing to disk. They don't need to be the same. Of course, as you know the limitation of (2) is that it must be a string, so we're forced to base64 it. (1) doesn't have the same limitation. It's more efficient to not do conversion and just pickle/unpickle directly into the base::Binary format, *after which* we convert to base64. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:12: path + '\'.'); This should fit on 1 line now. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:51: Add comment: /** * Normalizes |details| to a format suitable for sending to the browser, * for example converting ImageData to a binary representation. * * @param {ImageDetails} details * The ImageDetails passed into an extension action-style API. */ https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:80: } nit: blank line here. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:105: nit: no blank line here :) https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:117: } return after this. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:118: } You still need to throw an Error out here. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... File extensions/renderer/set_icon_natives.cc (right): https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:100: base::Base64Encode(bitmap_data, &data64); Ideally you'd only need to convert to a binary value that's understood by the V8ValueConverter already, I.e., rather than going: ImageData -> base::BinaryValue -> V8ValueConverter::ToV8Value -> ArrayBuffer -> JavaScript -> V8ValueConverter::FromV8Value -> base::BinaryValue just: ImageData -> ArrayBuffer -> JavaScript -> V8ValueConverter::FromV8Value -> base::BinaryValue i.e. cut out one of the base::BinaryValue conversion steps. Check out how V8ValueConverterImpl does it: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/v... https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:132: const v8::FunctionCallbackInfo<v8::Value>& args) { CHECK_EQ(1, args.Length()); CHECK(args[0]->IsObject()); https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:140: base::DictionaryValue* dict = new base::DictionaryValue(); This now leaks |dict| since it's not having its ownership transferred to |list_value|. Just declare |dict| on the stack now.
https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:12: path + '\'.'); On 2014/08/19 17:32:29, kalman wrote: > This should fit on 1 line now. Sweet deal! https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:51: On 2014/08/19 17:32:29, kalman wrote: > Add comment: > > /** > * Normalizes |details| to a format suitable for sending to the browser, > * for example converting ImageData to a binary representation. > * > * @param {ImageDetails} details > * The ImageDetails passed into an extension action-style API. > */ Done. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:80: } On 2014/08/19 17:32:29, kalman wrote: > nit: blank line here. Done. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:105: On 2014/08/19 17:32:29, kalman wrote: > nit: no blank line here :) Done. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:117: } On 2014/08/19 17:32:29, kalman wrote: > return after this. Done. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:118: } On 2014/08/19 17:32:29, kalman wrote: > You still need to throw an Error out here. Ah, right, because we can't validate that one of them exists with the schema. We can just validate that if they do exist, they are the correct type. Right? https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... File extensions/renderer/set_icon_natives.cc (right): https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:100: base::Base64Encode(bitmap_data, &data64); On 2014/08/19 17:32:29, kalman wrote: > Ideally you'd only need to convert to a binary value that's understood by the > V8ValueConverter already, I.e., rather than going: > > ImageData -> base::BinaryValue -> V8ValueConverter::ToV8Value -> ArrayBuffer -> > JavaScript -> V8ValueConverter::FromV8Value -> base::BinaryValue > > just: > > ImageData -> ArrayBuffer -> JavaScript -> V8ValueConverter::FromV8Value -> > base::BinaryValue > > i.e. cut out one of the base::BinaryValue conversion steps. Check out how > V8ValueConverterImpl does it: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/v... Well, that took a while, but I finally figured out how to work with these V8 values. Done. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:132: const v8::FunctionCallbackInfo<v8::Value>& args) { On 2014/08/19 17:32:29, kalman wrote: > CHECK_EQ(1, args.Length()); > CHECK(args[0]->IsObject()); Done. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:140: base::DictionaryValue* dict = new base::DictionaryValue(); On 2014/08/19 17:32:29, kalman wrote: > This now leaks |dict| since it's not having its ownership transferred to > |list_value|. > > Just declare |dict| on the stack now. Ah, gotcha. Nice catch!
lgtm with just a few more fairly straightforward changes. https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:118: } On 2014/08/19 22:05:07, gpdavis wrote: > On 2014/08/19 17:32:29, kalman wrote: > > You still need to throw an Error out here. > > Ah, right, because we can't validate that one of them exists with the schema. > We can just validate that if they do exist, they are the correct type. Right? Yeah exactly, there is no way to say in the schema "exactly one of these must exist". https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... File extensions/renderer/set_icon_natives.cc (right): https://codereview.chromium.org/477193003/diff/60001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:100: base::Base64Encode(bitmap_data, &data64); On 2014/08/19 22:05:07, gpdavis wrote: > On 2014/08/19 17:32:29, kalman wrote: > > Ideally you'd only need to convert to a binary value that's understood by the > > V8ValueConverter already, I.e., rather than going: > > > > ImageData -> base::BinaryValue -> V8ValueConverter::ToV8Value -> ArrayBuffer > -> > > JavaScript -> V8ValueConverter::FromV8Value -> base::BinaryValue > > > > just: > > > > ImageData -> ArrayBuffer -> JavaScript -> V8ValueConverter::FromV8Value -> > > base::BinaryValue > > > > i.e. cut out one of the base::BinaryValue conversion steps. Check out how > > V8ValueConverterImpl does it: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/v... > > Well, that took a while, but I finally figured out how to work with these V8 > values. Done. Nice! https://codereview.chromium.org/477193003/diff/80001/chrome/renderer/resource... File chrome/renderer/resources/extensions/browser_action_custom_bindings.js (right): https://codereview.chromium.org/477193003/diff/80001/chrome/renderer/resource... chrome/renderer/resources/extensions/browser_action_custom_bindings.js:3: // found in the LICENSE file. You *also* need to update system_indicator_custom_bindings.js. But that's all that's left (at least, when I grep for setIcon). https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:56: * The ImageDetails passed into an extension action-style API. There is another parameter here, |callback|. Document that. Also mention that the callback maybe called reentrantly. This is actually a shame, and if this were new code I'd say that we should ensure it's not re-entrant, BUT it's not worth it given it's old code. It works just fine the way it is. Just document it. https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:60: if ('iconIndex' in details) { Could you add a comment here: // Note that iconIndex is actually deprecated, and only available to the pageAction API. // TODO(kalman): Investigate whether this is for the pageActions API, and // if so, delete it. https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/set_... File extensions/renderer/set_icon_natives.cc (right): https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:137: if (!ConvertImageDataSetToBitmapValueSet(args, &bitmap_set_value)) All ConvertImageDataSetToBitmapValueSet does to |args| is 1- extract args[0] 2- get the isolate 1 is repeating work, because we grab args[0] here right after (line 140). So you should get the |details| first, then pass just that to ConvertImageDataSetToBitmapValueSet (of course you'll need to change the Arguments value to a const v8::Handle<v8::Object>&). It's better to do that anyway, it's a bit odd to validate the arguments to the function then ignore it. You should assign |details| straight after the CHECK. 2, well, we have the isolate here, it's context()->isolate() (yes they're the same isolate), so you can just call that directly from ConvertImageDataSetToBitmapValueSet. No need to pass it in.
https://codereview.chromium.org/477193003/diff/80001/chrome/renderer/resource... File chrome/renderer/resources/extensions/browser_action_custom_bindings.js (right): https://codereview.chromium.org/477193003/diff/80001/chrome/renderer/resource... chrome/renderer/resources/extensions/browser_action_custom_bindings.js:3: // found in the LICENSE file. On 2014/08/20 14:42:18, kalman wrote: > You *also* need to update system_indicator_custom_bindings.js. But that's all > that's left (at least, when I grep for setIcon). Done. https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:56: * The ImageDetails passed into an extension action-style API. On 2014/08/20 14:42:18, kalman wrote: > There is another parameter here, |callback|. Document that. > > Also mention that the callback maybe called reentrantly. This is actually a > shame, and if this were new code I'd say that we should ensure it's not > re-entrant, BUT it's not worth it given it's old code. It works just fine the > way it is. > > Just document it. Could you clarify what you mean by reentrantly, exactly? Is this because the callback has calls throughout the function, even though they're all followed by returns (which makes it okay)? Just want to make sure I understand this instead of just saying okay and doing it :) https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:60: if ('iconIndex' in details) { On 2014/08/20 14:42:18, kalman wrote: > Could you add a comment here: > > // Note that iconIndex is actually deprecated, and only available to the > pageAction API. > // TODO(kalman): Investigate whether this is for the pageActions API, and > // if so, delete it. Done. https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/set_... File extensions/renderer/set_icon_natives.cc (right): https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/set_... extensions/renderer/set_icon_natives.cc:137: if (!ConvertImageDataSetToBitmapValueSet(args, &bitmap_set_value)) On 2014/08/20 14:42:19, kalman wrote: > All ConvertImageDataSetToBitmapValueSet does to |args| is > 1- extract args[0] > 2- get the isolate > > 1 is repeating work, because we grab args[0] here right after (line 140). So you > should get the |details| first, then pass just that to > ConvertImageDataSetToBitmapValueSet (of course you'll need to change the > Arguments value to a const v8::Handle<v8::Object>&). > > It's better to do that anyway, it's a bit odd to validate the arguments to the > function then ignore it. You should assign |details| straight after the CHECK. > > 2, well, we have the isolate here, it's context()->isolate() (yes they're the > same isolate), so you can just call that directly from > ConvertImageDataSetToBitmapValueSet. No need to pass it in. Ah, I wasn't sure if these were the same isolate and I figured there was a reason the args were passed in (now that I look at it again, though, args was probably passed because more information was extracted in ConvertImageDataSetToBitmapValueSet before). Done.
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/477193003/1...
(still lgtm) https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:56: * The ImageDetails passed into an extension action-style API. On 2014/08/20 17:58:18, gpdavis wrote: > On 2014/08/20 14:42:18, kalman wrote: > > There is another parameter here, |callback|. Document that. > > > > Also mention that the callback maybe called reentrantly. This is actually a > > shame, and if this were new code I'd say that we should ensure it's not > > re-entrant, BUT it's not worth it given it's old code. It works just fine the > > way it is. > > > > Just document it. > > Could you clarify what you mean by reentrantly, exactly? Is this because the > callback has calls throughout the function, even though they're all followed by > returns (which makes it okay)? Just want to make sure I understand this instead > of just saying okay and doing it :) Reentrant is the difference between: foo(); [1,2,3].forEach(bar); baz(); and foo(); setTimeout(bar); baz(); In the first example foo() will be called, then bar() (3 times), then baz. This is reentrant. In the second example foo() will be called, then baz(), then at some point in the future bar(). This is not reentrant. It's *never* possible to call bar() before baz(). Mixing an API between reentrant callbacks and non-reentrant callbacks is confusing and can lead to subtle bugs, e.g. what if instead of the above we had: foo(); doSomething(bar); baz(); and doSomething can be either reentrant or not, it depends on the input or state of the world or whatever. That has undefined behaviour, since baz *may* be called after bar, or *may* not be. Hard to program for. This isn't just speculation, I've seen some really tricky bugs caused by it. You can fix that without too much effort, but like I said, not worth it. But worth commenting on.
https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:56: * The ImageDetails passed into an extension action-style API. On 2014/08/20 18:20:29, kalman wrote: > On 2014/08/20 17:58:18, gpdavis wrote: > > On 2014/08/20 14:42:18, kalman wrote: > > > There is another parameter here, |callback|. Document that. > > > > > > Also mention that the callback maybe called reentrantly. This is actually a > > > shame, and if this were new code I'd say that we should ensure it's not > > > re-entrant, BUT it's not worth it given it's old code. It works just fine > the > > > way it is. > > > > > > Just document it. > > > > Could you clarify what you mean by reentrantly, exactly? Is this because the > > callback has calls throughout the function, even though they're all followed > by > > returns (which makes it okay)? Just want to make sure I understand this > instead > > of just saying okay and doing it :) > > Reentrant is the difference between: > > foo(); > [1,2,3].forEach(bar); > baz(); > > and > > foo(); > setTimeout(bar); > baz(); > > In the first example foo() will be called, then bar() (3 times), then baz. This > is reentrant. > In the second example foo() will be called, then baz(), then at some point in > the future bar(). This is not reentrant. It's *never* possible to call bar() > before baz(). > > Mixing an API between reentrant callbacks and non-reentrant callbacks is > confusing and can lead to subtle bugs, e.g. what if instead of the above we had: > > foo(); > doSomething(bar); > baz(); > > and doSomething can be either reentrant or not, it depends on the input or state > of the world or whatever. That has undefined behaviour, since baz *may* be > called after bar, or *may* not be. Hard to program for. This isn't just > speculation, I've seen some really tricky bugs caused by it. > > You can fix that without too much effort, but like I said, not worth it. But > worth commenting on. Ahh, okay, I think I understand now. So we're comfortable with the way things are mostly because, like I said before, we always return immediately after the callback, so the unpredictability of reentrancy is unlikely to be problematic?
On 2014/08/20 18:27:18, gpdavis wrote: > https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... > File extensions/renderer/resources/set_icon.js (right): > > https://codereview.chromium.org/477193003/diff/80001/extensions/renderer/reso... > extensions/renderer/resources/set_icon.js:56: * The ImageDetails passed into > an extension action-style API. > On 2014/08/20 18:20:29, kalman wrote: > > On 2014/08/20 17:58:18, gpdavis wrote: > > > On 2014/08/20 14:42:18, kalman wrote: > > > > There is another parameter here, |callback|. Document that. > > > > > > > > Also mention that the callback maybe called reentrantly. This is actually > a > > > > shame, and if this were new code I'd say that we should ensure it's not > > > > re-entrant, BUT it's not worth it given it's old code. It works just fine > > the > > > > way it is. > > > > > > > > Just document it. > > > > > > Could you clarify what you mean by reentrantly, exactly? Is this because > the > > > callback has calls throughout the function, even though they're all followed > > by > > > returns (which makes it okay)? Just want to make sure I understand this > > instead > > > of just saying okay and doing it :) > > > > Reentrant is the difference between: > > > > foo(); > > [1,2,3].forEach(bar); > > baz(); > > > > and > > > > foo(); > > setTimeout(bar); > > baz(); > > > > In the first example foo() will be called, then bar() (3 times), then baz. > This > > is reentrant. > > In the second example foo() will be called, then baz(), then at some point in > > the future bar(). This is not reentrant. It's *never* possible to call bar() > > before baz(). > > > > Mixing an API between reentrant callbacks and non-reentrant callbacks is > > confusing and can lead to subtle bugs, e.g. what if instead of the above we > had: > > > > foo(); > > doSomething(bar); > > baz(); > > > > and doSomething can be either reentrant or not, it depends on the input or > state > > of the world or whatever. That has undefined behaviour, since baz *may* be > > called after bar, or *may* not be. Hard to program for. This isn't just > > speculation, I've seen some really tricky bugs caused by it. > > > > You can fix that without too much effort, but like I said, not worth it. But > > worth commenting on. > > Ahh, okay, I think I understand now. So we're comfortable with the way things > are mostly because, like I said before, we always return immediately after the > callback, so the unpredictability of reentrancy is unlikely to be problematic? (lgtm for CQ) It's all about how the caller of the function uses it. All the callers are currently safe, so yes, it's fine.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/477193003/1...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/477193003/1...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by kalman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/477193003/1...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/477193003/1...
Message was sent while issue was closed.
Committed patchset #4 (100001) as 291119 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
