Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929953002:
Make profile error dialog async (Closed)
DescriptionMake profile error dialog async
So that it does not run a nested message loop to block mojo
messages.
BUG=729701
Review-Url: https://codereview.chromium.org/2929953002
Cr-Commit-Position: refs/heads/master@{#479389}
Committed: https://chromium.googlesource.com/chromium/src/+/2404367d4408db72ffdda3a5d7e520ce9c4f4590
Patch Set 1 #Patch Set 2 : fix compile #Patch Set 3 : rebase #
Total comments: 7
Patch Set 4 : for #3 comments #
Messages
Total messages: 34 (25 generated)
The CQ bit was checked by xiyuan@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by xiyuan@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by xiyuan@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.
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
xiyuan@chromium.org changed reviewers: + afakhry@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
afakhry@chromium.org changed reviewers: + pkasting@chromium.org
lgtm. +pkasting@chromium.org who is the owner of this code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/simple_message_box_android.cc (right): https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/simple_message_box_android.cc:33: std::move(callback).Run(false); I don't understand the function of std::move() in this statement. It converts an lvalue to an rvalue, but I don't know why that matters. This confusion holds for the other files in this CL as well. https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/profi... File chrome/browser/ui/profile_error_dialog.cc (right): https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/profi... chrome/browser/ui/profile_error_dialog.cc:22: bool is_showing_profile_error_dialog = false; Nit: "g_" prefix? https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/simpl... File chrome/browser/ui/simple_message_box.h (right): https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/simpl... chrome/browser/ui/simple_message_box.h:8: #include "base/callback.h" callback_forward.h?
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/simple_message_box_android.cc (right): https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/simple_message_box_android.cc:33: std::move(callback).Run(false); On 2017/06/12 23:54:19, Peter Kasting wrote: > I don't understand the function of std::move() in this statement. It converts > an lvalue to an rvalue, but I don't know why that matters. > > This confusion holds for the other files in this CL as well. That is the current syntax to run a OnceClosure/OnceCallback. Doc here [1]. And one example in [2]. [1]: https://cs.chromium.org/chromium/src/docs/callback.md?rcl=762bb48da4b3ff06e5a... [2]: https://cs.chromium.org/chromium/src/base/barrier_closure.cc?rcl=eaa976b4923b... https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/profi... File chrome/browser/ui/profile_error_dialog.cc (right): https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/profi... chrome/browser/ui/profile_error_dialog.cc:22: bool is_showing_profile_error_dialog = false; On 2017/06/12 23:54:19, Peter Kasting wrote: > Nit: "g_" prefix? Done. https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/simpl... File chrome/browser/ui/simple_message_box.h (right): https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/simpl... chrome/browser/ui/simple_message_box.h:8: #include "base/callback.h" On 2017/06/12 23:54:19, Peter Kasting wrote: > callback_forward.h? Done.
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 https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... File chrome/browser/ui/android/simple_message_box_android.cc (right): https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... chrome/browser/ui/android/simple_message_box_android.cc:33: std::move(callback).Run(false); On 2017/06/13 21:49:44, xiyuan wrote: > On 2017/06/12 23:54:19, Peter Kasting wrote: > > I don't understand the function of std::move() in this statement. It converts > > an lvalue to an rvalue, but I don't know why that matters. > > > > This confusion holds for the other files in this CL as well. > > That is the current syntax to run a OnceClosure/OnceCallback. Doc here [1]. And > one example in [2]. > > [1]: > https://cs.chromium.org/chromium/src/docs/callback.md?rcl=762bb48da4b3ff06e5a... This doc doesn't really make any note of the move() call and why it's there, though. Do things not compile without it? Is it just trying to explicitly state to readers "be aware that .Run() will mutate this"? I'd really like this to be clearer about if this is "preferred" or "necessary", and in either case why. Doesn't have to be fixed in this CL, unless you aren't sure of the answer either and the code compiles without this, in which case I would hit the pause button here until we can get a definitive answer. If you're sure this is needed, then proceed, but try to update the docs separately.
On 2017/06/14 00:28:56, Peter Kasting wrote: > LGTM > > https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... > File chrome/browser/ui/android/simple_message_box_android.cc (right): > > https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... > chrome/browser/ui/android/simple_message_box_android.cc:33: > std::move(callback).Run(false); > On 2017/06/13 21:49:44, xiyuan wrote: > > On 2017/06/12 23:54:19, Peter Kasting wrote: > > > I don't understand the function of std::move() in this statement. It > converts > > > an lvalue to an rvalue, but I don't know why that matters. > > > > > > This confusion holds for the other files in this CL as well. > > > > That is the current syntax to run a OnceClosure/OnceCallback. Doc here [1]. > And > > one example in [2]. > > > > [1]: > > > https://cs.chromium.org/chromium/src/docs/callback.md?rcl=762bb48da4b3ff06e5a... > > This doc doesn't really make any note of the move() call and why it's there, > though. Do things not compile without it? Is it just trying to explicitly > state to readers "be aware that .Run() will mutate this"? I'd really like this > to be clearer about if this is "preferred" or "necessary", and in either case > why. > > Doesn't have to be fixed in this CL, unless you aren't sure of the answer either > and the code compiles without this, in which case I would hit the pause button > here until we can get a definitive answer. If you're sure this is needed, then > proceed, but try to update the docs separately. It would not compile without std::move. There are two Callback::Run function [1]. The lvalue-reference version could only be called for repating callbacks. OnceCallback can only call the rvalue-reference version of Run. [1]: https://cs.chromium.org/chromium/src/base/callback.h?rcl=c13482980f289f938644...
The CQ bit was checked by xiyuan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2929953002/#ps80001 (title: "for #3 comments")
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": 1497453195855170,
"parent_rev": "2d62599db9a2c87d542748c7992de0d24b32e513", "commit_rev":
"2404367d4408db72ffdda3a5d7e520ce9c4f4590"}
Message was sent while issue was closed.
Description was changed from ========== Make profile error dialog async So that it does not run a nested message loop to block mojo messages. BUG=729701 ========== to ========== Make profile error dialog async So that it does not run a nested message loop to block mojo messages. BUG=729701 Review-Url: https://codereview.chromium.org/2929953002 Cr-Commit-Position: refs/heads/master@{#479389} Committed: https://chromium.googlesource.com/chromium/src/+/2404367d4408db72ffdda3a5d7e5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2404367d4408db72ffdda3a5d7e5...
Message was sent while issue was closed.
On 2017/06/14 15:12:57, xiyuan wrote: > On 2017/06/14 00:28:56, Peter Kasting wrote: > > LGTM > > > > > https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... > > File chrome/browser/ui/android/simple_message_box_android.cc (right): > > > > > https://codereview.chromium.org/2929953002/diff/60001/chrome/browser/ui/andro... > > chrome/browser/ui/android/simple_message_box_android.cc:33: > > std::move(callback).Run(false); > > On 2017/06/13 21:49:44, xiyuan wrote: > > > On 2017/06/12 23:54:19, Peter Kasting wrote: > > > > I don't understand the function of std::move() in this statement. It > > converts > > > > an lvalue to an rvalue, but I don't know why that matters. > > > > > > > > This confusion holds for the other files in this CL as well. > > > > > > That is the current syntax to run a OnceClosure/OnceCallback. Doc here [1]. > > And > > > one example in [2]. > > > > > > [1]: > > > > > > https://cs.chromium.org/chromium/src/docs/callback.md?rcl=762bb48da4b3ff06e5a... > > > > This doc doesn't really make any note of the move() call and why it's there, > > though. Do things not compile without it? Is it just trying to explicitly > > state to readers "be aware that .Run() will mutate this"? I'd really like > this > > to be clearer about if this is "preferred" or "necessary", and in either case > > why. > > > > Doesn't have to be fixed in this CL, unless you aren't sure of the answer > either > > and the code compiles without this, in which case I would hit the pause button > > here until we can get a definitive answer. If you're sure this is needed, > then > > proceed, but try to update the docs separately. > > It would not compile without std::move. There are two Callback::Run function > [1]. The lvalue-reference version could only be called for repating callbacks. > OnceCallback can only call the rvalue-reference version of Run. > > [1]: > https://cs.chromium.org/chromium/src/base/callback.h?rcl=c13482980f289f938644... For the record, I found it is mentioned in "Running A Callback" section of callback.md [1]: "Note that `OnceCallback::Run` consumes the callback object and can only be invoked on a callback rvalue." [1]: https://cs.chromium.org/chromium/src/docs/callback.md?rcl=762bb48da4b3ff06e5a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
