@jdoerrie:
In a5676c60 you introduced changes like the following:
void ListValue::Append(std::unique_ptr<Value> in_value) {
- list_->push_back(std::move(in_value));
+ list_->push_back(std::move(*in_value));
}
From what I understand this causes |in_value| to be copied instead of the original value being inserted into |list_| (moved from |in_value|). Is this behavior change intentional?
@marshall:
This change is necessary because a5676c60 changed list_ from std::vector<std::unique_ptr<base::Value>> to std::vector<base::Value>.
There should be no copies involved, just moves. std::vector::push_back is overloaded for rvalue references that then move constructs the new object at the end of the vector. To my understanding you would be right if that line read |list_->push_back(*in_value);|.
(corrected version of comment #30)
@jdoerrie:
To provide a bit more background, I have an API that wraps Value objects in ref-counted objects. In order to avoid dangling pointers (e.g. to keep a ListValue object alive while its contents are referenced) I keep a map of the base::Value* pointers owned by the ListValue. With your change in comment #29 I can no longer associate the base::Value* passed into ListValue::Append with the |out_value| returned from ListValue::Remove.
OK, thanks for the clarification in comment #31. I'll take a closer look to see what's going in.
@comment#31: It looks like using a move constructor causes the contents of the base::Value to be moved (without copy) but the source base::Value* (owned by the unique_ptr) is discarded. So, it's fine from a performance standpoint but my pointer mapping technique no longer works :(
@marshall:
The purpose of this refactor is to move to value semantics, removing the need to have pointers to derived classes of base::Value to represent different types of Values. When this refactor is finished, you can simply have a single base::Value instance that depending on its type allows access to the underlying data. As of r465517 all ListValue methods are effectively deprecated and you should use Value::GetList if you can. For example, instead of ListValue::Append and ListValue::Remove you should be using Value::GetList::push_back and Value::GetList::erase.
Furthermore, it is now practically impossible to accidentally delete an object owned by a ListValue. Given that the underlying vector contains actual Values instead of pointers to Values, refcounting should not be necessary anymore.
I hope this provides a bit more clarity, feel free to provide more information or point to actual code in case I was unclear or did not understand your situation correctly.
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)
For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Can you please confirm if this is a safe merge? Has this been tested in Canary? Is there enough unit test coverage for this? This seems like a fairly large refactor, and are we certain this won't break anything else? Is this also critical for M59, or can it wait until M60?
Just to make sure we talk about the same revision to merge: I proposed r465517.
This is not a large refactor, it is a couple of lines, adding new API in base::Value, which is not used anywhere other than in the added tests (yes, it has appropriate test coverage).
The patch has been in Canary since 60.0.3075.0 and is safe to merge.
And to answer whether it is needed in M59 -- it is needed, because it blocks a fix to a crash, tracked in bug 712119. We don't want that crash to gradually reach stable. (Requesting merge for that fix will be done separately, though.)
Hello,
it seems this is suspending like a month. What is the current status of this and what's the plan/ETA?
I hit an issue that we cannot create base::Optional<base::DictionaryValue>, becuase we don't have Value::GetDict() unlike Value::GetList() etc. yet.
I wonder if such an API will be added sometime soon.
The refactoring is still going on. I have some large pending CLs that will remove many usages of the deprecated API, but I need to double check their impact on the binary size. However, all of the newly designed API methods are available in the base class and ready for use.
GetDict() is definitely on our radar, but first we want to change the underlying dictionary to not contain unique ptrs anymore. Doing this requires removing almost all usages of the deprecated API though, because it will break pointer stability.
Regarding your issue: Wouldn't it be possible to use a base::Optional<base::Value> instead? What requires you to use a base::DictionaryValue?
