Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650293002:
Make the session restore bubble not close on navigation (Closed)
DescriptionMake the session restore bubble not close on navigation
Given the chance for data loss the user should explicitly close it.
BUG=684773
R=msw@chromium.org
Review-Url: https://codereview.chromium.org/2650293002
Cr-Commit-Position: refs/heads/master@{#446146}
Committed: https://chromium.googlesource.com/chromium/src/+/a99673a93f2116245e00a6cb66f1a8e2149ed5b3
Patch Set 1 #
Total comments: 16
Patch Set 2 : prune includes #
Messages
Total messages: 16 (9 generated)
The CQ bit was checked by sky@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 sky@chromium.org
The CQ bit was checked by sky@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm with nits and a q https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:17: #include "chrome/browser/browser_process.h" nit: remove https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:18: #include "chrome/browser/chrome_notification_types.h" nit: remove https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:23: #include "chrome/browser/ui/tabs/tab_strip_model.h" nit: remove (if active web contents is no longer needed) https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:27: #include "chrome/common/url_constants.h" nit: remove https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:35: #include "content/public/browser/notification_source.h" nit: remove https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:36: #include "content/public/browser/render_frame_host.h" nit: remove https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:38: #include "ui/views/bubble/bubble_frame_view.h" nit: remove https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:166: if (!browser || !browser->tab_strip_model()->GetActiveWebContents()) { q: can we now show the bubble without a valid active web contents?
The CQ bit was checked by sky@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2650293002/#ps20001 (title: "prune includes")
https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:17: #include "chrome/browser/browser_process.h" On 2017/01/25 18:52:44, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:18: #include "chrome/browser/chrome_notification_types.h" On 2017/01/25 18:52:44, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:23: #include "chrome/browser/ui/tabs/tab_strip_model.h" On 2017/01/25 18:52:44, msw wrote: > nit: remove (if active web contents is no longer needed) Active web contents check is still worth while, so leaving include. https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:27: #include "chrome/common/url_constants.h" On 2017/01/25 18:52:44, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:35: #include "content/public/browser/notification_source.h" On 2017/01/25 18:52:44, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:36: #include "content/public/browser/render_frame_host.h" On 2017/01/25 18:52:44, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:38: #include "ui/views/bubble/bubble_frame_view.h" On 2017/01/25 18:52:44, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650293002/diff/1/chrome/browser/ui/views/ses... chrome/browser/ui/views/session_crashed_bubble_view.cc:166: if (!browser || !browser->tab_strip_model()->GetActiveWebContents()) { On 2017/01/25 18:52:44, msw wrote: > q: can we now show the bubble without a valid active web contents? Generally if there isn't an active web contents the browser is being shutdown, in which case we shouldn't show the bubble.
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": 20001, "attempt_start_ts": 1485380819727830,
"parent_rev": "2a78436aaa2277759c1d9f11b719b72cb95456b4", "commit_rev":
"a99673a93f2116245e00a6cb66f1a8e2149ed5b3"}
Message was sent while issue was closed.
Description was changed from ========== Make the session restore bubble not close on navigation Given the chance for data loss the user should explicitly close it. BUG=684773 R=msw@chromium.org ========== to ========== Make the session restore bubble not close on navigation Given the chance for data loss the user should explicitly close it. BUG=684773 R=msw@chromium.org Review-Url: https://codereview.chromium.org/2650293002 Cr-Commit-Position: refs/heads/master@{#446146} Committed: https://chromium.googlesource.com/chromium/src/+/a99673a93f2116245e00a6cb66f1... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a99673a93f2116245e00a6cb66f1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
