Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834943002:
Don't allow a widget to send close notifications more than once. (Closed)
|
Created:
3 years, 8 months ago by Evan Stade Modified:
3 years, 7 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't allow a widget to send close notifications more than once.
Add a test to make sure this works. Also fix a couple WidgetTests that
were disabled.
BUG=714334
Review-Url: https://codereview.chromium.org/2834943002
Cr-Commit-Position: refs/heads/master@{#467171}
Committed: https://chromium.googlesource.com/chromium/src/+/bf25b17bd99b5dbeff92e62f8dddd289c335dc0a
Patch Set 1 #Patch Set 2 : dont touch that one #
Total comments: 12
Patch Set 3 : sky review #
Messages
Total messages: 27 (18 generated)
The CQ bit was checked by estade@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 estade@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.
estade@chromium.org changed reviewers: + sky@chromium.org
necessary precursor for https://codereview.chromium.org/2831233003/
Description was changed from ========== Don't allow a widget to close more than once. Add a test to make sure this works. Also fix a couple WidgetTests that were disabled. BUG=714334 ========== to ========== Don't allow a widget to send close notifications more than once. Add a test to make sure this works. Also fix a couple WidgetTests that were disabled. BUG=714334 ==========
LGTM https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget.... ui/views/widget/widget.cc:586: can_close = non_client_view_->CanClose(); While you're here simplify this to an early return, e.g. if (non_client_client_ && !non_client_view_->CanClose()) return; https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget.... ui/views/widget/widget.cc:594: // GTK tabbed panes and their children, so normal deregistration via This comment seems out of date. https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:840: class WidgetFocusManager : public TestWidgetObserver { This name implies it's some how related to views::FocusManager, which it definitely isn't. Maybe WidgetFocusCloser? Something without FocusManager in the name. https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:842: explicit WidgetFocusManager(Widget* first_widget, Widget* second_widget) no explicit. https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:858: Widget* second_widget_; optional: widget_to_close_? https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:884: // Test for crbug.com/714334 I'm ok referencing bugs ids, but I shouldn't need to go to a bug to see what a test does. The description should stand on its own.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2834943002/#ps40001 (title: "sky review")
https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget.... ui/views/widget/widget.cc:586: can_close = non_client_view_->CanClose(); On 2017/04/25 05:04:10, sky wrote: > While you're here simplify this to an early return, e.g. > > if (non_client_client_ && !non_client_view_->CanClose()) > return; Done. https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget.... ui/views/widget/widget.cc:594: // GTK tabbed panes and their children, so normal deregistration via On 2017/04/25 05:04:09, sky wrote: > This comment seems out of date. I don't know what a GTK tabbed pane is, so I don't know how to update this comment and/or remove the comment and/or code. I believe FormManager was supposed to be FocusManager, so I fixed that. https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:840: class WidgetFocusManager : public TestWidgetObserver { On 2017/04/25 05:04:10, sky wrote: > This name implies it's some how related to views::FocusManager, which it > definitely isn't. Maybe WidgetFocusCloser? Something without FocusManager in the > name. done (WidgetActivationForwarder) https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:842: explicit WidgetFocusManager(Widget* first_widget, Widget* second_widget) On 2017/04/25 05:04:10, sky wrote: > no explicit. Done. https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:858: Widget* second_widget_; On 2017/04/25 05:04:10, sky wrote: > optional: widget_to_close_? Done. https://codereview.chromium.org/2834943002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:884: // Test for crbug.com/714334 On 2017/04/25 05:04:10, sky wrote: > I'm ok referencing bugs ids, but I shouldn't need to go to a bug to see what a > test does. The description should stand on its own. Done.
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": 40001, "attempt_start_ts": 1493160759985870,
"parent_rev": "c3e69c366f3e5c247276fec6c1dba93106f14ba9", "commit_rev":
"4f2dc89b1052308bda73e9171aec00e68740c877"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493160759985870,
"parent_rev": "dd1ce10d0b94ec4e7bd5805dcae9dc91805442aa", "commit_rev":
"bf25b17bd99b5dbeff92e62f8dddd289c335dc0a"}
Message was sent while issue was closed.
Description was changed from ========== Don't allow a widget to send close notifications more than once. Add a test to make sure this works. Also fix a couple WidgetTests that were disabled. BUG=714334 ========== to ========== Don't allow a widget to send close notifications more than once. Add a test to make sure this works. Also fix a couple WidgetTests that were disabled. BUG=714334 Review-Url: https://codereview.chromium.org/2834943002 Cr-Commit-Position: refs/heads/master@{#467171} Committed: https://chromium.googlesource.com/chromium/src/+/bf25b17bd99b5dbeff92e62f8ddd... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bf25b17bd99b5dbeff92e62f8ddd...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2844933002/ by alph@chromium.org. The reason for reverting is: The enabled WidgetObserverTest.ActivationChange test seems to be flaky https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/17210.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2842403002/ by estade@chromium.org. The reason for reverting is: We will disable the failing test on Mac..
Message was sent while issue was closed.
On 2017/04/27 01:44:11, Evan Stade wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/2842403002/ by mailto:estade@chromium.org. > > The reason for reverting is: We will disable the failing test on Mac.. We don't need to revert it. We need to reland it. :-)
Message was sent while issue was closed.
Description was changed from ========== Don't allow a widget to send close notifications more than once. Add a test to make sure this works. Also fix a couple WidgetTests that were disabled. BUG=714334 Review-Url: https://codereview.chromium.org/2834943002 Cr-Commit-Position: refs/heads/master@{#467171} Committed: https://chromium.googlesource.com/chromium/src/+/bf25b17bd99b5dbeff92e62f8ddd... ========== to ========== Don't allow a widget to send close notifications more than once. Add a test to make sure this works. Also fix a couple WidgetTests that were disabled. BUG=714334 Review-Url: https://codereview.chromium.org/2834943002 Cr-Commit-Position: refs/heads/master@{#467171} Committed: https://chromium.googlesource.com/chromium/src/+/bf25b17bd99b5dbeff92e62f8ddd... ==========
The CQ bit was checked by alph@chromium.org
The CQ bit was unchecked by alph@chromium.org
Message was sent while issue was closed.
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
