|
|
Created:
7 years, 6 months ago by scheib Modified:
4 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionchrome://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
Patch Set 1 : #Patch Set 2 : #
Total comments: 2
Patch Set 3 : text_area_ is designed to teardown first. #Patch Set 4 : PostTask to AttemptRestart #
Messages
Total messages: 31 (0 generated)
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
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?
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...
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.
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?
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... >
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...> > >
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.
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.
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/
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/ >
bump.
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/
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/...
+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?
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()
the other option is to change text_view_ to an OwnedWidgetGtk
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?
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?
sky, ptal. Deferring AttemptRestart with a PostTask removes the problem of gtk objects being destroyed while navigating.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/16745008/52001
Message was sent while issue was closed.
Change committed as 209553
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
Sos
Message was sent while issue was closed.
Sos
Message was sent while issue was closed.
lgtm You hackad my café and not er se wh att i cancer som
Message was sent while issue was closed.
On 2014/08/07 21:21:15, sanna101010 wrote: > lgtm > > You hackad my café and not er se wh att i cancer som
Message was sent while issue was closed.
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 |