Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2658013002:
Remove ScopedVector in components/spellcheck (Closed)
|
Created:
3 years, 11 months ago by ke.he Modified:
3 years, 10 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spell_chromium.org, timvolodine Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove ScopedVector in components/spellcheck
BUG=554289
Review-Url: https://codereview.chromium.org/2658013002
Cr-Commit-Position: refs/heads/master@{#447961}
Committed: https://chromium.googlesource.com/chromium/src/+/293806358ee234776a9f73f05fdb08a3e3e7e78a
Patch Set 1 #Patch Set 2 : Remove ScopedVector in components/spellcheck #Patch Set 3 : Remove ScopedVector in components/spellcheck #
Total comments: 9
Patch Set 4 : Remove ScopedVector in components/spellcheck #Patch Set 5 : code rebase #
Messages
Total messages: 37 (23 generated)
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ke.he@intel.com changed reviewers: + groby@chromium.org
Hi, groby@, Could you PTAL on this? Thanks very much.
Thanks for cleaning up ScopedVector! A few minor comments, but otherwise lg. https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/b... File components/spellcheck/browser/feedback_sender.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/b... components/spellcheck/browser/feedback_sender.cc:452: senders_.push_back(base::WrapUnique<net::URLFetcher>(sender)); This should probably be a unique_ptr from creation, and std::move at this point. https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... components/spellcheck/renderer/spellcheck.cc:440: for (auto& language : languages_) Why make this a reference? https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_provider_test.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_provider_test.cc:66: messages_.push_back(base::WrapUnique<IPC::Message>(message)); This makes me queasy, because the use of unique_ptr implies strict ownership management. And in that world, the API implies that message is owned outside this function. I know you can't fix it in this CL, but are you aware of any efforts to fix the IPC API?
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
groby@,Thank you! CL updated, PTAL. https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/b... File components/spellcheck/browser/feedback_sender.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/b... components/spellcheck/browser/feedback_sender.cc:452: senders_.push_back(base::WrapUnique<net::URLFetcher>(sender)); On 2017/01/26 17:40:47, groby wrote: > This should probably be a unique_ptr from creation, and std::move at this point. Done. https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... components/spellcheck/renderer/spellcheck.cc:440: for (auto& language : languages_) On 2017/01/26 17:40:47, groby wrote: > Why make this a reference? if write as "for (auto language : languages)", compiler complains: "error: call to deleted constructor of 'std::unique_ptr<SpellcheckLanguage>" https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_provider_test.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_provider_test.cc:66: messages_.push_back(base::WrapUnique<IPC::Message>(message)); On 2017/01/26 17:40:47, groby wrote: > This makes me queasy, because the use of unique_ptr implies strict ownership > management. And in that world, the API implies that message is owned outside > this function. > > I know you can't fix it in this CL, but are you aware of any efforts to fix the > IPC API? In this testing class, the Send() function doesn't send message to browser side, it only "emulate" as if it receives the response from browser side. If the message is handled(by itself), it deletes the message, else, it pushes the message into scopedVector for later destruct. So, Although the TestingSpellCheckProvider::Send() overrides from SpellCheckProvider, It seems we don't need to change the IPC API. what do you think? Thanks:)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friendly ping:)
friendly ping:)
Sigh. Wrote the comments, didn't send the reply, sorry :( LGTM https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... components/spellcheck/renderer/spellcheck.cc:440: for (auto& language : languages_) On 2017/01/27 03:15:40, ke.he wrote: > On 2017/01/26 17:40:47, groby wrote: > > Why make this a reference? > if write as "for (auto language : languages)", compiler complains: "error: call > to deleted constructor of 'std::unique_ptr<SpellcheckLanguage>" Ah, my apologies. My mind kept thinking of |language| as a SpellcheckLanguage*, not a unique_ptr :) https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_provider_test.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_provider_test.cc:66: messages_.push_back(base::WrapUnique<IPC::Message>(message)); On 2017/01/27 03:15:40, ke.he wrote: > On 2017/01/26 17:40:47, groby wrote: > > This makes me queasy, because the use of unique_ptr implies strict ownership > > management. And in that world, the API implies that message is owned outside > > this function. > > > > I know you can't fix it in this CL, but are you aware of any efforts to fix > the > > IPC API? > > In this testing class, the Send() function doesn't send message to browser side, > it only "emulate" as if it receives the response from browser side. If the > message is handled(by itself), it deletes the message, else, it pushes the > message into scopedVector for later destruct. So, Although the > TestingSpellCheckProvider::Send() overrides from SpellCheckProvider, It seems we > don't need to change the IPC API. what do you think? Thanks:) AIUI, the Send() function always takes ownership - see comment in ipc_sender.h: "The implementor takes ownership of the given Message regardless of whether or not this method succeeds" So it _should_ take a unique_ptr. In a world where we have 100% correct APIs :)
Hi, groby@, thanks very much! https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_provider_test.cc (right): https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_provider_test.cc:66: messages_.push_back(base::WrapUnique<IPC::Message>(message)); On 2017/02/02 16:05:44, groby wrote: > On 2017/01/27 03:15:40, ke.he wrote: > > On 2017/01/26 17:40:47, groby wrote: > > > This makes me queasy, because the use of unique_ptr implies strict ownership > > > management. And in that world, the API implies that message is owned outside > > > this function. > > > > > > I know you can't fix it in this CL, but are you aware of any efforts to fix > > the > > > IPC API? > > > > In this testing class, the Send() function doesn't send message to browser > side, > > it only "emulate" as if it receives the response from browser side. If the > > message is handled(by itself), it deletes the message, else, it pushes the > > message into scopedVector for later destruct. So, Although the > > TestingSpellCheckProvider::Send() overrides from SpellCheckProvider, It seems > we > > don't need to change the IPC API. what do you think? Thanks:) > > AIUI, the Send() function always takes ownership - see comment in ipc_sender.h: > "The implementor takes ownership of the given Message regardless of whether or > not this method succeeds" > > So it _should_ take a unique_ptr. In a world where we have 100% correct APIs :) Thanks:) My understanding is: "the send() function takes ownership" doesn't mean that it must take a unique_ptr as parameter. the ownership means that the send() function should take the responsibility of deleting the msg. Now in send() function, the msg is either deleted directly or push_backed to "messages_" vector for later deleting. So I guess it is ok for send() to take a commmon ptr as parameter? Anyway, I will merge this CL first. do you think it is ok?
On 2017/02/03 00:58:57, ke.he wrote: > Anyway, I will merge this CL first. do you think it is ok? Please do :) I'll talk a little bit more about unique_ptr<> below, if you're interested. I'm *not* asking you to change that API, though :) > > https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... > File components/spellcheck/renderer/spellcheck_provider_test.cc (right): > > https://codereview.chromium.org/2658013002/diff/40001/components/spellcheck/r... > components/spellcheck/renderer/spellcheck_provider_test.cc:66: > messages_.push_back(base::WrapUnique<IPC::Message>(message)); > On 2017/02/02 16:05:44, groby wrote: > > On 2017/01/27 03:15:40, ke.he wrote: > > > On 2017/01/26 17:40:47, groby wrote: > > > > This makes me queasy, because the use of unique_ptr implies strict > ownership > > > > management. And in that world, the API implies that message is owned > outside > > > > this function. > > > > > > > > I know you can't fix it in this CL, but are you aware of any efforts to > fix > > > the > > > > IPC API? > > > > > > In this testing class, the Send() function doesn't send message to browser > > side, > > > it only "emulate" as if it receives the response from browser side. If the > > > message is handled(by itself), it deletes the message, else, it pushes the > > > message into scopedVector for later destruct. So, Although the > > > TestingSpellCheckProvider::Send() overrides from SpellCheckProvider, It > seems > > we > > > don't need to change the IPC API. what do you think? Thanks:) > > > > AIUI, the Send() function always takes ownership - see comment in > ipc_sender.h: > > "The implementor takes ownership of the given Message regardless of whether or > > not this method succeeds" > > > > So it _should_ take a unique_ptr. In a world where we have 100% correct APIs > :) > > Thanks:) > My understanding is: "the send() function takes ownership" doesn't mean that it > must take a unique_ptr as parameter. We usually use unique_ptr arguments to express ownership transfer.The function promises that it will delete the pointer when it's unused, and nobody else is allowed to delete it. Which is what unique_ptr implies However, IPC::Sender predates the existence of unique_ptr<> in Chromium, and before we could use unique_ptr<>, we'd just write a "takes ownership" comment and hope people do the right thing. (I.e. remember to delete the pointer). Now, with unique_ptr<>, we can write an API where "doing the right thing" is automatic. When taking a unique_ptr<> argument, it's impossible for the function to not delete the pointer on exit (unless ownership is explicitly transferred elsewhere). > the ownership means that the send() > function should take the responsibility of deleting the msg. Now in send() > function, the msg is either deleted directly or push_backed to "messages_" > vector for later deleting. If we wanted to be correct about ownership semantics and used unique_ptr<>, push_back would take ownership via std::move - that's how we normally transfer ownership of unique ptrs from one object to another. > So I guess it is ok for send() to take a commmon ptr > as parameter? In a world with unique_ptr<>, a bare ptr (or common ptr) usually means that some other entity has ownership of the pointer. It's the callers job to guarantee that the pointer will be alive until the function - here, send() - is executed. BUT: We sometimes use bare pointers even when the function technically takes ownership. That's usually for some kind of memory or performance reason. I don't think it's worth changing the IPC API at all - it's a pretty risky change (lots of subclasses, and getting ownership semantics in IPC wrong might be a sandbox escape!), for a set of functions that's pretty much frozen. (IIRC, we're using mojo for all new IPC work).
Hi, groby@, Big thanks! Now I totally understand. I really learned something from our great discussion! About the mojo---Recently my main focus is on the "Content Modularization Project" of Gamepad, DeviceSensor and Pushmessaging. Converting IPC msgs into mojo interfaces is a important part of it. Do you have any plan to convert the Spellcheck IPC msgs into mojo? I'm glad to do something (after I finish the mojofication of pushmessaging, still ongoing) Thanks again:)
The CQ bit was checked by ke.he@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc:
While running git apply --index -p1;
error: patch failed:
components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc:68
error: components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc:
patch does not apply
Patch: components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc
Index: components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc
diff --git a/components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc
b/components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc
index
1440313d4efeca9e25d54c64ba5c67321598192c..d706308036b9bac3adc685d2aaada1f3dff3346d
100644
--- a/components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc
+++ b/components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc
@@ -42,7 +42,7 @@ TEST_F(SpellCheckProviderMacTest, SingleRoundtripSuccess) {
SpellCheckHostMsg_RequestTextCheck::Param read_parameters1;
bool ok = SpellCheckHostMsg_RequestTextCheck::Read(
- provider_.messages_[0], &read_parameters1);
+ provider_.messages_[0].get(), &read_parameters1);
EXPECT_TRUE(ok);
EXPECT_EQ(std::get<2>(read_parameters1), base::UTF8ToUTF16("hello "));
@@ -68,13 +68,13 @@ TEST_F(SpellCheckProviderMacTest, TwoRoundtripSuccess) {
SpellCheckHostMsg_RequestTextCheck::Param read_parameters1;
bool ok = SpellCheckHostMsg_RequestTextCheck::Read(
- provider_.messages_[0], &read_parameters1);
+ provider_.messages_[0].get(), &read_parameters1);
EXPECT_TRUE(ok);
EXPECT_EQ(std::get<2>(read_parameters1), base::UTF8ToUTF16("hello "));
SpellCheckHostMsg_RequestTextCheck::Param read_parameters2;
- ok = SpellCheckHostMsg_RequestTextCheck::Read(
- provider_.messages_[1], &read_parameters2);
+ ok = SpellCheckHostMsg_RequestTextCheck::Read(provider_.messages_[1].get(),
+ &read_parameters2);
EXPECT_TRUE(ok);
EXPECT_EQ(std::get<2>(read_parameters2), base::UTF8ToUTF16("bye "));
The CQ bit was checked by ke.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/2658013002/#ps80001 (title: "code rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486106212896470,
"parent_rev": "ce9ab459b6cce4f6d327b83ef49a8684a8839b9b", "commit_rev":
"293806358ee234776a9f73f05fdb08a3e3e7e78a"}
Message was sent while issue was closed.
Description was changed from ========== Remove ScopedVector in components/spellcheck BUG=554289 ========== to ========== Remove ScopedVector in components/spellcheck BUG=554289 Review-Url: https://codereview.chromium.org/2658013002 Cr-Commit-Position: refs/heads/master@{#447961} Committed: https://chromium.googlesource.com/chromium/src/+/293806358ee234776a9f73f05fdb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/293806358ee234776a9f73f05fdb...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2680563002/ by wfh@chromium.org. The reason for reverting is: causing crashes on canary see crbug.com/688770. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
