Offer to open the startup pages after a crash.

Offer to open the startup pages after a crash.

BUG=646834
TBR=jochen@chromium.org
for minor changes in components_strings.grd
TBR=asvitkine@chromium.org
for minor changes in histograms.xml

Review-Url: https://codereview.chromium.org/2644663003
Cr-Commit-Position: refs/heads/master@{#446321}
Committed: https://chromium.googlesource.com/chromium/src/+/f82ec0d63125d78d32a38be642350ebc76762b4c

[MAD, 2017-01-24 16:52:00]

Patchset #1 (id:1) has been deleted

[MAD, 2017-01-24 17:00:13]

mad@chromium.org changed reviewers:
+ georgesak@chromium.org

[MAD, 2017-01-24 17:00:14]

plz...

Thansk!

BYE
MAD

[MAD, 2017-01-24 17:01:25]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 17:01:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Georges Khalil, 2017-01-24 17:18:29]

LGTM % nits (mostly suggestions).

Thanks for tackling this!

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:272: static void
AppendURLsToBrowser(Browser* browser,
nit: I would simply move this outside of the class instead of making it static.

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:736: if
(browser->tab_strip_model()->count() == 1) {
nit: I would invert logic to make this slightly clearer (fail-fast).

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:340:
DCHECK(SessionStartupPref::GetStartupPref(browser_->profile()).type ==
nit: Not sure if it's worth it, but also a DCHECK(button ==
ui::DIALOG_BUTTON_CANCEL), in case something goes horribly wrong?

[MAD, 2017-01-24 18:24:17]

mad@chromium.org changed reviewers:
+ jochen@chromium.org, sky@chromium.org

[MAD, 2017-01-24 18:24:18]

Thanks Georges, all fixed.

Adding sky@ for OWNERS approval and 

TBR=jochen@chromium.org
for minor changes in components_strings.grd & histograms.xml

BYE
MAD

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:272: static void
AppendURLsToBrowser(Browser* browser,
On 2017/01/24 17:18:29, Georges Khalil wrote:
> nit: I would simply move this outside of the class instead of making it
static.

Done.

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:736: if
(browser->tab_strip_model()->count() == 1) {
On 2017/01/24 17:18:29, Georges Khalil wrote:
> nit: I would invert logic to make this slightly clearer (fail-fast).

Done.

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/2644663003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:340:
DCHECK(SessionStartupPref::GetStartupPref(browser_->profile()).type ==
On 2017/01/24 17:18:29, Georges Khalil wrote:
> nit: Not sure if it's worth it, but also a DCHECK(button ==
> ui::DIALOG_BUTTON_CANCEL), in case something goes horribly wrong?

Done.

[jochen (gone - plz use gerrit), 2017-01-24 18:27:04]

I can only approve UseCounter additions to histograms, for everything else
please ask somebody from the metrics team

[MAD, 2017-01-24 18:34:00]

Forgot to mention that strings and behavior was approved by Chrome UX.

[MAD, 2017-01-24 18:35:16]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 18:36:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MAD, 2017-01-24 19:42:45]

Description was changed from

==========
Offer to open the startup pages after a crash.

BUG=646834
==========

to

==========
Offer to open the startup pages after a crash.

BUG=646834

TBR=jochen@chromium.org
for minor changes in components_strings.grd
TBR=asvitkine@chromium.org
for minor changes in histograms.xml
==========

[MAD, 2017-01-24 19:43:27]

mad@chromium.org changed reviewers:
+ asvitkine@chromium.org

[MAD, 2017-01-24 19:43:28]

Sorry jochen@, I blindly relied on git cl owners.

Now adding asvitkine@, and properly adding TBR to CL description.

[commit-bot: I haz the power, 2017-01-24 19:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 19:54:10]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-01-24 20:47:49]

lgtm

https://codereview.chromium.org/2644663003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.h (right):

https://codereview.chromium.org/2644663003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.h:131: bool startup_pages_;
Nit: Naming is a bit confusing to me. How about |opened_startup_pages_|?

[jochen (gone - plz use gerrit), 2017-01-24 20:51:15]

strings lgtm

[jochen (gone - plz use gerrit), 2017-01-24 20:51:49]

I want to point out, however, that this CL doesn't have tests. Please consider
adding tests.

[MAD, 2017-01-24 22:07:23]

Good point, I added tests for the new session restore code.

We unfortunately don't have a test harness for the bubble.

Thanks!

BYE
MAD

[sky, 2017-01-24 23:00:40]

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:789: AppendURLsToBrowser(
I'm mildly concerned this isn't the same path we use if there was no crash. I
believe in the non-crash case we use
StartupBrowserCreatorImpl::OpenTabsInBrowser.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:791: if (tab_to_clobber)
You should make sure a tab was actually added.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:317: OpenStartupPages();
On quick glance this looks wrong. It's worth a comment that cancel() really
means clicked on restore startup pages.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:340: DCHECK(button ==
ui::DIALOG_BUTTON_CANCEL);
DCHECK_EQ

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:341:
DCHECK(SessionStartupPref::GetStartupPref(browser_->profile()).type ==
DCHECK_EQ: although I'm mildly worried about sync changing these out from under
you and hitting a DCHECK. I recommend removing this DCHECK.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:406:
SessionRestore::OpenStartupPagesAfterCrash(browser_);
Restoring tabs has side effects. Please restore last (e.g. close first). I have
the same comment about RestorePreviousSession, but I leave it to you if you want
to change that here.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.h (right):

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.h:131: bool startup_pages_;
As you can't both restore and click startup pages it seems an enum for the
action would be better, e.g.

enum class Action {
  RESTORED_PREVIOUS_SESSION,
  LAUNCHED_STARTUP_URLS,
  NONE,
};

[MAD, 2017-01-25 19:48:53]

Thanks! 

All addressed.

Anything else?

BYE
MAD

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:789: AppendURLsToBrowser(
On 2017/01/24 23:00:39, sky wrote:
> I'm mildly concerned this isn't the same path we use if there was no crash. I
> believe in the non-crash case we use
> StartupBrowserCreatorImpl::OpenTabsInBrowser.

I saw that, but I thought it would be too much trouble to expose in a usable way
without all the stuff that the StartupBrowserCreator needs. But after you
mentioned it, I thought I would give it a try. What do you think?

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:791: if (tab_to_clobber)
On 2017/01/24 23:00:39, sky wrote:
> You should make sure a tab was actually added.

Done.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:317: OpenStartupPages();
On 2017/01/24 23:00:40, sky wrote:
> On quick glance this looks wrong. It's worth a comment that cancel() really
> means clicked on restore startup pages.

Done.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:340: DCHECK(button ==
ui::DIALOG_BUTTON_CANCEL);
On 2017/01/24 23:00:39, sky wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:341:
DCHECK(SessionStartupPref::GetStartupPref(browser_->profile()).type ==
On 2017/01/24 23:00:39, sky wrote:
> DCHECK_EQ: although I'm mildly worried about sync changing these out from
under
> you and hitting a DCHECK. I recommend removing this DCHECK.

Done.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:406:
SessionRestore::OpenStartupPagesAfterCrash(browser_);
On 2017/01/24 23:00:40, sky wrote:
> Restoring tabs has side effects. Please restore last (e.g. close first). I
have
> the same comment about RestorePreviousSession, but I leave it to you if you
want
> to change that here.

Done.

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.h (right):

https://codereview.chromium.org/2644663003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.h:131: bool startup_pages_;
On 2017/01/24 23:00:40, sky wrote:
> As you can't both restore and click startup pages it seems an enum for the
> action would be better, e.g.
> 
> enum class Action {
>   RESTORED_PREVIOUS_SESSION,
>   LAUNCHED_STARTUP_URLS,
>   NONE,
> };

Actually, we don't need to differentiate them, so I merged them in a single
bool.

[MAD, 2017-01-25 20:49:54]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 20:50:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 22:00:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 22:00:11]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-25 22:13:18]

LGTM

https://codereview.chromium.org/2644663003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/session_crashed_bubble_view.h (right):

https://codereview.chromium.org/2644663003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/session_crashed_bubble_view.h:125: // Whether or not the
user ignored with the bubble. It is used to collect
'ignored with' -> 'ignored'.

[MAD, 2017-01-26 03:06:58]

Thanks! CQ'ing...

BYE
MAD

https://codereview.chromium.org/2644663003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/session_crashed_bubble_view.h (right):

https://codereview.chromium.org/2644663003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/session_crashed_bubble_view.h:125: // Whether or not the
user ignored with the bubble. It is used to collect
On 2017/01/25 22:13:18, sky wrote:
> 'ignored with' -> 'ignored'.

Oups, thanks... :-)

[MAD, 2017-01-26 03:07:06]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-01-26 03:07:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
georgesak@chromium.org, asvitkine@chromium.org, jochen@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2644663003/#ps180001
(title: "Last nit")

[commit-bot: I haz the power, 2017-01-26 03:07:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 03:10:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 03:10:58]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[MAD, 2017-01-26 03:31:51]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-01-26 03:31:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, jochen@chromium.org, georgesak@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2644663003/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-26 03:33:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 06:12:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 06:12:57]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[MAD, 2017-01-26 13:55:57]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2017-01-26 13:56:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 14:30:21]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1485438957341450,
"parent_rev": "bd795a387b29430c851f367b2b7f3015f47a4ff6", "commit_rev":
"f82ec0d63125d78d32a38be642350ebc76762b4c"}

[commit-bot: I haz the power, 2017-01-26 14:30:50]

Description was changed from

==========
Offer to open the startup pages after a crash.

BUG=646834

TBR=jochen@chromium.org
for minor changes in components_strings.grd
TBR=asvitkine@chromium.org
for minor changes in histograms.xml
==========

to

==========
Offer to open the startup pages after a crash.

BUG=646834

TBR=jochen@chromium.org
for minor changes in components_strings.grd
TBR=asvitkine@chromium.org
for minor changes in histograms.xml

Review-Url: https://codereview.chromium.org/2644663003
Cr-Commit-Position: refs/heads/master@{#446321}
Committed:
https://chromium.googlesource.com/chromium/src/+/f82ec0d63125d78d32a38be64235...
==========

[commit-bot: I haz the power, 2017-01-26 14:30:52]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/f82ec0d63125d78d32a38be64235...