Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2579193003:
arc: Implement more methods in FakeIntentHelperInstance. (Closed)
|
Created:
4 years ago by Daniel Erat Modified:
4 years ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Implement more methods in FakeIntentHelperInstance.
Implement HandleIntent() and RequestIntentHandlerList()
within FakeIntentHelperInstance.
BUG=633243
Committed: https://crrev.com/e9fd0a588722746c91c7d35ccd35affbe962202f
Cr-Commit-Position: refs/heads/master@{#439146}
Patch Set 1 #
Total comments: 17
Patch Set 2 : apply review comments #Patch Set 3 : use .Clone() for HandledIntent members #Patch Set 4 : switch push_back to emplace_back #Patch Set 5 : get std::move-based approach working #
Total comments: 8
Patch Set 6 : apply lhchavez@'s comments #Patch Set 7 : use default move constructor #
Messages
Total messages: 39 (23 generated)
derat@chromium.org changed reviewers: + hidehiko@chromium.org, lhchavez@chromium.org
https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.cc:21: : std::vector<std::string>()), this is also a bit ugly. all of these fields are base::Optional. https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.cc:98: FROM_HERE, base::Bind(callback, base::Passed(std::move(handlers)))); base::Passed(std::move()) looks very wrong to me, but i couldn't find any other way to get this to compile. :-( https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:39: struct HandledIntent { i tried hard to make this class have IntentInfoPtr and ActivityNamePtr members instead of copying out the individual fields, but i don't understand how std::move works in conjunction with mojo smart pointers. :-( on the other hand, std::string members are probably easier for tests to work with... https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:65: // RequestIntentHandlerList() call is made. do you know of any way to duplicate the IntentHandlerInfo pointers so new handlers don't need to passed in after each call to RequestIntentHandlerList()?
The CQ bit was checked by derat@chromium.org 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...
My initial scan. https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.cc:66: handled_intents_.emplace_back(HandledIntent(intent, activity)); emplace_back(intent, activity); Could you take a look at my comment in .h file, too? https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.cc:97: base::SequencedTaskRunnerHandle::Get()->PostTask( In ARC code, ThreadTaskRunnerHandle is used. How about to use it for consistency? https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.cc:98: FROM_HERE, base::Bind(callback, base::Passed(std::move(handlers)))); On 2016/12/16 02:29:03, Daniel Erat wrote: > base::Passed(std::move()) looks very wrong to me, but i couldn't find any other > way to get this to compile. :-( rvalue is actually needed for base::Passed, so it is necessary. Or, alternatively, base::Passed(&handlers) https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:39: struct HandledIntent { On 2016/12/16 02:29:03, Daniel Erat wrote: > i tried hard to make this class have IntentInfoPtr and ActivityNamePtr members > instead of copying out the individual fields, but i don't understand how > std::move works in conjunction with mojo smart pointers. :-( > > on the other hand, std::string members are probably easier for tests to work > with... How about; struct HandledIntent { HandledIntent(mojom::IntentInfoPtr intent, mojom::ActivityNamePtr activity); HandledIntent(HandledIntent&&) noexcept; ~HandledIntent(); mojom::IntentInfoPtr intent; mojom::ActivityNamePtr activity; DISALLOW_COPY(HandledIntent); }; HandledIntent::HandledIntent(mojom::IntentInfoPtr intent, mojom::ActivityNamePtr activity) : intent(std::move(intent)), activity(std::move(activity)) {} HandledIntent::HandledIntent(HandledIntent&& other) = default; HandledIntent::~HandledIntent() = default; In caller; handled_intents_.emplace_back(std::move(intent), std::move(activity)); WDYT? https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:65: // RequestIntentHandlerList() call is made. On 2016/12/16 02:29:03, Daniel Erat wrote: > do you know of any way to duplicate the IntentHandlerInfo pointers so new > handlers don't need to passed in after each call to RequestIntentHandlerList()? IntentHandlerInfoPtr::Clone() is what you need? https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/struct_ptr.h?ty... E.g. in caller: RequestIntentHandlerList() { std::vector<IntentHandlerInfoPtr> handlers; auto it = intent_handlers_.find(intent->action); if (it != intent_handlers_.end()) { handlers.reserve(it->second.size()); for (const auto& handler : it->second) { handlers.emplace_back(handler.Clone()); } } ...->PostTask( FROM_HERE, base::Bind(callback, base::Passed(&handlers))); } or something? https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:125: std::map<std::string, std::vector<mojom::IntentHandlerInfoPtr>> Could you add #include <map>?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.cc:66: handled_intents_.emplace_back(HandledIntent(intent, activity)); On 2016/12/16 04:27:12, hidehiko wrote: > emplace_back(intent, activity); > > Could you take a look at my comment in .h file, too? thanks; done. https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.cc:97: base::SequencedTaskRunnerHandle::Get()->PostTask( On 2016/12/16 04:27:12, hidehiko wrote: > In ARC code, ThreadTaskRunnerHandle is used. How about to use it for > consistency? sure. ThreadTaskRunnerHandle has a comment recommending using this instead, but i agree consistency is better. (are these methods callable from any thread?) https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.cc:98: FROM_HERE, base::Bind(callback, base::Passed(std::move(handlers)))); On 2016/12/16 04:27:12, hidehiko wrote: > On 2016/12/16 02:29:03, Daniel Erat wrote: > > base::Passed(std::move()) looks very wrong to me, but i couldn't find any > other > > way to get this to compile. :-( > > rvalue is actually needed for base::Passed, so it is necessary. Or, > alternatively, > > base::Passed(&handlers) Acknowledged. https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:39: struct HandledIntent { On 2016/12/16 04:27:12, hidehiko wrote: > On 2016/12/16 02:29:03, Daniel Erat wrote: > > i tried hard to make this class have IntentInfoPtr and ActivityNamePtr members > > instead of copying out the individual fields, but i don't understand how > > std::move works in conjunction with mojo smart pointers. :-( > > > > on the other hand, std::string members are probably easier for tests to work > > with... > > How about; > > struct HandledIntent { > HandledIntent(mojom::IntentInfoPtr intent, mojom::ActivityNamePtr activity); > HandledIntent(HandledIntent&&) noexcept; > ~HandledIntent(); > > mojom::IntentInfoPtr intent; > mojom::ActivityNamePtr activity; > > DISALLOW_COPY(HandledIntent); > }; > > HandledIntent::HandledIntent(mojom::IntentInfoPtr intent, mojom::ActivityNamePtr > activity) > : intent(std::move(intent)), activity(std::move(activity)) {} > > HandledIntent::HandledIntent(HandledIntent&& other) = default; > HandledIntent::~HandledIntent() = default; > > In caller; > > handled_intents_.emplace_back(std::move(intent), std::move(activity)); > > WDYT? i tried something similar to this earlier, but i'm getting lengthy compilation errors that i don't completely understand. if you'd like to take a look, i copied the relevant code and errors to http://gpaste/6474333710974976 -- please let me know if i'm missing something obvious. :-) https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:65: // RequestIntentHandlerList() call is made. On 2016/12/16 04:27:12, hidehiko wrote: > On 2016/12/16 02:29:03, Daniel Erat wrote: > > do you know of any way to duplicate the IntentHandlerInfo pointers so new > > handlers don't need to passed in after each call to > RequestIntentHandlerList()? > > IntentHandlerInfoPtr::Clone() is what you need? > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/struct_ptr.h?ty... > > E.g. in caller: > > RequestIntentHandlerList() { > std::vector<IntentHandlerInfoPtr> handlers; > auto it = intent_handlers_.find(intent->action); > if (it != intent_handlers_.end()) { > handlers.reserve(it->second.size()); > for (const auto& handler : it->second) { > handlers.emplace_back(handler.Clone()); > } > } > ...->PostTask( > FROM_HERE, > base::Bind(callback, base::Passed(&handlers))); > } > > or something? thanks, i missed this when i was looking through the pointer class. https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:125: std::map<std::string, std::vector<mojom::IntentHandlerInfoPtr>> On 2016/12/16 04:27:12, hidehiko wrote: > Could you add #include <map>? whoops, done.
The CQ bit was checked by derat@chromium.org 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...
https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... components/arc/test/fake_intent_helper_instance.h:39: struct HandledIntent { On 2016/12/16 05:41:04, Daniel Erat wrote: > On 2016/12/16 04:27:12, hidehiko wrote: > > On 2016/12/16 02:29:03, Daniel Erat wrote: > > > i tried hard to make this class have IntentInfoPtr and ActivityNamePtr > members > > > instead of copying out the individual fields, but i don't understand how > > > std::move works in conjunction with mojo smart pointers. :-( > > > > > > on the other hand, std::string members are probably easier for tests to work > > > with... > > > > How about; > > > > struct HandledIntent { > > HandledIntent(mojom::IntentInfoPtr intent, mojom::ActivityNamePtr activity); > > HandledIntent(HandledIntent&&) noexcept; > > ~HandledIntent(); > > > > mojom::IntentInfoPtr intent; > > mojom::ActivityNamePtr activity; > > > > DISALLOW_COPY(HandledIntent); > > }; > > > > HandledIntent::HandledIntent(mojom::IntentInfoPtr intent, > mojom::ActivityNamePtr > > activity) > > : intent(std::move(intent)), activity(std::move(activity)) {} > > > > HandledIntent::HandledIntent(HandledIntent&& other) = default; > > HandledIntent::~HandledIntent() = default; > > > > In caller; > > > > handled_intents_.emplace_back(std::move(intent), std::move(activity)); > > > > WDYT? > > i tried something similar to this earlier, but i'm getting lengthy compilation > errors that i don't completely understand. if you'd like to take a look, i > copied the relevant code and errors to http://gpaste/6474333710974976 -- please > let me know if i'm missing something obvious. :-) Wow, the copied error message is very easy to understand what's happening! Thank you for sharing. According to error message, it seems you were using const reference instead of value (const mojom::IntentInfoPtr& v.s. mojom::IntentInfoPtr) for arguments. In this case, you need value, so that the ownership can be transferred. Also, it looks necessary to add move assignment operator, which is used in emplace_back(). E.g.; HandledIntent(mojom::IntentInfoPtr intent, ...) : intent(std::move(intent)) ...; HandledIntent& operator=(HandledIntent&& other) = default; (or if removing user defined move ctor works, it'd be simpler).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/16 06:30:20, hidehiko wrote: > https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... > File components/arc/test/fake_intent_helper_instance.h (right): > > https://codereview.chromium.org/2579193003/diff/1/components/arc/test/fake_in... > components/arc/test/fake_intent_helper_instance.h:39: struct HandledIntent { > On 2016/12/16 05:41:04, Daniel Erat wrote: > > On 2016/12/16 04:27:12, hidehiko wrote: > > > On 2016/12/16 02:29:03, Daniel Erat wrote: > > > > i tried hard to make this class have IntentInfoPtr and ActivityNamePtr > > members > > > > instead of copying out the individual fields, but i don't understand how > > > > std::move works in conjunction with mojo smart pointers. :-( > > > > > > > > on the other hand, std::string members are probably easier for tests to > work > > > > with... > > > > > > How about; > > > > > > struct HandledIntent { > > > HandledIntent(mojom::IntentInfoPtr intent, mojom::ActivityNamePtr > activity); > > > HandledIntent(HandledIntent&&) noexcept; > > > ~HandledIntent(); > > > > > > mojom::IntentInfoPtr intent; > > > mojom::ActivityNamePtr activity; > > > > > > DISALLOW_COPY(HandledIntent); > > > }; > > > > > > HandledIntent::HandledIntent(mojom::IntentInfoPtr intent, > > mojom::ActivityNamePtr > > > activity) > > > : intent(std::move(intent)), activity(std::move(activity)) {} > > > > > > HandledIntent::HandledIntent(HandledIntent&& other) = default; > > > HandledIntent::~HandledIntent() = default; > > > > > > In caller; > > > > > > handled_intents_.emplace_back(std::move(intent), std::move(activity)); > > > > > > WDYT? > > > > i tried something similar to this earlier, but i'm getting lengthy compilation > > errors that i don't completely understand. if you'd like to take a look, i > > copied the relevant code and errors to http://gpaste/6474333710974976 -- > please > > let me know if i'm missing something obvious. :-) > > Wow, the copied error message is very easy to understand what's happening! Thank > you for sharing. > > According to error message, it seems you were using const reference instead of > value (const mojom::IntentInfoPtr& v.s. mojom::IntentInfoPtr) for arguments. > In this case, you need value, so that the ownership can be transferred. Also, it > looks necessary to add move assignment operator, which is used in > emplace_back(). > > E.g.; > > HandledIntent(mojom::IntentInfoPtr intent, ...) > : intent(std::move(intent)) ...; > > HandledIntent& operator=(HandledIntent&& other) = default; > > (or if removing user defined move ctor works, it'd be simpler). sigh, i'm having so much trouble with this. :-( thanks for the previous suggestions. i agree that using const references was wrong, and i had another bug where i made the activity field an IntentInfoPtr. even after that, i'm still getting errors, though: https://paste.googleplex.com/6576344552964096 i finally went the simpler-seeming route of avoiding std::move and adding traditional regular, copy, and copy assignment c'tors that call .Clone() on the member pointers. are you okay with that?
The CQ bit was checked by derat@chromium.org 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...
thanks again for the help! it looks like it works after defining move-based copy and copy-assignment operators.
The CQ bit was checked by derat@chromium.org 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.
lgtm with some nits. https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.cc:87: auto it = intent_handlers_.find(intent->action); nit: const auto? https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.cc:94: base::ThreadTaskRunnerHandle::Get()->PostTask( Can you add a brief comment as to why you need to post this to another thread? https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.h:40: HandledIntent(mojom::IntentInfoPtr intent, You can even get rid of all this boilerplate if you use aggregate initialization in the callsites: struct HandledIntent { mojom::IntentInfoPtr intent; mojom::ActivityNamePtr activity; }; ... handled_intents_.emplace_back( HandledIntent{std::move(intent), std::move(activity)}); compiled for me.
LGTM with Luis's comments!
(haven't uploaded updated version yet due to unresolved comment suggesting implicit c'tors and d'tor) https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.cc:87: auto it = intent_handlers_.find(intent->action); On 2016/12/16 16:57:44, Luis Héctor Chávez wrote: > nit: const auto? thanks, forgot to update this once the iterator was no longer erased. https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.cc:94: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/12/16 16:57:44, Luis Héctor Chávez wrote: > Can you add a brief comment as to why you need to post this to another thread? Done. https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.h:40: HandledIntent(mojom::IntentInfoPtr intent, On 2016/12/16 16:57:44, Luis Héctor Chávez wrote: > You can even get rid of all this boilerplate if you use aggregate initialization > in the callsites: > > struct HandledIntent { > mojom::IntentInfoPtr intent; > mojom::ActivityNamePtr activity; > }; > > ... > > handled_intents_.emplace_back( > HandledIntent{std::move(intent), std::move(activity)}); > > compiled for me. hmm. this was the first thing i tried in this change, but i get an error when i depend on implicit c'tors or d'tors: In file included from ../../components/arc/test/fake_intent_helper_instance.cc:5: ../../components/arc/test/fake_intent_helper_instance.h:39:3: error: [chromium-style] Complex class/struct needs an explicit out-of-line constructor. struct HandledIntent { ^ ../../components/arc/test/fake_intent_helper_instance.h:39:3: error: [chromium-style] Complex class/struct needs an explicit out-of-line destructor. 2 errors generated am i doing something wrong?
https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.h:40: HandledIntent(mojom::IntentInfoPtr intent, On 2016/12/16 17:05:37, Daniel Erat wrote: > On 2016/12/16 16:57:44, Luis Héctor Chávez wrote: > > You can even get rid of all this boilerplate if you use aggregate > initialization > > in the callsites: > > > > struct HandledIntent { > > mojom::IntentInfoPtr intent; > > mojom::ActivityNamePtr activity; > > }; > > > > ... > > > > handled_intents_.emplace_back( > > HandledIntent{std::move(intent), std::move(activity)}); > > > > compiled for me. > > hmm. this was the first thing i tried in this change, but i get an error when i > depend on implicit c'tors or d'tors: > > In file included from > ../../components/arc/test/fake_intent_helper_instance.cc:5: > ../../components/arc/test/fake_intent_helper_instance.h:39:3: error: > [chromium-style] Complex class/struct needs an explicit out-of-line constructor. > struct HandledIntent { > ^ > ../../components/arc/test/fake_intent_helper_instance.h:39:3: error: > [chromium-style] Complex class/struct needs an explicit out-of-line destructor. > 2 errors generated > > am i doing something wrong? argh, foiled again by the fact that we use multiple compilers / flags for different targets (I was building this in a cros checkout that uses g++ instead of clang). no, you're not doing anything wrong, let's keep this stuff as-is :(.
https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2579193003/diff/80001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.h:40: HandledIntent(mojom::IntentInfoPtr intent, On 2016/12/16 17:16:18, Luis Héctor Chávez wrote: > On 2016/12/16 17:05:37, Daniel Erat wrote: > > On 2016/12/16 16:57:44, Luis Héctor Chávez wrote: > > > You can even get rid of all this boilerplate if you use aggregate > > initialization > > > in the callsites: > > > > > > struct HandledIntent { > > > mojom::IntentInfoPtr intent; > > > mojom::ActivityNamePtr activity; > > > }; > > > > > > ... > > > > > > handled_intents_.emplace_back( > > > HandledIntent{std::move(intent), std::move(activity)}); > > > > > > compiled for me. > > > > hmm. this was the first thing i tried in this change, but i get an error when > i > > depend on implicit c'tors or d'tors: > > > > In file included from > > ../../components/arc/test/fake_intent_helper_instance.cc:5: > > ../../components/arc/test/fake_intent_helper_instance.h:39:3: error: > > [chromium-style] Complex class/struct needs an explicit out-of-line > constructor. > > struct HandledIntent { > > ^ > > ../../components/arc/test/fake_intent_helper_instance.h:39:3: error: > > [chromium-style] Complex class/struct needs an explicit out-of-line > destructor. > > 2 errors generated > > > > am i doing something wrong? > > argh, foiled again by the fact that we use multiple compilers / flags for > different targets (I was building this in a cros checkout that uses g++ instead > of clang). > > no, you're not doing anything wrong, let's keep this stuff as-is :(. Oh, you hit https://www.chromium.org/developers/coding-style/chromium-style-checker-errors... Optional: BTW, HandledIntent(HandledIntent&& other) = default; (in .cc) looks working and simpler to me. Up to you.
thanks again! and sorry that i am awful at c++11.
The CQ bit was checked by derat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2579193003/#ps50003 (title: "apply lhchavez@'s comments")
The CQ bit was unchecked by derat@chromium.org
thanks, just saw your comment. i've switched to the default move c'tor.
The CQ bit was checked by derat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2579193003/#ps110001 (title: "use default move constructor")
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": 110001, "attempt_start_ts": 1481909011317110,
"parent_rev": "15c578a390981217a2f8d911702aab56d2adf355", "commit_rev":
"d4e848b1ae84ecdc4a139af7fead18b0f3d3c75b"}
Message was sent while issue was closed.
Description was changed from ========== arc: Implement more methods in FakeIntentHelperInstance. Implement HandleIntent() and RequestIntentHandlerList() within FakeIntentHelperInstance. BUG=633243 ========== to ========== arc: Implement more methods in FakeIntentHelperInstance. Implement HandleIntent() and RequestIntentHandlerList() within FakeIntentHelperInstance. BUG=633243 Review-Url: https://codereview.chromium.org/2579193003 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== arc: Implement more methods in FakeIntentHelperInstance. Implement HandleIntent() and RequestIntentHandlerList() within FakeIntentHelperInstance. BUG=633243 Review-Url: https://codereview.chromium.org/2579193003 ========== to ========== arc: Implement more methods in FakeIntentHelperInstance. Implement HandleIntent() and RequestIntentHandlerList() within FakeIntentHelperInstance. BUG=633243 Committed: https://crrev.com/e9fd0a588722746c91c7d35ccd35affbe962202f Cr-Commit-Position: refs/heads/master@{#439146} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e9fd0a588722746c91c7d35ccd35affbe962202f Cr-Commit-Position: refs/heads/master@{#439146} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
