CefView / CefViewCore

A common library providing clean and easy consuming of CEF
MIT License
99 stars 52 forks source link

A question about "CefViewClient::V8ValueToCefValue" #26

Closed ShelleyLake closed 1 year ago

ShelleyLake commented 1 year ago

A comment states the following:

"The IsDouble, IsInt and IsUint methods return a boolean value indicating whether the CefV8Value instance is a target type or can be converted to the target type."

My question is, from where do you deduce that those functions tells if the value can be converted, beside testing the type?

Looking in the CEF headers, I read this and (if I am not mistaken) conclude that the functions ONLY check the type. So the order in which the IsDouble, IsInt and IsUint functions are used should be irrelevant.

  ///
  /// True if the value type is int.
  ///
  /*--cef()--*/
  virtual bool IsInt() = 0;

  ///
  /// True if the value type is unsigned int.
  ///
  /*--cef()--*/
  virtual bool IsUInt() = 0;

Thanks

tishion commented 1 year ago

I remember this comment was added by fixing the issue (#10).

And after debugging, I found there is mistake in this comment.

Just try the code below:

   auto v = CefV8Value::CreateInt(1000);
   auto isDouble = v->IsDouble();
   logD("isDouble: %d", isDouble);  // true
   auto isUnint = v->IsUInt();
   logD("isUnint: %d", isUnint);    // true
   auto isInt = v->IsInt();
   logD("isInt: %d", isInt);        // true

And also, there is a bug in the executeJavascriptWithResult method, I am fixing it now.

ShelleyLake commented 1 year ago

Thanks.

ShelleyLake commented 1 year ago

Argh! My bad. If I understand correctly in Javascript there is only one value (V8Value) of type "Number" stored as float. https://www.w3schools.com/js/js_numbers.asp So that's probably why "1000" will always give true in all three conditions: technically it's a "double", but it can be converted to "uint" and "int". Excuse me, but if that is the case I see a problem with the order of your conditions: if isDouble()is always "true", this prevents the other conditions isUint(), is Int() from being reached. So wouldn't it make sense to check the value in the order: isUint(), isInt(), isDouble()?

tishion commented 1 year ago

uhm..... I think you are correct...will draft a fix

tishion commented 1 year ago

can review this commit: https://github.com/CefView/CefViewCore/commit/aa4b74228ad4ce9b6e1459620e9d00c85251366d