chrome://restart implemented.

chrome://restart implemented.

To assist in manual testing, chrome://restart provides
a convenient mechanism to cause a restart on demand.

BUG=248210
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209553

[jam, 2013-06-10 23:28:06]

How important/needed is this? we haven't needed it in 6 years, so I'm inclined
to think that people have been able to debug so far without it. if you do add
it, no need to change content

[scheib, 2013-06-10 23:57:34]

It is coming up in packaged apps development, where many applications and or the
system must respond correctly during restarts. Many issues (e.g. from this
simplistic search
https://code.google.com/p/chromium/issues/list?q=Cr:Platform-Apps+restart) need
to be easily verified by quality assurance and the platform team.

Further, application developers should have an easy mechanism to test that their
apps are written to behave correctly through a restart. That is a growing need.

This patch adds chrome://restart to about:about for discoverability. Is there a
way to do that without being in content?

[jam, 2013-06-11 20:33:37]

On 2013/06/10 23:57:34, scheib wrote:
> It is coming up in packaged apps development, where many applications and or
the
> system must respond correctly during restarts. Many issues (e.g. from this
> simplistic search
> https://code.google.com/p/chromium/issues/list?q=Cr:Platform-Apps+restart)
need
> to be easily verified by quality assurance and the platform team.

so testers/devs can't test this now by just closing chrome and starting it
again?

> 
> Further, application developers should have an easy mechanism to test that
their
> apps are written to behave correctly through a restart. That is a growing
need.
> 
> This patch adds chrome://restart to about:about for discoverability. Is there
a
> way to do that without being in content?

about:about is implemented in chrome, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

[scheib, 2013-06-14 22:15:40]

jam, thanks, responses below & you're no longer needed for owners coverage.
sky, would you take a look please.

On 2013/06/11 20:33:37, jam wrote:
> On 2013/06/10 23:57:34, scheib wrote:
> > It is coming up in packaged apps development, where many applications and or
> the
> > system must respond correctly during restarts. Many issues (e.g. from this
> > simplistic search
> > https://code.google.com/p/chromium/issues/list?q=Cr:Platform-Apps+restart)
> need
> > to be easily verified by quality assurance and the platform team.
> 
> so testers/devs can't test this now by just closing chrome and starting it
> again?

Correct, restarting is different than shutdown & launching again. The intent
here is to reproduce the same result that e.g. a version restart would.

Restart and exit code differences:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...


> 
> > 
> > Further, application developers should have an easy mechanism to test that
> their
> > apps are written to behave correctly through a restart. That is a growing
> need.
> > 
> > This patch adds chrome://restart to about:about for discoverability. Is
there
> a
> > way to do that without being in content?
> 
> about:about is implemented in chrome, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

Thanks, this patch has been modified to be chrome/ only.

[jam, 2013-06-14 22:57:58]

On 2013/06/14 22:15:40, scheib wrote:
> jam, thanks, responses below & you're no longer needed for owners coverage.
> sky, would you take a look please.
> 
> On 2013/06/11 20:33:37, jam wrote:
> > On 2013/06/10 23:57:34, scheib wrote:
> > > It is coming up in packaged apps development, where many applications and
or
> > the
> > > system must respond correctly during restarts. Many issues (e.g. from this
> > > simplistic search
> > > https://code.google.com/p/chromium/issues/list?q=Cr:Platform-Apps+restart)
> > need
> > > to be easily verified by quality assurance and the platform team.
> > 
> > so testers/devs can't test this now by just closing chrome and starting it
> > again?
> 
> Correct, restarting is different than shutdown & launching again. The intent
> here is to reproduce the same result that e.g. a version restart would.
> 
> Restart and exit code differences:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...

I see. Naive question: how often do we need this? Since you just filed the bug
with not much information, I couldn't figure out this information myself.

if we don't need this frequently (if it's just you?), isn't chrome://flags the
same thing by toggling a setting to another value and back to default?

[scheib, 2013-06-14 23:34:21]

On Fri, Jun 14, 2013 at 3:57 PM, <jam@chromium.org> wrote:

> On 2013/06/14 22:15:40, scheib wrote:
>
>> jam, thanks, responses below & you're no longer needed for owners
>> coverage.
>> sky, would you take a look please.
>>
>
>  On 2013/06/11 20:33:37, jam wrote:
>> > On 2013/06/10 23:57:34, scheib wrote:
>> > > It is coming up in packaged apps development, where many applications
>> and
>>
> or
>
>> > the
>> > > system must respond correctly during restarts. Many issues (e.g. from
>> this
>> > > simplistic search
>> > > https://code.google.com/p/**chromium/issues/list?q=Cr:**
>>
Platform-Apps+restart<https://code.google.com/p/chromium/issues/list?q=Cr:Platform-Apps+restart>
>> )
>> > need
>> > > to be easily verified by quality assurance and the platform team.
>> >
>> > so testers/devs can't test this now by just closing chrome and starting
>> it
>> > again?
>>
>
>  Correct, restarting is different than shutdown & launching again. The
>> intent
>> here is to reproduce the same result that e.g. a version restart would.
>>
>
>  Restart and exit code differences:
>>
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/chrome/browser/lifetime/**application_lifetime.cc&sq=**
> package:chromium&type=cs&q=**application_lifetime%**2520attemptrestart%**
>
2520attemptexitinternal&l=82<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lifetime/application_lifetime.cc&sq=package:chromium&type=cs&q=application_lifetime%2520attemptrestart%2520attemptexitinternal&l=82>
>
> I see. Naive question: how often do we need this? Since you just filed the
> bug
> with not much information, I couldn't figure out this information myself.
>

Hard to say -- it was irritating me so I decided to fix it. I've run into
only a handful of issues so far. If you'd like a chromium-dev thread
surveying interest, sure. I suspect it will be discovered in about:about
and used by quite a few devs when they inevitably have a similar need.

As packaged apps development adoption grows I believe this will be useful
more frequently to both chromium developers and external app developers
ensuring their applications behave correctly through a restart.


> if we don't need this frequently (if it's just you?), isn't chrome://flags
> the
> same thing by toggling a setting to another value and back to default?
>

It took me a while to think of that method, and it does work. But, it is
more work and I don't think it's worth the code simplicity of not adding
this patch. When you are in a development iteration loop the extra steps
between:
- type chrome:restart
and
- type about:flags, mouse over to toggle something back and forth, click
restart
- type about:flags, tab, tab, enter, tab, enter, shift-tab, shift-tab,
shift-tab, shift-tab, enter
is enough to make you want to write this patch.

I see restarting as a fundamental operation Chrome does that should be easy
for developers to trigger. I appreciate scrutiny in code changes (and I'm
glad to have escaped content/*), but I'm not sure why the resistance to
this change other than "we haven't done it yet".


>
https://codereview.chromium.**org/16745008/<https://codereview.chromium.org/1...
>

[jam, 2013-06-15 00:07:01]

On 2013/06/14 23:34:21, scheib wrote:
> On Fri, Jun 14, 2013 at 3:57 PM, <mailto:jam@chromium.org> wrote:
> 
> > On 2013/06/14 22:15:40, scheib wrote:
> >
> >> jam, thanks, responses below & you're no longer needed for owners
> >> coverage.
> >> sky, would you take a look please.
> >>
> >
> >  On 2013/06/11 20:33:37, jam wrote:
> >> > On 2013/06/10 23:57:34, scheib wrote:
> >> > > It is coming up in packaged apps development, where many applications
> >> and
> >>
> > or
> >
> >> > the
> >> > > system must respond correctly during restarts. Many issues (e.g. from
> >> this
> >> > > simplistic search
> >> > > https://code.google.com/p/**chromium/issues/list?q=Cr:**
> >>
>
Platform-Apps+restart<https://code.google.com/p/chromium/issues/list?q=Cr:Platform-Apps+restart>
> >> )
> >> > need
> >> > > to be easily verified by quality assurance and the platform team.
> >> >
> >> > so testers/devs can't test this now by just closing chrome and starting
> >> it
> >> > again?
> >>
> >
> >  Correct, restarting is different than shutdown & launching again. The
> >> intent
> >> here is to reproduce the same result that e.g. a version restart would.
> >>
> >
> >  Restart and exit code differences:
> >>
> >
> > https://code.google.com/p/**chromium/codesearch#chromium/**
> > src/chrome/browser/lifetime/**application_lifetime.cc&sq=**
> > package:chromium&type=cs&q=**application_lifetime%**2520attemptrestart%**
> >
>
2520attemptexitinternal&l=82<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lifetime/application_lifetime.cc&sq=package:chromium&type=cs&q=application_lifetime%2520attemptrestart%2520attemptexitinternal&l=82>
> >
> > I see. Naive question: how often do we need this? Since you just filed the
> > bug
> > with not much information, I couldn't figure out this information myself.
> >
> 
> Hard to say -- it was irritating me so I decided to fix it. I've run into
> only a handful of issues so far. If you'd like a chromium-dev thread
> surveying interest, sure. I suspect it will be discovered in about:about
> and used by quite a few devs when they inevitably have a similar need.
> 
> As packaged apps development adoption grows I believe this will be useful
> more frequently to both chromium developers and external app developers
> ensuring their applications behave correctly through a restart.

i see. i just wasn't sure if this is needed one off now or into the future.
> 
> 
> > if we don't need this frequently (if it's just you?), isn't chrome://flags
> > the
> > same thing by toggling a setting to another value and back to default?
> >
> 
> It took me a while to think of that method, and it does work. But, it is
> more work and I don't think it's worth the code simplicity of not adding
> this patch. When you are in a development iteration loop the extra steps
> between:
> - type chrome:restart
> and
> - type about:flags, mouse over to toggle something back and forth, click
> restart
> - type about:flags, tab, tab, enter, tab, enter, shift-tab, shift-tab,
> shift-tab, shift-tab, enter
> is enough to make you want to write this patch.
> 
> I see restarting as a fundamental operation Chrome does that should be easy
> for developers to trigger. I appreciate scrutiny in code changes (and I'm
> glad to have escaped content/*), but I'm not sure why the resistance to
> this change other than "we haven't done it yet".

i was just trying to see if this is worth the (little) complexity. i.e. couldn't
figure out why the chrome/browser/lifetime/application_lifetime.cc changes are
needed?

i'll defer to sky on the review
> 
> 
> >
>
https://codereview.chromium.**org/16745008/%3Chttps://codereview.chromium.org...>
> >

[sky, 2013-06-17 14:55:27]

https://codereview.chromium.org/16745008/diff/15001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/16745008/diff/15001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:180: static bool
restart_already_requested = false;
Why do we need this? AttemptExit may not succeed.

[scheib, 2013-06-17 15:34:23]

Thanks sky:

https://codereview.chromium.org/16745008/diff/15001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/16745008/diff/15001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:180: static bool
restart_already_requested = false;
On 2013/06/17 14:55:27, sky wrote:
> Why do we need this? AttemptExit may not succeed.

WillHandleBrowserAboutURL (browser_about_handler.cc) is called twice when
navigating, which will cause a crash and failure to restart if AttemptRestart is
called twice.

[sky1, 2013-06-17 16:38:16]

Where is the crash? AFAICT it is fine to invoke AttemptRestart more than once.

On Mon, Jun 17, 2013 at 8:34 AM,  <scheib@chromium.org> wrote:
> Thanks sky:
>
>
>
>
https://codereview.chromium.org/16745008/diff/15001/chrome/browser/lifetime/a...
> File chrome/browser/lifetime/application_lifetime.cc (right):
>
>
https://codereview.chromium.org/16745008/diff/15001/chrome/browser/lifetime/a...
> chrome/browser/lifetime/application_lifetime.cc:180: static bool
> restart_already_requested = false;
> On 2013/06/17 14:55:27, sky wrote:
>>
>> Why do we need this? AttemptExit may not succeed.
>
>
> WillHandleBrowserAboutURL (browser_about_handler.cc) is called twice
> when navigating, which will cause a crash and failure to restart if
> AttemptRestart is called twice.
>
> https://codereview.chromium.org/16745008/

[scheib, 2013-06-17 17:14:27]

On Mon, Jun 17, 2013 at 9:38 AM, Scott Violet <sky@google.com> wrote:

> Where is the crash? AFAICT it is fine to invoke AttemptRestart more than
> once.
>

In omnibox code. My speculation is that the shutdown is underway with the
second invocation of WillHandleBrowserAboutURL pending.


>
> On Mon, Jun 17, 2013 at 8:34 AM,  <scheib@chromium.org> wrote:
> > Thanks sky:
> >
> >
> >
> >
>
https://codereview.chromium.org/16745008/diff/15001/chrome/browser/lifetime/a...
> > File chrome/browser/lifetime/application_lifetime.cc (right):
> >
> >
>
https://codereview.chromium.org/16745008/diff/15001/chrome/browser/lifetime/a...
> > chrome/browser/lifetime/application_lifetime.cc:180: static bool
> > restart_already_requested = false;
> > On 2013/06/17 14:55:27, sky wrote:
> >>
> >> Why do we need this? AttemptExit may not succeed.
> >
> >
> > WillHandleBrowserAboutURL (browser_about_handler.cc) is called twice
> > when navigating, which will cause a crash and failure to restart if
> > AttemptRestart is called twice.
> >
> > https://codereview.chromium.org/16745008/
>

[scheib, 2013-06-19 22:50:15]

bump.

[sky1, 2013-06-19 22:59:43]

Did you resolve the AttemptRestart() issue? As I said, adding a static
boolean to make it isn't attempted to be processed twice is wrong.

On Wed, Jun 19, 2013 at 3:50 PM,  <scheib@chromium.org> wrote:
> bump.
>
> https://codereview.chromium.org/16745008/

[scheib, 2013-06-26 23:28:19]

sky, PTAL.

Removed static limiting to just one AttemptRestart. Adjusted to handle
OmniboxViewGtk::text_view_ being null, which is by design:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[sky, 2013-06-26 23:59:51]

+estade

I'm a bit nervous about adding a conditional to the gtk omnibox code. What about
your patch makes it so we now hit it? What does the stack look like?

[scheib, 2013-06-27 00:36:29]

I don't know the root cause. It did not trigger when I plumbed the restart from
content. There may be some race condition that exposed by this path. The
text_view_ member is cleared when the gtk destroy signal is encountered. 

 OmniboxViewGtk::AdjustTextJustification()
 OmniboxViewGtk::SetTextAndSelectedRange()
 OmniboxViewGtk::SetWindowTextAndCaretPos()
 OmniboxEditModel::Revert()
 OmniboxView::RevertAll()
 OmniboxViewGtk::Update()
 LocationBarViewGtk::Update()
 BrowserToolbarGtk::UpdateWebContents()
 BrowserWindowGtk::UpdateToolbar()
 Browser::ActiveTabChanged()
 TabStripModel::NotifyIfActiveTabChanged()
 TabStripModel::NotifyIfActiveOrSelectionChanged()
 TabStripModel::SetSelection()
 TabStripModel::InsertWebContentsAt()
 TabStripModel::AddWebContents()
 chrome::Navigate()

[Evan Stade, 2013-06-27 18:08:39]

the other option is to change text_view_ to an OwnedWidgetGtk

[scheib, 2013-06-27 19:27:08]

On 2013/06/27 18:08:39, Evan Stade wrote:
> the other option is to change text_view_ to an OwnedWidgetGtk

Evan, could you offer guidance of you think this change is OK but there are
options, you're uncertain, or you think OwnedWidgetGtk is definitely the way to
go?

[Evan Stade, 2013-06-27 19:30:32]

I'm not really certain of the best fix. If |text_view_| has been destroyed it's
probably because destruction of the browser window has already started or even
finished, at which point why are we still navigating?

[scheib, 2013-07-01 21:13:43]

sky, ptal.

Deferring AttemptRestart with a PostTask removes the problem of gtk objects
being destroyed while navigating.

[sky, 2013-07-01 21:21:01]

LGTM

[commit-bot: I haz the power, 2013-07-01 21:27:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/16745008/52001

[commit-bot: I haz the power, 2013-07-02 00:14:17]

Change committed as 209553

[sanna101010, 2014-07-27 19:55:28]

lgtm

[sanna101010, 2014-07-27 19:55:30]

lgtm

[sanna101010, 2014-07-27 20:00:17]

Sos

[sanna101010, 2014-07-27 20:02:17]

Sos

[sanna101010, 2014-08-07 21:21:15]

lgtm

You hackad my café and not er se wh att i cancer som

[kristy.kstewart.stewart585, 2016-08-09 03:53:43]

On 2014/08/07 21:21:15, sanna101010 wrote:
> lgtm
> 
> You hackad my café and not er se wh att i cancer som

[kristy.kstewart.stewart585, 2016-08-09 04:00:59]

On 2016/08/09 03:53:43, kristy.kstewart.stewart585 wrote:
> On 2014/08/07 21:21:15, sanna101010 wrote:
> > lgtm
> > 
> > You hackad my café and not er se wh att i cancer som