Add more conditions to reload when --reload-killed-tab option is used.

Add more conditions to reload when --reload-killed-tab option is used.


BUG=NONE
TEST=compiles


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165831

[simonhong_, 2012-10-30 21:40:11]

Dear Scott,
Please review this patch.
Thank you.

[sky, 2012-10-30 22:33:01]

https://codereview.chromium.org/11342055/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11342055/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:652: if (crashed_status ==
base::TERMINATION_STATUS_ABNORMAL_TERMINATION ||
Move this into an anonmyous function that does all this so that code isn't
duplicated between the two functions. It should also check the command line
switch.

[simonhong_, 2012-10-31 20:48:08]

https://codereview.chromium.org/11342055/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11342055/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:652: if (crashed_status ==
base::TERMINATION_STATUS_ABNORMAL_TERMINATION ||
On 2012/10/30 22:33:01, sky wrote:
> Move this into an anonmyous function that does all this so that code isn't
> duplicated between the two functions. It should also check the command line
> switch.

Done.

[sky, 2012-10-31 21:30:15]

https://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:249: bool IsTabCrashed(WebContents* contents) {
Name this ShouldReloadCrashedTab and have it include the check for the switch.

https://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:252: if (crashed_status ==
base::TERMINATION_STATUS_ABNORMAL_TERMINATION ||
Make this a single return statement, eg:

return crashed_status == xx || crashed_status == yyy ...

[simonhong_, 2012-10-31 21:52:15]

Done!
Thank you.
2012. 11. 1. 오전 6:30에 <sky@chromium.org>님이 작성:
>
>
>
https://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
>
>
https://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser....
> chrome/browser/ui/browser.cc:249: bool IsTabCrashed(WebContents*
> contents) {
> Name this ShouldReloadCrashedTab and have it include the check for the
> switch.
>
>
https://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser....
> chrome/browser/ui/browser.cc:252: if (crashed_status ==
> base::TERMINATION_STATUS_ABNORMAL_TERMINATION ||
> Make this a single return statement, eg:
>
> return crashed_status == xx || crashed_status == yyy ...
>
> https://codereview.chromium.org/11342055/

[simonhong_, 2012-10-31 22:06:43]

http://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:249: bool IsTabCrashed(WebContents* contents) {
On 2012/10/31 21:30:15, sky wrote:
> Name this ShouldReloadCrashedTab and have it include the check for the switch.

Done.

http://codereview.chromium.org/11342055/diff/5001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:252: if (crashed_status ==
base::TERMINATION_STATUS_ABNORMAL_TERMINATION ||
On 2012/10/31 21:30:15, sky wrote:
> Make this a single return statement, eg:
> 
> return crashed_status == xx || crashed_status == yyy ...

Done.

[sky, 2012-10-31 23:42:38]

https://codereview.chromium.org/11342055/diff/9001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11342055/diff/9001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:661: if (contents) {
Combine into a single if, eg:
if (contents && ShouldReload...

https://codereview.chromium.org/11342055/diff/9001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1091: if
(ShouldReloadCrashedTab(new_contents->web_contents())) {
Same thing here.

[simonhong_, 2012-11-01 18:28:23]

I changed it into a single if statement.

On 2012/10/31 23:42:38, sky wrote:
>
https://codereview.chromium.org/11342055/diff/9001/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
> 
>
https://codereview.chromium.org/11342055/diff/9001/chrome/browser/ui/browser....
> chrome/browser/ui/browser.cc:661: if (contents) {
> Combine into a single if, eg:
> if (contents && ShouldReload...
> 
>
https://codereview.chromium.org/11342055/diff/9001/chrome/browser/ui/browser....
> chrome/browser/ui/browser.cc:1091: if
> (ShouldReloadCrashedTab(new_contents->web_contents())) {
> Same thing here.

[sky, 2012-11-01 21:37:36]

https://codereview.chromium.org/11342055/diff/11002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11342055/diff/11002/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:661: if (contents &&
ShouldReloadCrashedTab(contents)) {
nit: in general no {} for single line ifs.

[simonhong_, 2012-11-01 21:47:52]

https://codereview.chromium.org/11342055/diff/11002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11342055/diff/11002/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:661: if (contents &&
ShouldReloadCrashedTab(contents)) {
Done.
Thank you!

On 2012/11/01 21:37:36, sky wrote:
> nit: in general no {} for single line ifs.

[simonhong_, 2012-11-01 21:51:57]

On 2012/11/01 21:47:52, Simon Hong wrote:
>
https://codereview.chromium.org/11342055/diff/11002/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
> 
>
https://codereview.chromium.org/11342055/diff/11002/chrome/browser/ui/browser...
> chrome/browser/ui/browser.cc:661: if (contents &&
> ShouldReloadCrashedTab(contents)) {
> Done.
> Thank you!
> 
> On 2012/11/01 21:37:36, sky wrote:
> > nit: in general no {} for single line ifs.

Dear Scott,
I added crash animation to this issue.

[simonhong_, 2012-11-02 11:17:27]

Modified again.
Thank you.
On 2012/11/01 21:51:57, Simon Hong wrote:
> On 2012/11/01 21:47:52, Simon Hong wrote:
> >
>
https://codereview.chromium.org/11342055/diff/11002/chrome/browser/ui/browser.cc
> > File chrome/browser/ui/browser.cc (right):
> > 
> >
>
https://codereview.chromium.org/11342055/diff/11002/chrome/browser/ui/browser...
> > chrome/browser/ui/browser.cc:661: if (contents &&
> > ShouldReloadCrashedTab(contents)) {
> > Done.
> > Thank you!
> > 
> > On 2012/11/01 21:37:36, sky wrote:
> > > nit: in general no {} for single line ifs.
> 
> Dear Scott,
> I added crash animation to this issue.

[sky, 2012-11-02 15:57:37]

https://codereview.chromium.org/11342055/diff/17001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11342055/diff/17001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:1087: LOG(WARNING) << "Reloading killed tab at " <<
index;
fix indentation of these lines.

[simonhong_, 2012-11-02 21:23:51]

https://codereview.chromium.org/11342055/diff/17001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11342055/diff/17001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:1087: LOG(WARNING) << "Reloading killed tab at " <<
index;
Oops, My mistake.
Done.
On 2012/11/02 15:57:37, sky wrote:
> fix indentation of these lines.

[sky, 2012-11-02 23:15:36]

LGTM

[commit-bot: I haz the power, 2012-11-02 23:25:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/11342055/11006

[commit-bot: I haz the power, 2012-11-03 02:37:47]

Change committed as 165831