Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801213002:
Output warning when the container window doesn't exist unexpectedly (Closed)
DescriptionOutput warning when the container window doesn't exist unexpectedly
BUG=None
TEST=try
Review-Url: https://codereview.chromium.org/2801213002
Cr-Commit-Position: refs/heads/master@{#464750}
Committed: https://chromium.googlesource.com/chromium/src/+/1ccf3c29e30f0bbaf223f9d6059c613dc927cd48
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase. #Patch Set 3 : rebase #Patch Set 4 : use CHECK. #Patch Set 5 : revert #Patch Set 6 : Used DCHECK only and commented retionale why the windw should have been created. #Messages
Total messages: 23 (11 generated)
The CQ bit was checked by oka@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.
Description was changed from ========== Output warning when container window doesn't exist unexpectedly BUG=None TEST=try ========== to ========== Output warning when the container window doesn't exist unexpectedly BUG=None TEST=try ==========
oka@chromium.org changed reviewers: + bshe@chromium.org
bshe@ PTAL.
Friendly ping.
sorry for the delay https://codereview.chromium.org/2801213002/diff/1/ui/keyboard/keyboard_contro... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/2801213002/diff/1/ui/keyboard/keyboard_contro... ui/keyboard/keyboard_controller.cc:411: LOG(WARNING) << "container_ was not initialized unexpectedly."; If you have a DCHECK, you shouldn't need to handle its failure: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... If container_.get() can't be null, perhaps remove the if check?
rebase.
rebase
use CHECK.
Description was changed from ========== Output warning when the container window doesn't exist unexpectedly BUG=None TEST=try ========== to ========== Assert container window exists when ShowKeyboardInternal is called BUG=None TEST=try ==========
revert
Description was changed from ========== Assert container window exists when ShowKeyboardInternal is called BUG=None TEST=try ========== to ========== Output warning when the container window doesn't exist unexpectedly BUG=None TEST=try ==========
Used DCHECK only and commented retionale why the windw should have been created.
@bshe PTAL. https://codereview.chromium.org/2801213002/diff/1/ui/keyboard/keyboard_contro... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/2801213002/diff/1/ui/keyboard/keyboard_contro... ui/keyboard/keyboard_controller.cc:411: LOG(WARNING) << "container_ was not initialized unexpectedly."; On 2017/04/10 13:38:37, bshe wrote: > If you have a DCHECK, you shouldn't need to handle its failure: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > If container_.get() can't be null, perhaps remove the if check? Done. I added DCHECK only and commened the rationale why the window should exist there.
On 2017/04/13 09:07:57, oka wrote: > @bshe PTAL. > > https://codereview.chromium.org/2801213002/diff/1/ui/keyboard/keyboard_contro... > File ui/keyboard/keyboard_controller.cc (right): > > https://codereview.chromium.org/2801213002/diff/1/ui/keyboard/keyboard_contro... > ui/keyboard/keyboard_controller.cc:411: LOG(WARNING) << "container_ was not > initialized unexpectedly."; > On 2017/04/10 13:38:37, bshe wrote: > > If you have a DCHECK, you shouldn't need to handle its failure: > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > If container_.get() can't be null, perhaps remove the if check? > > Done. I added DCHECK only and commened the rationale why the window should exist > there. lgtm
The CQ bit was checked by oka@chromium.org
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": 100001, "attempt_start_ts": 1492189809353260,
"parent_rev": "11af7926845a1d4bc85fb141dbcad568075e7408", "commit_rev":
"1ccf3c29e30f0bbaf223f9d6059c613dc927cd48"}
Message was sent while issue was closed.
Description was changed from ========== Output warning when the container window doesn't exist unexpectedly BUG=None TEST=try ========== to ========== Output warning when the container window doesn't exist unexpectedly BUG=None TEST=try Review-Url: https://codereview.chromium.org/2801213002 Cr-Commit-Position: refs/heads/master@{#464750} Committed: https://chromium.googlesource.com/chromium/src/+/1ccf3c29e30f0bbaf223f9d6059c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/1ccf3c29e30f0bbaf223f9d6059c... |
