Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714483003:
cros: Fix restoring session windows after crash inconsistent minimized window counts (Closed)
|
Created:
3 years, 10 months ago by Qiang(Joe) Xu Modified:
3 years, 9 months ago Reviewers:
sky CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Fix restoring session windows after crash inconsistent minimized window counts
Changes:
Close the startup browser after closing the crash bubble view and then do session restore.
BUG=694854
TEST=emulator tests and device test saw bug fixed; also test coverage is added.
Patch Set 1 #
Total comments: 2
Patch Set 2 : based on ps1's comments #
Total comments: 2
Patch Set 3 : swap the first session window #Patch Set 4 : SetInitialFocus when active #
Total comments: 2
Patch Set 5 : based on ps4's comments #
Total comments: 2
Patch Set 6 : do not reorder #Patch Set 7 : fix error in above patch #Patch Set 8 : improved comments #
Total comments: 6
Patch Set 9 : RestoreSessionAfterCrash on nullptr #
Messages
Total messages: 61 (43 generated)
The CQ bit was checked by warx@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...
Description was changed from ========== Restoring pages to minimized browsers should keep those minimized BUG=694854 TEST=emulator tests; test coverage is also added. ========== to ========== cros: Restoring pages to minimized browsers should keep those minimized BUG=694854 TEST=emulator tests; test coverage is also added. ==========
warx@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== cros: Restoring pages to minimized browsers should keep those minimized BUG=694854 TEST=emulator tests; test coverage is also added. ========== to ========== cros: Restoring pages to minimized browsers should keep those minimized Changes: Pass SessionWindow's show_state to ShowBrowser() so that when RestoreTabsToBrowser(), minimized window state could keep minimized. BUG=694854 TEST=emulator tests; test coverage is also added. ==========
Hi sky@, ptal thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2714483003/diff/1/chrome/browser/sessions/ses... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/1/chrome/browser/sessions/ses... chrome/browser/sessions/session_restore.cc:641: params.initial_show_state = show_state; The show state is plumbed through here. Is the Show() call somehow triggering ignoring the intial_show_state?
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Description was changed from ========== cros: Restoring pages to minimized browsers should keep those minimized Changes: Pass SessionWindow's show_state to ShowBrowser() so that when RestoreTabsToBrowser(), minimized window state could keep minimized. BUG=694854 TEST=emulator tests; test coverage is also added. ========== to ========== cros: Restoring pages to minimized browsers should keep those minimized Changes: If the first set of tabs belongs to minimized browser, the existing browser should be minimized to keep consistent show state. BUG=694854 TEST=emulator tests; test coverage is also added. ==========
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.
Hi sky@, updated, ptal. https://codereview.chromium.org/2714483003/diff/1/chrome/browser/sessions/ses... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/1/chrome/browser/sessions/ses... chrome/browser/sessions/session_restore.cc:641: params.initial_show_state = show_state; On 2017/02/23 03:37:37, sky wrote: > The show state is plumbed through here. Is the Show() call somehow triggering > ignoring the intial_show_state? show states are all right. The ps1's remedy seems not the root cause. It is because |browser_| not null for bug case. Then it could be a show-state-mismatch between |browser_| and the browser that the first set of tabs belonged to.
https://codereview.chromium.org/2714483003/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore.cc:415: if ((*i)->show_state == ui::SHOW_STATE_MINIMIZED) If there is only one window, I think we should not minimize it. Otherwise it looks like nothing opened. Also, I think this should set the initial show state (assuming browser isn't visible already).
The CQ bit was checked by warx@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...
Description was changed from ========== cros: Restoring pages to minimized browsers should keep those minimized Changes: If the first set of tabs belongs to minimized browser, the existing browser should be minimized to keep consistent show state. BUG=694854 TEST=emulator tests; test coverage is also added. ========== to ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If the first set of tabs belongs to minimized browser, the existing browser should be minimized to keep consistent show state. BUG=694854 TEST=emulator tests; test coverage is also added. ==========
Description was changed from ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If the first set of tabs belongs to minimized browser, the existing browser should be minimized to keep consistent show state. BUG=694854 TEST=emulator tests; test coverage is also added. ========== to ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If the first session window is minimized and tabbed and the existing browser is tabbed and active, we should first try to find an unminimized If the first set of tabs belongs to minimized browser, the existing browser should be minimized to keep consistent show state. BUG=694854 TEST=emulator tests; test coverage is also added. ==========
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by warx@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...
Description was changed from ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If the first session window is minimized and tabbed and the existing browser is tabbed and active, we should first try to find an unminimized If the first set of tabs belongs to minimized browser, the existing browser should be minimized to keep consistent show state. BUG=694854 TEST=emulator tests; test coverage is also added. ========== to ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If the first session window is minimized and tabbed, and the existing browser is tabbed and active, we should first try to find an unminimized session window to swap it (this is to swap session window's show state to keep the show state restore consistent). If all session windows are minimized, since the existing browser is active, the active_window_id should become the first session window's id. Thus, |browser_to_activate| is the existing browser. BUG=694854 TEST=emulator tests; test coverage is also added. ==========
Patchset #3 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If the first session window is minimized and tabbed, and the existing browser is tabbed and active, we should first try to find an unminimized session window to swap it (this is to swap session window's show state to keep the show state restore consistent). If all session windows are minimized, since the existing browser is active, the active_window_id should become the first session window's id. Thus, |browser_to_activate| is the existing browser. BUG=694854 TEST=emulator tests; test coverage is also added. ========== to ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If the first session window is minimized and tabbed, and is restored to the existing active tabbed browser, this existing browser should be minimized. BUG=694854 TEST=emulator tests; test coverage is also added. ==========
Description was changed from ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If the first session window is minimized and tabbed, and is restored to the existing active tabbed browser, this existing browser should be minimized. BUG=694854 TEST=emulator tests; test coverage is also added. ========== to ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If restoring session windows to an existing tabbed browser, we swapped the first session window with the session window that has active_window_id. In this way, the browser_to_activate will be the existing browser, which should be the case. BUG=694854 TEST=emulator tests saw bug fixed; also test coverage is added. ==========
Description was changed from ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If restoring session windows to an existing tabbed browser, we swapped the first session window with the session window that has active_window_id. In this way, the browser_to_activate will be the existing browser, which should be the case. BUG=694854 TEST=emulator tests saw bug fixed; also test coverage is added. ========== to ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If restoring session windows to an existing tabbed browser, we swapped the first session window with the session window that has active_window_id. In this way, the browser_to_activate will be the existing browser, which should be the case. BUG=694854 TEST=emulator tests saw bug fixed; also test coverage is added. Device test may occasionally still see the bug. This is because https://cs.chromium.org/chromium/src/chrome/browser/sessions/session_restore.... may change browser show state from minimized to unminimized, which is a separate issue. ==========
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Hi sky@, ptal, thanks! https://codereview.chromium.org/2714483003/diff/20001/chrome/browser/sessions... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/20001/chrome/browser/sessions... chrome/browser/sessions/session_restore.cc:415: if ((*i)->show_state == ui::SHOW_STATE_MINIMIZED) On 2017/02/23 21:41:06, sky wrote: > If there is only one window, I think we should not minimize it. Otherwise it > looks like nothing opened. Also, I think this should set the initial show state > (assuming browser isn't visible already). Yes, I agree. We shall not minimize here. Another way is, for restoring pages to the existing browser, we already have the activated browser (the existing browser), but the first set of tabs may not have |active_window_id|, which leads to another |browser_to_activate| below. After this change, it works well on emulator tests. But it may occasionally break on device, and I find it is because https://cs.chromium.org/chromium/src/chrome/browser/sessions/session_restore.... function call will change browser from minimized to unminimized. It doesn't always change. I believe this is a separate issue.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
When you say 'fail on device.' Can you be more specific? What fails? -Scott On Mon, Mar 6, 2017 at 2:36 PM, <warx@chromium.org> wrote: > Hi sky@, ptal, thanks! > > > https://codereview.chromium.org/2714483003/diff/20001/ > chrome/browser/sessions/session_restore.cc > File chrome/browser/sessions/session_restore.cc (right): > > https://codereview.chromium.org/2714483003/diff/20001/ > chrome/browser/sessions/session_restore.cc#newcode415 > chrome/browser/sessions/session_restore.cc:415: if ((*i)->show_state == > ui::SHOW_STATE_MINIMIZED) > On 2017/02/23 21:41:06, sky wrote: > > If there is only one window, I think we should not minimize it. > Otherwise it > > looks like nothing opened. Also, I think this should set the initial > show state > > (assuming browser isn't visible already). > > Yes, I agree. We shall not minimize here. > Another way is, for restoring pages to the existing browser, we already > have the activated browser (the existing browser), but the first set of > tabs may not have |active_window_id|, which leads to another > |browser_to_activate| below. > > After this change, it works well on emulator tests. But it may > occasionally break on device, and I find it is because > https://cs.chromium.org/chromium/src/chrome/browser/ > sessions/session_restore.cc?l=451 > function call will change browser from minimized to unminimized. It > doesn't always change. I believe this is a separate issue. > > https://codereview.chromium.org/2714483003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Description was changed from ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If restoring session windows to an existing tabbed browser, we swapped the first session window with the session window that has active_window_id. In this way, the browser_to_activate will be the existing browser, which should be the case. BUG=694854 TEST=emulator tests saw bug fixed; also test coverage is added. Device test may occasionally still see the bug. This is because https://cs.chromium.org/chromium/src/chrome/browser/sessions/session_restore.... may change browser show state from minimized to unminimized, which is a separate issue. ========== to ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If restoring session windows to an existing tabbed browser, we swapped the first session window with the session window that has active_window_id. In this way, the browser_to_activate will be the existing browser, which should be the case. BUG=694854 TEST=emulator tests and device test saw bug fixed; also test coverage is added. ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/07 00:05:42, sky wrote: > When you say 'fail on device.' Can you be more specific? What fails? > > -Scott > > On Mon, Mar 6, 2017 at 2:36 PM, <mailto:warx@chromium.org> wrote: > > > Hi sky@, ptal, thanks! > > > > > > https://codereview.chromium.org/2714483003/diff/20001/ > > chrome/browser/sessions/session_restore.cc > > File chrome/browser/sessions/session_restore.cc (right): > > > > https://codereview.chromium.org/2714483003/diff/20001/ > > chrome/browser/sessions/session_restore.cc#newcode415 > > chrome/browser/sessions/session_restore.cc:415: if ((*i)->show_state == > > ui::SHOW_STATE_MINIMIZED) > > On 2017/02/23 21:41:06, sky wrote: > > > If there is only one window, I think we should not minimize it. > > Otherwise it > > > looks like nothing opened. Also, I think this should set the initial > > show state > > > (assuming browser isn't visible already). > > > > Yes, I agree. We shall not minimize here. > > Another way is, for restoring pages to the existing browser, we already > > have the activated browser (the existing browser), but the first set of > > tabs may not have |active_window_id|, which leads to another > > |browser_to_activate| below. > > > > After this change, it works well on emulator tests. But it may > > occasionally break on device, and I find it is because > > https://cs.chromium.org/chromium/src/chrome/browser/ > > sessions/session_restore.cc?l=451 > > function call will change browser from minimized to unminimized. It > > doesn't always change. I believe this is a separate issue. > > > > https://codereview.chromium.org/2714483003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Sure. That is the reporter bug's case, two browser windows, one minimized, the other is not. Crash the non-minimized browser and click 'restore pages'. Sometimes the two browsers will both unminimized. In the new patch, I find it is the line browser->tap_strip_model()->GetActiveWebContents()->SetInitialFocus() that shows the minimized window up. crbug.com/8123 describes that the initial focus may not be the location bar when restoring. So I think we should not delete this line. Then the proper fix I can think of is do not SetInitialFocus() when browser window is not active. This will leave the initial focus set by NativeWidgetAura::ShowWithWindowState.
https://codereview.chromium.org/2714483003/diff/100001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/100001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:418: for (auto j = windows->begin(); j != windows->end(); ++j) { It's confusing to have this logic here. Could you do the swap before entering the loop? Maybe always move the SessionWindow to the front with the active id?
Hi sky@, ptal, thanks! https://codereview.chromium.org/2714483003/diff/100001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/100001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:418: for (auto j = windows->begin(); j != windows->end(); ++j) { On 2017/03/07 01:05:36, sky wrote: > It's confusing to have this logic here. Could you do the swap before entering > the loop? Maybe always move the SessionWindow to the front with the active id? Done.
https://codereview.chromium.org/2714483003/diff/120001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:402: // Always move the session window with |active_window_id| to the front, so Now that I think about it you shouldn't reorder windows. The reason for that is that windows is ordered based on creation time, and creation ordering can have effects on the system. For example, on windows there is an accelerator to access the windows in an application. If you change windows you change creation ordering and the accelerator won't behave consistently.
The CQ bit was checked by warx@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 ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: If restoring session windows to an existing tabbed browser, we swapped the first session window with the session window that has active_window_id. In this way, the browser_to_activate will be the existing browser, which should be the case. BUG=694854 TEST=emulator tests and device test saw bug fixed; also test coverage is added. ========== to ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: Before restoring session windows, do a for loop to check whether we need to restore active window's tabs or the first set of tabs to the existing browser, if existing browser exists, on-record, and tabbed. BUG=694854 TEST=emulator tests and device test saw bug fixed; also test coverage is added. ==========
Hi sky@, ptal, thanks! https://codereview.chromium.org/2714483003/diff/120001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:402: // Always move the session window with |active_window_id| to the front, so On 2017/03/07 17:34:19, sky wrote: > Now that I think about it you shouldn't reorder windows. The reason for that is > that windows is ordered based on creation time, and creation ordering can have > effects on the system. For example, on windows there is an accelerator to access > the windows in an application. If you change windows you change creation > ordering and the accelerator won't behave consistently. ok, thanks for the info. Another solution I uploaded right now is, before the restoring session windows loop, do a loop to check whether |browser_| should accept the tabs from active window or the first window. The change is now, if active window exists and tabbed, do not restore first set of tabs on |browser_|, restore active window's tabs on |browser_| instead.
Before you continue with this look at the comment I have here: "I think you should only do this logic if the browser window is not visible. Otherwise you're changing the order that I mentioned previously. What state is the browser's window in when you get here?" I'm worried this patch will break the ordering that I mentioned. https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:396: for (const auto& window : *windows) { This function is getting rather long and hard to reason about. Please move your new conditions into this loop. (current code loops through windows 3 times, that feels unnecessary). https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:407: bool active_window_on_existing_browser = false; Renane to browser_is_active_window. https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:408: bool first_window_on_existing_browser = false; Rename to restore_first_window_to_browser. https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:416: active_window_on_existing_browser = true; I think you should only do this logic if the browser window is not visible. Otherwise you're changing the order that I mentioned previously. What state is the browser's window in when you get here? https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:436: (first_window_on_existing_browser && i == windows->begin())) { That you have to check windows->begin() here is confusing. How about removing first_window_on_existing_browser and checking the type?
https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:416: active_window_on_existing_browser = true; On 2017/03/07 22:41:33, sky wrote: > I think you should only do this logic if the browser window is not visible. > Otherwise you're changing the order that I mentioned previously. What state is > the browser's window in when you get here? I checked the visibility of |browser_|'s native window, it is visible. I think this makes sense since |browser_| is already opened and active before session restore starts in the reporter's case. hmm, I am a little out of solution right now. I will firstly send this comment back, and see if I can figure out another solution later.
On 2017/03/08 04:59:38, Qiang(Joe) Xu wrote: > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... > File chrome/browser/sessions/session_restore.cc (right): > > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... > chrome/browser/sessions/session_restore.cc:416: > active_window_on_existing_browser = true; > On 2017/03/07 22:41:33, sky wrote: > > I think you should only do this logic if the browser window is not visible. > > Otherwise you're changing the order that I mentioned previously. What state is > > the browser's window in when you get here? > > I checked the visibility of |browser_|'s native window, it is visible. I think > this makes sense since |browser_| is already opened and active before session > restore starts in the reporter's case. > > hmm, I am a little out of solution right now. I will firstly send this comment > back, and see if I can figure out another solution later. I suspect the right fix is that we should not be creating a browser during startup if session restore is enabled. Instead session restore should take care of creating the browser. This way session restore can be sure the first browser is created in the right state.
Description was changed from ========== cros: Fix restoring session windows to an existing browser inconsistent minimized window counts Changes: Before restoring session windows, do a for loop to check whether we need to restore active window's tabs or the first set of tabs to the existing browser, if existing browser exists, on-record, and tabbed. BUG=694854 TEST=emulator tests and device test saw bug fixed; also test coverage is added. ========== to ========== cros: Fix restoring session windows after crash inconsistent minimized window counts Changes: Close the startup browser after closing the crash bubble view and then do session restore. BUG=694854 TEST=emulator tests and device test saw bug fixed; also test coverage is added. ==========
The CQ bit was checked by warx@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.
On 2017/03/08 05:31:57, sky wrote: > On 2017/03/08 04:59:38, Qiang(Joe) Xu wrote: > > > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... > > File chrome/browser/sessions/session_restore.cc (right): > > > > > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... > > chrome/browser/sessions/session_restore.cc:416: > > active_window_on_existing_browser = true; > > On 2017/03/07 22:41:33, sky wrote: > > > I think you should only do this logic if the browser window is not visible. > > > Otherwise you're changing the order that I mentioned previously. What state > is > > > the browser's window in when you get here? > > > > I checked the visibility of |browser_|'s native window, it is visible. I think > > this makes sense since |browser_| is already opened and active before session > > restore starts in the reporter's case. > > > > hmm, I am a little out of solution right now. I will firstly send this comment > > back, and see if I can figure out another solution later. > > I suspect the right fix is that we should not be creating a browser during > startup if session restore is enabled. Instead session restore should take care > of creating the browser. This way session restore can be sure the first browser > is created in the right state. I think we must create a startup browser in this case, since user can interact with crash bubble, either choose restore session or dismiss it. The new patch introduces a new solution, "Close the startup browser after closing the crash bubble view and then do session restore." What do you think? Thanks!
That sounds like a jarring experience. Please correct me if I'm wrong, AFAICT here are the steps from the bug: two windows, A and B with A minimized crash restart chrome new window comes up, click restore button. In this case I would expect the window you clicked the restore button on to contain the contents of what was in A and because the window was already visible I wouldn't expect it to close or to minimize. In other words I don't think this is a bug. Am I missing something? -Scott On Thu, Mar 9, 2017 at 9:20 AM, <warx@chromium.org> wrote: > On 2017/03/08 05:31:57, sky wrote: >> On 2017/03/08 04:59:38, Qiang(Joe) Xu wrote: >> > >> > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... >> > File chrome/browser/sessions/session_restore.cc (right): >> > >> > >> > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... >> > chrome/browser/sessions/session_restore.cc:416: >> > active_window_on_existing_browser = true; >> > On 2017/03/07 22:41:33, sky wrote: >> > > I think you should only do this logic if the browser window is not > visible. >> > > Otherwise you're changing the order that I mentioned previously. What > state >> is >> > > the browser's window in when you get here? >> > >> > I checked the visibility of |browser_|'s native window, it is visible. I > think >> > this makes sense since |browser_| is already opened and active before > session >> > restore starts in the reporter's case. >> > >> > hmm, I am a little out of solution right now. I will firstly send this > comment >> > back, and see if I can figure out another solution later. >> >> I suspect the right fix is that we should not be creating a browser during >> startup if session restore is enabled. Instead session restore should take > care >> of creating the browser. This way session restore can be sure the first > browser >> is created in the right state. > > I think we must create a startup browser in this case, since user can > interact > with crash bubble, either choose restore session or dismiss it. > The new patch introduces a new solution, "Close the startup browser after > closing the crash bubble view and then do session restore." What do you > think? > Thanks! > > https://codereview.chromium.org/2714483003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/09 20:51:01, sky wrote: > That sounds like a jarring experience. Please correct me if I'm wrong, > AFAICT here are the steps from the bug: > > two windows, A and B with A minimized > crash > restart chrome > new window comes up, click restore button. > > In this case I would expect the window you clicked the restore button > on to contain the contents of what was in A and because the window was > already visible I wouldn't expect it to close or to minimize. In other > words I don't think this is a bug. Am I missing something? > > -Scott > > On Thu, Mar 9, 2017 at 9:20 AM, <mailto:warx@chromium.org> wrote: > > On 2017/03/08 05:31:57, sky wrote: > >> On 2017/03/08 04:59:38, Qiang(Joe) Xu wrote: > >> > > >> > > > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... > >> > File chrome/browser/sessions/session_restore.cc (right): > >> > > >> > > >> > > > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... > >> > chrome/browser/sessions/session_restore.cc:416: > >> > active_window_on_existing_browser = true; > >> > On 2017/03/07 22:41:33, sky wrote: > >> > > I think you should only do this logic if the browser window is not > > visible. > >> > > Otherwise you're changing the order that I mentioned previously. What > > state > >> is > >> > > the browser's window in when you get here? > >> > > >> > I checked the visibility of |browser_|'s native window, it is visible. I > > think > >> > this makes sense since |browser_| is already opened and active before > > session > >> > restore starts in the reporter's case. > >> > > >> > hmm, I am a little out of solution right now. I will firstly send this > > comment > >> > back, and see if I can figure out another solution later. > >> > >> I suspect the right fix is that we should not be creating a browser during > >> startup if session restore is enabled. Instead session restore should take > > care > >> of creating the browser. This way session restore can be sure the first > > browser > >> is created in the right state. > > > > I think we must create a startup browser in this case, since user can > > interact > > with crash bubble, either choose restore session or dismiss it. > > The new patch introduces a new solution, "Close the startup browser after > > closing the crash bubble view and then do session restore." What do you > > think? > > Thanks! > > > > https://codereview.chromium.org/2714483003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > That is exactly what is happening as you said. But visually, it will make one more window unminimized. I am also inclined to agree this can be a WontFix. If we both agree with this, let me close this issue and comment on the bug then.
Yes please. Close as will not fix. Sorry I didn't realize this earlier. On Thu, Mar 9, 2017 at 1:51 PM, <warx@chromium.org> wrote: > On 2017/03/09 20:51:01, sky wrote: >> That sounds like a jarring experience. Please correct me if I'm wrong, >> AFAICT here are the steps from the bug: >> >> two windows, A and B with A minimized >> crash >> restart chrome >> new window comes up, click restore button. >> >> In this case I would expect the window you clicked the restore button >> on to contain the contents of what was in A and because the window was >> already visible I wouldn't expect it to close or to minimize. In other >> words I don't think this is a bug. Am I missing something? >> >> -Scott >> >> On Thu, Mar 9, 2017 at 9:20 AM, <mailto:warx@chromium.org> wrote: >> > On 2017/03/08 05:31:57, sky wrote: >> >> On 2017/03/08 04:59:38, Qiang(Joe) Xu wrote: >> >> > >> >> >> > >> > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... >> >> > File chrome/browser/sessions/session_restore.cc (right): >> >> > >> >> > >> >> >> > >> > https://codereview.chromium.org/2714483003/diff/180001/chrome/browser/session... >> >> > chrome/browser/sessions/session_restore.cc:416: >> >> > active_window_on_existing_browser = true; >> >> > On 2017/03/07 22:41:33, sky wrote: >> >> > > I think you should only do this logic if the browser window is not >> > visible. >> >> > > Otherwise you're changing the order that I mentioned previously. >> >> > > What >> > state >> >> is >> >> > > the browser's window in when you get here? >> >> > >> >> > I checked the visibility of |browser_|'s native window, it is >> >> > visible. I >> > think >> >> > this makes sense since |browser_| is already opened and active before >> > session >> >> > restore starts in the reporter's case. >> >> > >> >> > hmm, I am a little out of solution right now. I will firstly send >> >> > this >> > comment >> >> > back, and see if I can figure out another solution later. >> >> >> >> I suspect the right fix is that we should not be creating a browser >> >> during >> >> startup if session restore is enabled. Instead session restore should >> >> take >> > care >> >> of creating the browser. This way session restore can be sure the first >> > browser >> >> is created in the right state. >> > >> > I think we must create a startup browser in this case, since user can >> > interact >> > with crash bubble, either choose restore session or dismiss it. >> > The new patch introduces a new solution, "Close the startup browser >> > after >> > closing the crash bubble view and then do session restore." What do you >> > think? >> > Thanks! >> > >> > https://codereview.chromium.org/2714483003/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > That is exactly what is happening as you said. But visually, it will make > one > more window unminimized. > I am also inclined to agree this can be a WontFix. If we both agree with > this, > let me close this issue and comment on the bug then. > > https://codereview.chromium.org/2714483003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
