Fix panels being removed from PanelManager prematurely.

Fix panels being removed from PanelManager prematurely.

BUG=102720
TEST=Manual repro steps in the bug report.

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

[jennb, 2011-11-10 21:02:21]

Should be able to create a test for this.

On Wed, Nov 9, 2011 at 7:56 PM, <prasadt@chromium.org> wrote:

> Reviewers: Dmitry Titov,
>
> Description:
> Fix panels being removed from PanelManager prematurely.
>
> BUG=102720
> TEST=Manual repro steps in the bug report.
>
>
> Please review this at
http://codereview.chromium.**org/8505047/<http://codereview.chromium.org/8505...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/ui/panels/**panel.cc
>  M chrome/browser/ui/panels/**panel_browser_view.cc
>  M chrome/browser/ui/panels/**panel_browser_window_gtk.cc
>
>
> Index: chrome/browser/ui/panels/**panel.cc
> diff --git a/chrome/browser/ui/panels/**panel.cc
> b/chrome/browser/ui/panels/**panel.cc
> index 8c416afe757eb77e4cbd0efcdb14b0**939083e703..**
> a565017c8fb3510a72ecc6f3faa8c5**4161f1a0f2 100644
> --- a/chrome/browser/ui/panels/**panel.cc
> +++ b/chrome/browser/ui/panels/**panel.cc
> @@ -168,14 +168,6 @@ void Panel::SetBounds(const gfx::Rect& bounds) {
>  // close on the first attempt.
>  void Panel::Close() {
>   native_panel_->ClosePanel();
> -
> -// TODO(dimich): Only implemented fully async on Mac. Need to update other
> -// platforms. The panel should be removed from PanelManager when and if it
> -// was actually closed. The closing can be cancelled because of
> onbeforeunload
> -// handler on the web page. http://crbug.com/102720
> -#if !defined(OS_MACOSX)
> -  manager()->Remove(this);
> -#endif
>  }
>
>  void Panel::Activate() {
> Index: chrome/browser/ui/panels/**panel_browser_view.cc
> diff --git a/chrome/browser/ui/panels/**panel_browser_view.cc
> b/chrome/browser/ui/panels/**panel_browser_view.cc
> index fe84d774019a734c6e51613234fe10**40956c6771..**
> b79179c472f3952d29728feb9e47d8**afded552cc 100644
> --- a/chrome/browser/ui/panels/**panel_browser_view.cc
> +++ b/chrome/browser/ui/panels/**panel_browser_view.cc
> @@ -56,6 +56,7 @@ PanelBrowserView::**PanelBrowserView(Browser* browser,
> Panel* panel,
>  }
>
>  PanelBrowserView::~**PanelBrowserView() {
> +  panel_->manager()->Remove(**panel_.get());
>  }
>
>  void PanelBrowserView::Init() {
> Index: chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> diff --git a/chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> b/chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> index 08434f29808b38cf7c2a079ef927ec**6c6edf3565..**
> a4881bcae9d65f56fa15a042fcc9ee**0c0b1ff0cf 100644
> --- a/chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> +++ b/chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> @@ -69,6 +69,7 @@ PanelBrowserWindowGtk::~**PanelBrowserWindowGtk() {
>     gtk_grab_remove(drag_widget_);
>     DestroyDragWidget();
>   }
> +  panel_->manager()->Remove(**panel_.get());
>  }
>
>  void PanelBrowserWindowGtk::Init() {
>
>
>

[prasadt, 2011-11-10 21:24:50]

On 2011/11/10 21:02:21, jennb wrote:
> Should be able to create a test for this.

I'm working on adding a test for this. I thought I already mentioned it earlier
today.

> On Wed, Nov 9, 2011 at 7:56 PM, <mailto:prasadt@chromium.org> wrote:
> 
> > Reviewers: Dmitry Titov,
> >
> > Description:
> > Fix panels being removed from PanelManager prematurely.
> >
> > BUG=102720
> > TEST=Manual repro steps in the bug report.
> >
> >
> > Please review this at
>
http://codereview.chromium.**org/8505047/%3Chttp://codereview.chromium.org/85...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >  M chrome/browser/ui/panels/**panel.cc
> >  M chrome/browser/ui/panels/**panel_browser_view.cc
> >  M chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> >
> >
> > Index: chrome/browser/ui/panels/**panel.cc
> > diff --git a/chrome/browser/ui/panels/**panel.cc
> > b/chrome/browser/ui/panels/**panel.cc
> > index 8c416afe757eb77e4cbd0efcdb14b0**939083e703..**
> > a565017c8fb3510a72ecc6f3faa8c5**4161f1a0f2 100644
> > --- a/chrome/browser/ui/panels/**panel.cc
> > +++ b/chrome/browser/ui/panels/**panel.cc
> > @@ -168,14 +168,6 @@ void Panel::SetBounds(const gfx::Rect& bounds) {
> >  // close on the first attempt.
> >  void Panel::Close() {
> >   native_panel_->ClosePanel();
> > -
> > -// TODO(dimich): Only implemented fully async on Mac. Need to update other
> > -// platforms. The panel should be removed from PanelManager when and if it
> > -// was actually closed. The closing can be cancelled because of
> > onbeforeunload
> > -// handler on the web page. http://crbug.com/102720
> > -#if !defined(OS_MACOSX)
> > -  manager()->Remove(this);
> > -#endif
> >  }
> >
> >  void Panel::Activate() {
> > Index: chrome/browser/ui/panels/**panel_browser_view.cc
> > diff --git a/chrome/browser/ui/panels/**panel_browser_view.cc
> > b/chrome/browser/ui/panels/**panel_browser_view.cc
> > index fe84d774019a734c6e51613234fe10**40956c6771..**
> > b79179c472f3952d29728feb9e47d8**afded552cc 100644
> > --- a/chrome/browser/ui/panels/**panel_browser_view.cc
> > +++ b/chrome/browser/ui/panels/**panel_browser_view.cc
> > @@ -56,6 +56,7 @@ PanelBrowserView::**PanelBrowserView(Browser* browser,
> > Panel* panel,
> >  }
> >
> >  PanelBrowserView::~**PanelBrowserView() {
> > +  panel_->manager()->Remove(**panel_.get());
> >  }
> >
> >  void PanelBrowserView::Init() {
> > Index: chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> > diff --git a/chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> > b/chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> > index 08434f29808b38cf7c2a079ef927ec**6c6edf3565..**
> > a4881bcae9d65f56fa15a042fcc9ee**0c0b1ff0cf 100644
> > --- a/chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> > +++ b/chrome/browser/ui/panels/**panel_browser_window_gtk.cc
> > @@ -69,6 +69,7 @@ PanelBrowserWindowGtk::~**PanelBrowserWindowGtk() {
> >     gtk_grab_remove(drag_widget_);
> >     DestroyDragWidget();
> >   }
> > +  panel_->manager()->Remove(**panel_.get());
> >  }
> >
> >  void PanelBrowserWindowGtk::Init() {
> >
> >
> >

[prasadt, 2011-11-11 04:26:46]

Added unit test.

[Dmitry Titov, 2011-11-11 05:18:10]

http://codereview.chromium.org/8505047/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8505047/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:7: #include
"base/test/test_timeouts.h"
is this include file needed?

http://codereview.chromium.org/8505047/diff/6001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8505047/diff/6001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:948: NOTIFICATION_PANEL_DELETED,
is this notification used? I see where it is fired but don't see where it is
listened for...

[dcheng, 2011-11-11 06:01:09]

Drive by.

Just my two cents. I think the test is OK as is right now, but it would be great
to see us move it to a UI test. That way we can share framework with existing UI
tests (see
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/unload...)
rather than keep adding our own testing infrastructure throughout the browser
code.

[Dmitry Titov, 2011-11-11 17:48:27]

On 2011/11/11 06:01:09, dcheng wrote:
> Drive by.
> 
> Just my two cents. I think the test is OK as is right now, but it would be
great
> to see us move it to a UI test. That way we can share framework with existing
UI
> tests (see
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/unload...)
> rather than keep adding our own testing infrastructure throughout the browser
> code.

Good point in general, I think in this specific case it's indeed ok to just keep
it a browser_test.

I think it's ok to add notifications, if that's what you mean by 'adding testing
infrastructure'... It seems to be a commonplace practice and often encouraged
(since it reduces flakiness apparently). Keeping test a browser_test makes it
easier to debug since the code is in browser process.

[prasadt, 2011-11-11 18:29:58]

http://codereview.chromium.org/8505047/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8505047/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:7: #include
"base/test/test_timeouts.h"
On 2011/11/11 05:18:10, Dmitry Titov wrote:
> is this include file needed?

I had it when I was doing timeout instead of waiting for notification.

But now that I think about it, I should have a timeout for the case where I
check to make sure that Panel is not closed. There isn't really any way to wait
for that.

I need it now for these timeouts.

http://codereview.chromium.org/8505047/diff/6001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8505047/diff/6001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:948: NOTIFICATION_PANEL_DELETED,
On 2011/11/11 05:18:10, Dmitry Titov wrote:
> is this notification used? I see where it is fired but don't see where it is
> listened for...

Its listened for in panel_browsertest.cc

[prasadt, 2011-11-11 19:22:30]

http://codereview.chromium.org/8505047/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8505047/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:7: #include
"base/test/test_timeouts.h"
On 2011/11/11 18:29:58, prasadt wrote:
> On 2011/11/11 05:18:10, Dmitry Titov wrote:
> > is this include file needed?
> 
> I had it when I was doing timeout instead of waiting for notification.
> 
> But now that I think about it, I should have a timeout for the case where I
> check to make sure that Panel is not closed. There isn't really any way to
wait
> for that.
> 
> I need it now for these timeouts.

After I sent this, I realized that I could return the "proceed" value from
RenderViewHost to figure if the window will be closed or not. Added this to
Details of the notification and using that in the test instead of timeout.

Please take another look.

[prasadt, 2011-11-11 19:29:06]

On 2011/11/11 17:48:27, Dmitry Titov wrote:
> On 2011/11/11 06:01:09, dcheng wrote:
> > Drive by.
> > 
> > Just my two cents. I think the test is OK as is right now, but it would be
> great
> > to see us move it to a UI test. That way we can share framework with
existing
> UI
> > tests (see
> >
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/unload...)
> > rather than keep adding our own testing infrastructure throughout the
browser
> > code.
> 
> Good point in general, I think in this specific case it's indeed ok to just
keep
> it a browser_test.
> 
> I think it's ok to add notifications, if that's what you mean by 'adding
testing
> infrastructure'... It seems to be a commonplace practice and often encouraged
> (since it reduces flakiness apparently). Keeping test a browser_test makes it
> easier to debug since the code is in browser process.

What dimich said, plus what they're doing in that test is sub-optimal as they're
using timeouts of 1 sec after every click on the dialog. One of the reasons is
because they don't have this notification. Its possible they may want to wait
anyway, depends on the exact intent of the test.

I'll look into fixing up some of the tests that are using timeouts currently in
a later CL.

[Dmitry Titov, 2011-11-11 20:45:13]

LGTM

[prasadt, 2011-11-11 20:51:14]

John - I need an OWNERS lgtm for content/public/browser/notification_types.h and
content/browser/renderer_host/render_view_host.cc.

Thanks,
Prasad

[jam, 2011-11-11 21:19:19]

http://codereview.chromium.org/8505047/diff/10002/content/public/browser/noti...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/8505047/diff/10002/content/public/browser/noti...
content/public/browser/notification_types.h:356:
NOTIFICATION_RENDER_VIEW_HOST_RECEIVED_ON_MSG_SHOULD_CLOSE_ACK,
notifications, in general, are a hacky way of getting some code to call another
part of code. in this case, it might be used by a test only, but then later
others might use it as well and refactoring that code which sends it becomes
complicated.

are there are no other existing notifications or callbacks that you can reuse
here?

[jennb, 2011-11-11 21:26:02]

Can you use NOTIFICATION_BROWSER_CLOSED?

On Fri, Nov 11, 2011 at 1:19 PM, <jam@chromium.org> wrote:

>
> http://codereview.chromium.**org/8505047/diff/10002/**
>
content/public/browser/**notification_types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> File content/public/browser/**notification_types.h (right):
>
> http://codereview.chromium.**org/8505047/diff/10002/**
>
content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> content/public/browser/**notification_types.h:356:
> NOTIFICATION_RENDER_VIEW_HOST_**RECEIVED_ON_MSG_SHOULD_CLOSE_**ACK,
> notifications, in general, are a hacky way of getting some code to call
> another part of code. in this case, it might be used by a test only, but
> then later others might use it as well and refactoring that code which
> sends it becomes complicated.
>
> are there are no other existing notifications or callbacks that you can
> reuse here?
>
>
http://codereview.chromium.**org/8505047/<http://codereview.chromium.org/8505...
>

[prasadt, 2011-11-11 21:28:21]

On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/8505047/diff/10002/content/public/browser/noti...
> File content/public/browser/notification_types.h (right):
> 
>
http://codereview.chromium.org/8505047/diff/10002/content/public/browser/noti...
> content/public/browser/notification_types.h:356:
> NOTIFICATION_RENDER_VIEW_HOST_RECEIVED_ON_MSG_SHOULD_CLOSE_ACK,
> notifications, in general, are a hacky way of getting some code to call
another
> part of code. in this case, it might be used by a test only, but then later
> others might use it as well and refactoring that code which sends it becomes
> complicated.
> 
> are there are no other existing notifications or callbacks that you can reuse
> here?

John,

There are no other notifications I can use as far as I can tell. I looked around
quite a bit.

Thanks,
Prasad

[prasadt, 2011-11-11 21:29:31]

On 2011/11/11 21:26:02, jennb wrote:
> Can you use NOTIFICATION_BROWSER_CLOSED?

Nope. Browser window is not always closed in this scenario.

> On Fri, Nov 11, 2011 at 1:19 PM, <mailto:jam@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/8505047/diff/10002/**
> >
>
content/public/browser/**notification_types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> > File content/public/browser/**notification_types.h (right):
> >
> > http://codereview.chromium.**org/8505047/diff/10002/**
> >
>
content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> > content/public/browser/**notification_types.h:356:
> > NOTIFICATION_RENDER_VIEW_HOST_**RECEIVED_ON_MSG_SHOULD_CLOSE_**ACK,
> > notifications, in general, are a hacky way of getting some code to call
> > another part of code. in this case, it might be used by a test only, but
> > then later others might use it as well and refactoring that code which
> > sends it becomes complicated.
> >
> > are there are no other existing notifications or callbacks that you can
> > reuse here?
> >
> >
>
http://codereview.chromium.**org/8505047/%3Chttp://codereview.chromium.org/85...>
> >

[jennb, 2011-11-11 21:33:16]

Didn't look at the new test.

http://codereview.chromium.org/8505047/diff/10002/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/8505047/diff/10002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:167:
chrome::NOTIFICATION_PANEL_DELETED,
Should use BROWSER_CLOSED notification here to verify that whenever a browser is
closed, the panel is also removed from the panel manager.

[jam, 2011-11-11 21:55:30]

On Fri, Nov 11, 2011 at 1:28 PM, <prasadt@chromium.org> wrote:

> On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
>
> http://codereview.chromium.**org/8505047/diff/10002/**
>
content/public/browser/**notification_types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
>
>> File content/public/browser/**notification_types.h (right):
>>
>
>
> http://codereview.chromium.**org/8505047/diff/10002/**
>
content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
>
>> content/public/browser/**notification_types.h:356:
>> NOTIFICATION_RENDER_VIEW_HOST_**RECEIVED_ON_MSG_SHOULD_CLOSE_**ACK,
>> notifications, in general, are a hacky way of getting some code to call
>>
> another
>
>> part of code. in this case, it might be used by a test only, but then
>> later
>> others might use it as well and refactoring that code which sends it
>> becomes
>> complicated.
>>
>
>  are there are no other existing notifications or callbacks that you can
>> reuse
>> here?
>>
>
> John,
>
> There are no other notifications I can use as far as I can tell. I looked
> around
> quite a bit.
>
> Thanks,
> Prasad
>

I don't have this code in my head, and looking in detail to investigate
alternatives would take a bit of time. What I don't understand is why this
panel code is so different from the other ways we have html content in
windows (i.e. popups, notifications etc) that it needs a new notification
to test it. Please try harder to find a way without adding a notification.
From experience, notifications really get in the way of refactoring.


>
>
http://codereview.chromium.**org/8505047/<http://codereview.chromium.org/8505...
>

[prasadt, 2011-11-11 22:02:44]

http://codereview.chromium.org/8505047/diff/10002/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/8505047/diff/10002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:167:
chrome::NOTIFICATION_PANEL_DELETED,
On 2011/11/11 21:33:16, jennb wrote:
> Should use BROWSER_CLOSED notification here to verify that whenever a browser
is
> closed, the panel is also removed from the panel manager.

Done.

[prasadt, 2011-11-11 22:26:47]

On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
> On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org> wrote:
> 
> > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
> >
> > http://codereview.chromium.**org/8505047/diff/10002/**
> >
>
content/public/browser/**notification_types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> >
> >> File content/public/browser/**notification_types.h (right):
> >>
> >
> >
> > http://codereview.chromium.**org/8505047/diff/10002/**
> >
>
content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> >
> >> content/public/browser/**notification_types.h:356:
> >> NOTIFICATION_RENDER_VIEW_HOST_**RECEIVED_ON_MSG_SHOULD_CLOSE_**ACK,
> >> notifications, in general, are a hacky way of getting some code to call
> >>
> > another
> >
> >> part of code. in this case, it might be used by a test only, but then
> >> later
> >> others might use it as well and refactoring that code which sends it
> >> becomes
> >> complicated.
> >>
> >
> >  are there are no other existing notifications or callbacks that you can
> >> reuse
> >> here?
> >>
> >
> > John,
> >
> > There are no other notifications I can use as far as I can tell. I looked
> > around
> > quite a bit.
> >
> > Thanks,
> > Prasad
> >
> 
> I don't have this code in my head, and looking in detail to investigate
> alternatives would take a bit of time. What I don't understand is why this
> panel code is so different from the other ways we have html content in
> windows (i.e. popups, notifications etc) that it needs a new notification
> to test it. Please try harder to find a way without adding a notification.
> From experience, notifications really get in the way of refactoring.

John,

The only other way that I know is to introduce a Platform::Sleep() and then
check for count of panels. And that's what some of the other tests that handle
this event are doing. They do a Platform::Sleep() for 1 second. Using Sleep()
tends to be flaky or slow depending on what you set it to.

I had this coded with a Sleep() but then did a fair amount of work to dig
through and figure out the right place to throw this notification so I can avoid
the Sleep() and I didn't see an existing notification that I can use that'll
serve my purpose.

I thought notifications are a fairly standard way of doing unit testing in
Chrome. Would you rather have the Sleep() in test code?

Thanks,
Prasad

[jam, 2011-11-11 22:40:54]

On Fri, Nov 11, 2011 at 2:26 PM, <prasadt@chromium.org> wrote:

> On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
>
>  On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org> wrote:
>>
>
>  > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
>> >
>> > http://codereview.chromium.****org/8505047/diff/10002/**
>> >
>>
>
> content/public/browser/****notification_types.h<http://**
> codereview.chromium.org/**8505047/diff/10002/content/**
>
public/browser/notification_**types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> >
>
>> >
>> >> File content/public/browser/****notification_types.h (right):
>> >>
>> >
>> >
>> > http://codereview.chromium.****org/8505047/diff/10002/**
>> >
>>
>
> content/public/browser/****notification_types.h#****newcode356<
> http://codereview.**chromium.org/8505047/diff/**
>
10002/content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> >
>
>> >
>> >> content/public/browser/****notification_types.h:356:
>> >> NOTIFICATION_RENDER_VIEW_HOST_****RECEIVED_ON_MSG_SHOULD_**
>> CLOSE_**ACK,
>>
>> >> notifications, in general, are a hacky way of getting some code to call
>> >>
>> > another
>> >
>> >> part of code. in this case, it might be used by a test only, but then
>> >> later
>> >> others might use it as well and refactoring that code which sends it
>> >> becomes
>> >> complicated.
>> >>
>> >
>> >  are there are no other existing notifications or callbacks that you can
>> >> reuse
>> >> here?
>> >>
>> >
>> > John,
>> >
>> > There are no other notifications I can use as far as I can tell. I
>> looked
>> > around
>> > quite a bit.
>> >
>> > Thanks,
>> > Prasad
>> >
>>
>
>  I don't have this code in my head, and looking in detail to investigate
>> alternatives would take a bit of time. What I don't understand is why this
>> panel code is so different from the other ways we have html content in
>> windows (i.e. popups, notifications etc) that it needs a new notification
>> to test it. Please try harder to find a way without adding a notification.
>>  From experience, notifications really get in the way of refactoring.
>>
>
> John,
>
> The only other way that I know is to introduce a Platform::Sleep() and then
> check for count of panels. And that's what some of the other tests that
> handle
> this event are doing. They do a Platform::Sleep() for 1 second. Using
> Sleep()
> tends to be flaky or slow depending on what you set it to.
>
> I had this coded with a Sleep() but then did a fair amount of work to dig
> through and figure out the right place to throw this notification so I can
> avoid
> the Sleep() and I didn't see an existing notification that I can use
> that'll
> serve my purpose.
>
> I thought notifications are a fairly standard way of doing unit testing in
> Chrome. Would you rather have the Sleep() in test code?
>

Sleep is definitely not good, as it's very flaky and slow as you point
out. Notifications
being used in tests to hook in random places is not standard or
recommended, but it's unfortunately done in some places. This makes it
really hard to refactor code.

Looking at this a little more: if I'm understanding this correctly, you
want to get notified that the onbeforeunload has run. So why not have your
tests's onbeforeunload do a setTimeout with a 0 timeout, and that can
change the title as a signal to the test. then you can use
ui_test_utils::TitleWatcher.


> Thanks,
> Prasad
>
>
http://codereview.chromium.**org/8505047/<http://codereview.chromium.org/8505...
>

[prasadt, 2011-11-11 23:06:39]

On 2011/11/11 22:40:54, John Abd-El-Malek wrote:
> On Fri, Nov 11, 2011 at 2:26 PM, <mailto:prasadt@chromium.org> wrote:
> 
> > On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
> >
> >  On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org> wrote:
> >>
> >
> >  > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
> >> >
> >> > http://codereview.chromium.****org/8505047/diff/10002/**
> >> >
> >>
> >
> > content/public/browser/****notification_types.h<http://**
> > codereview.chromium.org/**8505047/diff/10002/content/**
> >
>
public/browser/notification_**types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> > >
> >
> >> >
> >> >> File content/public/browser/****notification_types.h (right):
> >> >>
> >> >
> >> >
> >> > http://codereview.chromium.****org/8505047/diff/10002/**
> >> >
> >>
> >
> > content/public/browser/****notification_types.h#****newcode356<
> > http://codereview.**chromium.org/8505047/diff/**
> >
>
10002/content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> > >
> >
> >> >
> >> >> content/public/browser/****notification_types.h:356:
> >> >> NOTIFICATION_RENDER_VIEW_HOST_****RECEIVED_ON_MSG_SHOULD_**
> >> CLOSE_**ACK,
> >>
> >> >> notifications, in general, are a hacky way of getting some code to call
> >> >>
> >> > another
> >> >
> >> >> part of code. in this case, it might be used by a test only, but then
> >> >> later
> >> >> others might use it as well and refactoring that code which sends it
> >> >> becomes
> >> >> complicated.
> >> >>
> >> >
> >> >  are there are no other existing notifications or callbacks that you can
> >> >> reuse
> >> >> here?
> >> >>
> >> >
> >> > John,
> >> >
> >> > There are no other notifications I can use as far as I can tell. I
> >> looked
> >> > around
> >> > quite a bit.
> >> >
> >> > Thanks,
> >> > Prasad
> >> >
> >>
> >
> >  I don't have this code in my head, and looking in detail to investigate
> >> alternatives would take a bit of time. What I don't understand is why this
> >> panel code is so different from the other ways we have html content in
> >> windows (i.e. popups, notifications etc) that it needs a new notification
> >> to test it. Please try harder to find a way without adding a notification.
> >>  From experience, notifications really get in the way of refactoring.
> >>
> >
> > John,
> >
> > The only other way that I know is to introduce a Platform::Sleep() and then
> > check for count of panels. And that's what some of the other tests that
> > handle
> > this event are doing. They do a Platform::Sleep() for 1 second. Using
> > Sleep()
> > tends to be flaky or slow depending on what you set it to.
> >
> > I had this coded with a Sleep() but then did a fair amount of work to dig
> > through and figure out the right place to throw this notification so I can
> > avoid
> > the Sleep() and I didn't see an existing notification that I can use
> > that'll
> > serve my purpose.
> >
> > I thought notifications are a fairly standard way of doing unit testing in
> > Chrome. Would you rather have the Sleep() in test code?
> >
> 
> Sleep is definitely not good, as it's very flaky and slow as you point
> out. Notifications
> being used in tests to hook in random places is not standard or
> recommended, but it's unfortunately done in some places. This makes it
> really hard to refactor code.
> 
> Looking at this a little more: if I'm understanding this correctly, you
> want to get notified that the onbeforeunload has run. So why not have your
> tests's onbeforeunload do a setTimeout with a 0 timeout, and that can
> change the title as a signal to the test. then you can use
> ui_test_utils::TitleWatcher.

That would be too early. The title would've changed before the close confirm
dialog comes up. Also, I need to wait for RenderViewHost::OnMsgShouldCloseACK to
run before I can try to close the window again, which my test does. Otherwise
onbeforeunload handler will not get fired.

There are other tests in Chrome that can profitable use this notification and
stop relying on Platform::Sleep as they're doing now.

> > Thanks,
> > Prasad
> >
> >
>
http://codereview.chromium.**org/8505047/%3Chttp://codereview.chromium.org/85...>
> >

[jam, 2011-11-14 19:25:10]

On 2011/11/11 23:06:39, prasadt wrote:
> On 2011/11/11 22:40:54, John Abd-El-Malek wrote:
> > On Fri, Nov 11, 2011 at 2:26 PM, <mailto:prasadt@chromium.org> wrote:
> > 
> > > On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
> > >
> > >  On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org> wrote:
> > >>
> > >
> > >  > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
> > >> >
> > >> > http://codereview.chromium.****org/8505047/diff/10002/**
> > >> >
> > >>
> > >
> > > content/public/browser/****notification_types.h<http://**
> > > codereview.chromium.org/**8505047/diff/10002/content/**
> > >
> >
>
public/browser/notification_**types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> > > >
> > >
> > >> >
> > >> >> File content/public/browser/****notification_types.h (right):
> > >> >>
> > >> >
> > >> >
> > >> > http://codereview.chromium.****org/8505047/diff/10002/**
> > >> >
> > >>
> > >
> > > content/public/browser/****notification_types.h#****newcode356<
> > > http://codereview.**chromium.org/8505047/diff/**
> > >
> >
>
10002/content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> > > >
> > >
> > >> >
> > >> >> content/public/browser/****notification_types.h:356:
> > >> >> NOTIFICATION_RENDER_VIEW_HOST_****RECEIVED_ON_MSG_SHOULD_**
> > >> CLOSE_**ACK,
> > >>
> > >> >> notifications, in general, are a hacky way of getting some code to
call
> > >> >>
> > >> > another
> > >> >
> > >> >> part of code. in this case, it might be used by a test only, but then
> > >> >> later
> > >> >> others might use it as well and refactoring that code which sends it
> > >> >> becomes
> > >> >> complicated.
> > >> >>
> > >> >
> > >> >  are there are no other existing notifications or callbacks that you
can
> > >> >> reuse
> > >> >> here?
> > >> >>
> > >> >
> > >> > John,
> > >> >
> > >> > There are no other notifications I can use as far as I can tell. I
> > >> looked
> > >> > around
> > >> > quite a bit.
> > >> >
> > >> > Thanks,
> > >> > Prasad
> > >> >
> > >>
> > >
> > >  I don't have this code in my head, and looking in detail to investigate
> > >> alternatives would take a bit of time. What I don't understand is why
this
> > >> panel code is so different from the other ways we have html content in
> > >> windows (i.e. popups, notifications etc) that it needs a new notification
> > >> to test it. Please try harder to find a way without adding a
notification.
> > >>  From experience, notifications really get in the way of refactoring.
> > >>
> > >
> > > John,
> > >
> > > The only other way that I know is to introduce a Platform::Sleep() and
then
> > > check for count of panels. And that's what some of the other tests that
> > > handle
> > > this event are doing. They do a Platform::Sleep() for 1 second. Using
> > > Sleep()
> > > tends to be flaky or slow depending on what you set it to.
> > >
> > > I had this coded with a Sleep() but then did a fair amount of work to dig
> > > through and figure out the right place to throw this notification so I can
> > > avoid
> > > the Sleep() and I didn't see an existing notification that I can use
> > > that'll
> > > serve my purpose.
> > >
> > > I thought notifications are a fairly standard way of doing unit testing in
> > > Chrome. Would you rather have the Sleep() in test code?
> > >
> > 
> > Sleep is definitely not good, as it's very flaky and slow as you point
> > out. Notifications
> > being used in tests to hook in random places is not standard or
> > recommended, but it's unfortunately done in some places. This makes it
> > really hard to refactor code.
> > 
> > Looking at this a little more: if I'm understanding this correctly, you
> > want to get notified that the onbeforeunload has run. So why not have your
> > tests's onbeforeunload do a setTimeout with a 0 timeout, and that can
> > change the title as a signal to the test. then you can use
> > ui_test_utils::TitleWatcher.
> 
> That would be too early. The title would've changed before the close confirm
> dialog comes up. 

Are you sure, i.e have you tested this? If I do a setTimeout in a beforeunload
handler then it doesn't get called until after the dialog is closed. if the
timeout function changes the title, that'll be an async message which will also
be received after OnMsgShouldCloseACK.

> Also, I need to wait for RenderViewHost::OnMsgShouldCloseACK to
> run before I can try to close the window again, which my test does. Otherwise
> onbeforeunload handler will not get fired.
> 
> There are other tests in Chrome that can profitable use this notification and
> stop relying on Platform::Sleep as they're doing now.

if we can come up with a way to do this per above, then we can also switch other
tests to that.
> 
> > > Thanks,
> > > Prasad
> > >
> > >
> >
>
http://codereview.chromium.**org/8505047/%253Chttp://codereview.chromium.org/...>
> > >

[prasadt, 2011-11-14 19:42:14]

On 2011/11/14 19:25:10, John Abd-El-Malek wrote:
> On 2011/11/11 23:06:39, prasadt wrote:
> > On 2011/11/11 22:40:54, John Abd-El-Malek wrote:
> > > On Fri, Nov 11, 2011 at 2:26 PM, <mailto:prasadt@chromium.org> wrote:
> > > 
> > > > On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
> > > >
> > > >  On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org> wrote:
> > > >>
> > > >
> > > >  > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
> > > >> >
> > > >> > http://codereview.chromium.****org/8505047/diff/10002/**
> > > >> >
> > > >>
> > > >
> > > > content/public/browser/****notification_types.h<http://**
> > > > codereview.chromium.org/**8505047/diff/10002/content/**
> > > >
> > >
> >
>
public/browser/notification_**types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> > > > >
> > > >
> > > >> >
> > > >> >> File content/public/browser/****notification_types.h (right):
> > > >> >>
> > > >> >
> > > >> >
> > > >> > http://codereview.chromium.****org/8505047/diff/10002/**
> > > >> >
> > > >>
> > > >
> > > > content/public/browser/****notification_types.h#****newcode356<
> > > > http://codereview.**chromium.org/8505047/diff/**
> > > >
> > >
> >
>
10002/content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> > > > >
> > > >
> > > >> >
> > > >> >> content/public/browser/****notification_types.h:356:
> > > >> >> NOTIFICATION_RENDER_VIEW_HOST_****RECEIVED_ON_MSG_SHOULD_**
> > > >> CLOSE_**ACK,
> > > >>
> > > >> >> notifications, in general, are a hacky way of getting some code to
> call
> > > >> >>
> > > >> > another
> > > >> >
> > > >> >> part of code. in this case, it might be used by a test only, but
then
> > > >> >> later
> > > >> >> others might use it as well and refactoring that code which sends it
> > > >> >> becomes
> > > >> >> complicated.
> > > >> >>
> > > >> >
> > > >> >  are there are no other existing notifications or callbacks that you
> can
> > > >> >> reuse
> > > >> >> here?
> > > >> >>
> > > >> >
> > > >> > John,
> > > >> >
> > > >> > There are no other notifications I can use as far as I can tell. I
> > > >> looked
> > > >> > around
> > > >> > quite a bit.
> > > >> >
> > > >> > Thanks,
> > > >> > Prasad
> > > >> >
> > > >>
> > > >
> > > >  I don't have this code in my head, and looking in detail to investigate
> > > >> alternatives would take a bit of time. What I don't understand is why
> this
> > > >> panel code is so different from the other ways we have html content in
> > > >> windows (i.e. popups, notifications etc) that it needs a new
notification
> > > >> to test it. Please try harder to find a way without adding a
> notification.
> > > >>  From experience, notifications really get in the way of refactoring.
> > > >>
> > > >
> > > > John,
> > > >
> > > > The only other way that I know is to introduce a Platform::Sleep() and
> then
> > > > check for count of panels. And that's what some of the other tests that
> > > > handle
> > > > this event are doing. They do a Platform::Sleep() for 1 second. Using
> > > > Sleep()
> > > > tends to be flaky or slow depending on what you set it to.
> > > >
> > > > I had this coded with a Sleep() but then did a fair amount of work to
dig
> > > > through and figure out the right place to throw this notification so I
can
> > > > avoid
> > > > the Sleep() and I didn't see an existing notification that I can use
> > > > that'll
> > > > serve my purpose.
> > > >
> > > > I thought notifications are a fairly standard way of doing unit testing
in
> > > > Chrome. Would you rather have the Sleep() in test code?
> > > >
> > > 
> > > Sleep is definitely not good, as it's very flaky and slow as you point
> > > out. Notifications
> > > being used in tests to hook in random places is not standard or
> > > recommended, but it's unfortunately done in some places. This makes it
> > > really hard to refactor code.
> > > 
> > > Looking at this a little more: if I'm understanding this correctly, you
> > > want to get notified that the onbeforeunload has run. So why not have your
> > > tests's onbeforeunload do a setTimeout with a 0 timeout, and that can
> > > change the title as a signal to the test. then you can use
> > > ui_test_utils::TitleWatcher.
> > 
> > That would be too early. The title would've changed before the close confirm
> > dialog comes up. 
> 
> Are you sure, i.e have you tested this?

Yes I tested this. Go to http://www.corp.google.com/~prasadt/helloworld.html and
then try to close it.

You'll see the title change while the dialog is still up.

> If I do a setTimeout in a beforeunload
> handler then it doesn't get called until after the dialog is closed. if the
> timeout function changes the title, that'll be an async message which will
also
> be received after OnMsgShouldCloseACK.

Are you proposing using a timeout? There is a lag between the dialog being
closed and OnMsgShouldCloseACK getting called, so the timeout will need to be
large enough. Using a a timeout may have the same issues as Platform::Sleep().

Is my choice of name for the notification part of the issue? Would a better name
in any way help with your refactoring concerns?

> > Also, I need to wait for RenderViewHost::OnMsgShouldCloseACK to
> > run before I can try to close the window again, which my test does.
Otherwise
> > onbeforeunload handler will not get fired.
> > 
> > There are other tests in Chrome that can profitable use this notification
and
> > stop relying on Platform::Sleep as they're doing now.
> 
> if we can come up with a way to do this per above, then we can also switch
other
> tests to that.
> > 
> > > > Thanks,
> > > > Prasad
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.**org/8505047/%25253Chttp://codereview.chromium.or...>
> > > >

[prasadt, 2011-11-14 19:43:01]

On 2011/11/14 19:42:14, prasadt wrote:
> On 2011/11/14 19:25:10, John Abd-El-Malek wrote:
> > On 2011/11/11 23:06:39, prasadt wrote:
> > > On 2011/11/11 22:40:54, John Abd-El-Malek wrote:
> > > > On Fri, Nov 11, 2011 at 2:26 PM, <mailto:prasadt@chromium.org> wrote:
> > > > 
> > > > > On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
> > > > >
> > > > >  On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org> wrote:
> > > > >>
> > > > >
> > > > >  > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
> > > > >> >
> > > > >> > http://codereview.chromium.****org/8505047/diff/10002/**
> > > > >> >
> > > > >>
> > > > >
> > > > > content/public/browser/****notification_types.h<http://**
> > > > > codereview.chromium.org/**8505047/diff/10002/content/**
> > > > >
> > > >
> > >
> >
>
public/browser/notification_**types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> > > > > >
> > > > >
> > > > >> >
> > > > >> >> File content/public/browser/****notification_types.h (right):
> > > > >> >>
> > > > >> >
> > > > >> >
> > > > >> > http://codereview.chromium.****org/8505047/diff/10002/**
> > > > >> >
> > > > >>
> > > > >
> > > > > content/public/browser/****notification_types.h#****newcode356<
> > > > > http://codereview.**chromium.org/8505047/diff/**
> > > > >
> > > >
> > >
> >
>
10002/content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> > > > > >
> > > > >
> > > > >> >
> > > > >> >> content/public/browser/****notification_types.h:356:
> > > > >> >> NOTIFICATION_RENDER_VIEW_HOST_****RECEIVED_ON_MSG_SHOULD_**
> > > > >> CLOSE_**ACK,
> > > > >>
> > > > >> >> notifications, in general, are a hacky way of getting some code to
> > call
> > > > >> >>
> > > > >> > another
> > > > >> >
> > > > >> >> part of code. in this case, it might be used by a test only, but
> then
> > > > >> >> later
> > > > >> >> others might use it as well and refactoring that code which sends
it
> > > > >> >> becomes
> > > > >> >> complicated.
> > > > >> >>
> > > > >> >
> > > > >> >  are there are no other existing notifications or callbacks that
you
> > can
> > > > >> >> reuse
> > > > >> >> here?
> > > > >> >>
> > > > >> >
> > > > >> > John,
> > > > >> >
> > > > >> > There are no other notifications I can use as far as I can tell. I
> > > > >> looked
> > > > >> > around
> > > > >> > quite a bit.
> > > > >> >
> > > > >> > Thanks,
> > > > >> > Prasad
> > > > >> >
> > > > >>
> > > > >
> > > > >  I don't have this code in my head, and looking in detail to
investigate
> > > > >> alternatives would take a bit of time. What I don't understand is why
> > this
> > > > >> panel code is so different from the other ways we have html content
in
> > > > >> windows (i.e. popups, notifications etc) that it needs a new
> notification
> > > > >> to test it. Please try harder to find a way without adding a
> > notification.
> > > > >>  From experience, notifications really get in the way of refactoring.
> > > > >>
> > > > >
> > > > > John,
> > > > >
> > > > > The only other way that I know is to introduce a Platform::Sleep() and
> > then
> > > > > check for count of panels. And that's what some of the other tests
that
> > > > > handle
> > > > > this event are doing. They do a Platform::Sleep() for 1 second. Using
> > > > > Sleep()
> > > > > tends to be flaky or slow depending on what you set it to.
> > > > >
> > > > > I had this coded with a Sleep() but then did a fair amount of work to
> dig
> > > > > through and figure out the right place to throw this notification so I
> can
> > > > > avoid
> > > > > the Sleep() and I didn't see an existing notification that I can use
> > > > > that'll
> > > > > serve my purpose.
> > > > >
> > > > > I thought notifications are a fairly standard way of doing unit
testing
> in
> > > > > Chrome. Would you rather have the Sleep() in test code?
> > > > >
> > > > 
> > > > Sleep is definitely not good, as it's very flaky and slow as you point
> > > > out. Notifications
> > > > being used in tests to hook in random places is not standard or
> > > > recommended, but it's unfortunately done in some places. This makes it
> > > > really hard to refactor code.
> > > > 
> > > > Looking at this a little more: if I'm understanding this correctly, you
> > > > want to get notified that the onbeforeunload has run. So why not have
your
> > > > tests's onbeforeunload do a setTimeout with a 0 timeout, and that can
> > > > change the title as a signal to the test. then you can use
> > > > ui_test_utils::TitleWatcher.
> > > 
> > > That would be too early. The title would've changed before the close
confirm
> > > dialog comes up. 
> > 
> > Are you sure, i.e have you tested this?
> 
> Yes I tested this. Go to http://www.corp.google.com/%7Eprasadt/helloworld.html
and
> then try to close it.
> 
> You'll see the title change while the dialog is still up.
> 
> > If I do a setTimeout in a beforeunload
> > handler then it doesn't get called until after the dialog is closed. if the
> > timeout function changes the title, that'll be an async message which will
> also
> > be received after OnMsgShouldCloseACK.
> 
> Are you proposing using a timeout? There is a lag between the dialog being
> closed and OnMsgShouldCloseACK getting called, so the timeout will need to be
> large enough. Using a a timeout may have the same issues as Platform::Sleep().
> 
> Is my choice of name for the notification part of the issue? Would a better
name
> in any way help with your refactoring concerns?
> 
> > > Also, I need to wait for RenderViewHost::OnMsgShouldCloseACK to
> > > run before I can try to close the window again, which my test does.
> Otherwise
> > > onbeforeunload handler will not get fired.
> > > 
> > > There are other tests in Chrome that can profitable use this notification
> and
> > > stop relying on Platform::Sleep as they're doing now.
> > 
> > if we can come up with a way to do this per above, then we can also switch
> other
> > tests to that.

Yes. Agreed. i.e if we can.

> > > 
> > > > > Thanks,
> > > > > Prasad
> > > > >
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.**org/8505047/%2525253Chttp://codereview.chromium....>
> > > > >

[prasadt, 2011-11-14 20:15:46]

John,

Here is the sequence of messages being traded between RenderViewHost and
RenderView as part of the closing sequence:

1) RenderViewHost send ViewMsg_ShouldClose
2) RenderView sends ViewHostMsg_RunBeforeUnloadConfirm and runs a nested message
loop. So its waiting for a response.
3) RenderViewHost creates and runs the AppModalDialog and return after user
handles.
4) RenderView now gets of the nested loop and sends back
ViewHostMsg_ShouldClose_ACK message.
5) RenderViewHost as part of processing this message, resets its state where it
can handle another onbeforeunload handler.

What I need to do in my test is to ensure that step 5) happened. And I'm calling
the notificaton with this particular name,
NOTIFICATION_RENDER_VIEW_HOST_RECEIVED_ON_MSG_SHOULD_CLOSE_ACK, because it
notifies that ViewHostMsg_ShouldClose_ACK is receiving. This seems consistent
with the naming for other similar notifications.

I hope this helps under the motivation for my approach and give you better
context to suggest a more effective way to handle this.

Thanks,
Prasad

On 2011/11/14 19:43:01, prasadt wrote:
> On 2011/11/14 19:42:14, prasadt wrote:
> > On 2011/11/14 19:25:10, John Abd-El-Malek wrote:
> > > On 2011/11/11 23:06:39, prasadt wrote:
> > > > On 2011/11/11 22:40:54, John Abd-El-Malek wrote:
> > > > > On Fri, Nov 11, 2011 at 2:26 PM, <mailto:prasadt@chromium.org> wrote:
> > > > > 
> > > > > > On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
> > > > > >
> > > > > >  On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org>
wrote:
> > > > > >>
> > > > > >
> > > > > >  > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
> > > > > >> >
> > > > > >> > http://codereview.chromium.****org/8505047/diff/10002/**
> > > > > >> >
> > > > > >>
> > > > > >
> > > > > > content/public/browser/****notification_types.h<http://**
> > > > > > codereview.chromium.org/**8505047/diff/10002/content/**
> > > > > >
> > > > >
> > > >
> > >
> >
>
public/browser/notification_**types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> > > > > > >
> > > > > >
> > > > > >> >
> > > > > >> >> File content/public/browser/****notification_types.h (right):
> > > > > >> >>
> > > > > >> >
> > > > > >> >
> > > > > >> > http://codereview.chromium.****org/8505047/diff/10002/**
> > > > > >> >
> > > > > >>
> > > > > >
> > > > > > content/public/browser/****notification_types.h#****newcode356<
> > > > > > http://codereview.**chromium.org/8505047/diff/**
> > > > > >
> > > > >
> > > >
> > >
> >
>
10002/content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> > > > > > >
> > > > > >
> > > > > >> >
> > > > > >> >> content/public/browser/****notification_types.h:356:
> > > > > >> >> NOTIFICATION_RENDER_VIEW_HOST_****RECEIVED_ON_MSG_SHOULD_**
> > > > > >> CLOSE_**ACK,
> > > > > >>
> > > > > >> >> notifications, in general, are a hacky way of getting some code
to
> > > call
> > > > > >> >>
> > > > > >> > another
> > > > > >> >
> > > > > >> >> part of code. in this case, it might be used by a test only, but
> > then
> > > > > >> >> later
> > > > > >> >> others might use it as well and refactoring that code which
sends
> it
> > > > > >> >> becomes
> > > > > >> >> complicated.
> > > > > >> >>
> > > > > >> >
> > > > > >> >  are there are no other existing notifications or callbacks that
> you
> > > can
> > > > > >> >> reuse
> > > > > >> >> here?
> > > > > >> >>
> > > > > >> >
> > > > > >> > John,
> > > > > >> >
> > > > > >> > There are no other notifications I can use as far as I can tell.
I
> > > > > >> looked
> > > > > >> > around
> > > > > >> > quite a bit.
> > > > > >> >
> > > > > >> > Thanks,
> > > > > >> > Prasad
> > > > > >> >
> > > > > >>
> > > > > >
> > > > > >  I don't have this code in my head, and looking in detail to
> investigate
> > > > > >> alternatives would take a bit of time. What I don't understand is
why
> > > this
> > > > > >> panel code is so different from the other ways we have html content
> in
> > > > > >> windows (i.e. popups, notifications etc) that it needs a new
> > notification
> > > > > >> to test it. Please try harder to find a way without adding a
> > > notification.
> > > > > >>  From experience, notifications really get in the way of
refactoring.
> > > > > >>
> > > > > >
> > > > > > John,
> > > > > >
> > > > > > The only other way that I know is to introduce a Platform::Sleep()
and
> > > then
> > > > > > check for count of panels. And that's what some of the other tests
> that
> > > > > > handle
> > > > > > this event are doing. They do a Platform::Sleep() for 1 second.
Using
> > > > > > Sleep()
> > > > > > tends to be flaky or slow depending on what you set it to.
> > > > > >
> > > > > > I had this coded with a Sleep() but then did a fair amount of work
to
> > dig
> > > > > > through and figure out the right place to throw this notification so
I
> > can
> > > > > > avoid
> > > > > > the Sleep() and I didn't see an existing notification that I can use
> > > > > > that'll
> > > > > > serve my purpose.
> > > > > >
> > > > > > I thought notifications are a fairly standard way of doing unit
> testing
> > in
> > > > > > Chrome. Would you rather have the Sleep() in test code?
> > > > > >
> > > > > 
> > > > > Sleep is definitely not good, as it's very flaky and slow as you point
> > > > > out. Notifications
> > > > > being used in tests to hook in random places is not standard or
> > > > > recommended, but it's unfortunately done in some places. This makes it
> > > > > really hard to refactor code.
> > > > > 
> > > > > Looking at this a little more: if I'm understanding this correctly,
you
> > > > > want to get notified that the onbeforeunload has run. So why not have
> your
> > > > > tests's onbeforeunload do a setTimeout with a 0 timeout, and that can
> > > > > change the title as a signal to the test. then you can use
> > > > > ui_test_utils::TitleWatcher.
> > > > 
> > > > That would be too early. The title would've changed before the close
> confirm
> > > > dialog comes up. 
> > > 
> > > Are you sure, i.e have you tested this?
> > 
> > Yes I tested this. Go to
http://www.corp.google.com/%257Eprasadt/helloworld.html
> and
> > then try to close it.
> > 
> > You'll see the title change while the dialog is still up.
> > 
> > > If I do a setTimeout in a beforeunload
> > > handler then it doesn't get called until after the dialog is closed. if
the
> > > timeout function changes the title, that'll be an async message which will
> > also
> > > be received after OnMsgShouldCloseACK.
> > 
> > Are you proposing using a timeout? There is a lag between the dialog being
> > closed and OnMsgShouldCloseACK getting called, so the timeout will need to
be
> > large enough. Using a a timeout may have the same issues as
Platform::Sleep().
> > 
> > Is my choice of name for the notification part of the issue? Would a better
> name
> > in any way help with your refactoring concerns?
> > 
> > > > Also, I need to wait for RenderViewHost::OnMsgShouldCloseACK to
> > > > run before I can try to close the window again, which my test does.
> > Otherwise
> > > > onbeforeunload handler will not get fired.
> > > > 
> > > > There are other tests in Chrome that can profitable use this
notification
> > and
> > > > stop relying on Platform::Sleep as they're doing now.
> > > 
> > > if we can come up with a way to do this per above, then we can also switch
> > other
> > > tests to that.
> 
> Yes. Agreed. i.e if we can.
> 
> > > > 
> > > > > > Thanks,
> > > > > > Prasad
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.**org/8505047/%252525253Chttp://codereview.chromiu...>
> > > > > >

[jam, 2011-11-14 20:19:58]

On Mon, Nov 14, 2011 at 11:42 AM, <prasadt@chromium.org> wrote:

> On 2011/11/14 19:25:10, John Abd-El-Malek wrote:
>
>> On 2011/11/11 23:06:39, prasadt wrote:
>> > On 2011/11/11 22:40:54, John Abd-El-Malek wrote:
>> > > On Fri, Nov 11, 2011 at 2:26 PM, <mailto:prasadt@chromium.org> wrote:
>> > >
>> > > > On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
>> > > >
>> > > >  On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org>
>> wrote:
>> > > >>
>> > > >
>> > > >  > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
>> > > >> >
>> > > >> > http://codereview.chromium.******org/8505047/diff/10002/**
>> > > >> >
>> > > >>
>> > > >
>> > > > content/public/browser/******notification_types.h<http://**
>> > > >
codereview.chromium.org/****8505047/diff/10002/content/**<http://codereview.chromium.org/**8505047/diff/10002/content/**>
>> > > >
>> > >
>> >
>>
>
> public/browser/notification_****types.h<http://codereview.**
> chromium.org/8505047/diff/**10002/content/public/browser/**
>
notification_types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> >
>
>> > > > >
>> > > >
>> > > >> >
>> > > >> >> File content/public/browser/******notification_types.h (right):
>> > > >> >>
>> > > >> >
>> > > >> >
>> > > >> > http://codereview.chromium.******org/8505047/diff/10002/**
>> > > >> >
>> > > >>
>> > > >
>> > > > content/public/browser/******notification_types.h#******newcode356<
>> > > >
http://codereview.**chromium.**org/8505047/diff/**<http://chromium.org/850504...
>> > > >
>> > >
>> >
>>
>
> 10002/content/public/browser/****notification_types.h#****newcode356<
> http://codereview.**chromium.org/8505047/diff/**
>
10002/content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> >
>
>> > > > >
>> > > >
>> > > >> >
>> > > >> >> content/public/browser/******notification_types.h:356:
>> > > >> >> NOTIFICATION_RENDER_VIEW_HOST_******RECEIVED_ON_MSG_SHOULD_**
>> > > >> CLOSE_**ACK,
>> > > >>
>> > > >> >> notifications, in general, are a hacky way of getting some code
>> to
>> call
>> > > >> >>
>> > > >> > another
>> > > >> >
>> > > >> >> part of code. in this case, it might be used by a test only, but
>>
> then
>
>> > > >> >> later
>> > > >> >> others might use it as well and refactoring that code which
>> sends it
>> > > >> >> becomes
>> > > >> >> complicated.
>> > > >> >>
>> > > >> >
>> > > >> >  are there are no other existing notifications or callbacks that
>> you
>> can
>> > > >> >> reuse
>> > > >> >> here?
>> > > >> >>
>> > > >> >
>> > > >> > John,
>> > > >> >
>> > > >> > There are no other notifications I can use as far as I can tell.
>> I
>> > > >> looked
>> > > >> > around
>> > > >> > quite a bit.
>> > > >> >
>> > > >> > Thanks,
>> > > >> > Prasad
>> > > >> >
>> > > >>
>> > > >
>> > > >  I don't have this code in my head, and looking in detail to
>> investigate
>> > > >> alternatives would take a bit of time. What I don't understand is
>> why
>> this
>> > > >> panel code is so different from the other ways we have html
>> content in
>> > > >> windows (i.e. popups, notifications etc) that it needs a new
>>
> notification
>
>> > > >> to test it. Please try harder to find a way without adding a
>> notification.
>> > > >>  From experience, notifications really get in the way of
>> refactoring.
>> > > >>
>> > > >
>> > > > John,
>> > > >
>> > > > The only other way that I know is to introduce a Platform::Sleep()
>> and
>> then
>> > > > check for count of panels. And that's what some of the other tests
>> that
>> > > > handle
>> > > > this event are doing. They do a Platform::Sleep() for 1 second.
>> Using
>> > > > Sleep()
>> > > > tends to be flaky or slow depending on what you set it to.
>> > > >
>> > > > I had this coded with a Sleep() but then did a fair amount of work
>> to
>>
> dig
>
>> > > > through and figure out the right place to throw this notification
>> so I
>>
> can
>
>> > > > avoid
>> > > > the Sleep() and I didn't see an existing notification that I can use
>> > > > that'll
>> > > > serve my purpose.
>> > > >
>> > > > I thought notifications are a fairly standard way of doing unit
>> testing
>>
> in
>
>> > > > Chrome. Would you rather have the Sleep() in test code?
>> > > >
>> > >
>> > > Sleep is definitely not good, as it's very flaky and slow as you point
>> > > out. Notifications
>> > > being used in tests to hook in random places is not standard or
>> > > recommended, but it's unfortunately done in some places. This makes it
>> > > really hard to refactor code.
>> > >
>> > > Looking at this a little more: if I'm understanding this correctly,
>> you
>> > > want to get notified that the onbeforeunload has run. So why not have
>> your
>> > > tests's onbeforeunload do a setTimeout with a 0 timeout, and that can
>> > > change the title as a signal to the test. then you can use
>> > > ui_test_utils::TitleWatcher.
>> >
>> > That would be too early. The title would've changed before the close
>> confirm
>> > dialog comes up.
>>
>
>  Are you sure, i.e have you tested this?
>>
>
> Yes I tested this. Go to http://www.corp.google.com/~**
>
prasadt/helloworld.html<http://www.corp.google.com/~prasadt/helloworld.html>and
> then try to close it.
>
> You'll see the title change while the dialog is still up.


my suggestion was to use a setTimeout in the beforeUnload handler

>
>
>  If I do a setTimeout in a beforeunload
>> handler then it doesn't get called until after the dialog is closed. if
>> the
>> timeout function changes the title, that'll be an async message which will
>>
> also
>
>> be received after OnMsgShouldCloseACK.
>>
>
> Are you proposing using a timeout? There is a lag between the dialog being
> closed and OnMsgShouldCloseACK getting called, so the timeout will need to
> be
> large enough. Using a a timeout may have the same issues as
> Platform::Sleep().


i had suggested timeout of 0 so that we also avoid the issue of slow tests.
i suspect that it'll run in the renderer message loop after the call that
sends the MsgShouldCloseACK message

>
> Is my choice of name for the notification part of the issue? Would a
> better name
> in any way help with your refactoring concerns?
>
>
the issue is not the name, it's the fact that having lots of notifications
makes refactoring the code hard. i.e. someone comes along and wants to
break up the code or move it, and having notifications makes things harder.
it's much better if tests depend on things that will never change (i.e.
unload handlers) rather than where IPCs get dispatched, which might change
in the future. i realize it's much more convenient to the latter, and sorry
for dragging this on, but in the long run this is better.


>
>  > Also, I need to wait for RenderViewHost::**OnMsgShouldCloseACK to
>> > run before I can try to close the window again, which my test does.
>>
> Otherwise
>
>> > onbeforeunload handler will not get fired.
>> >
>> > There are other tests in Chrome that can profitable use this
>> notification
>>
> and
>
>> > stop relying on Platform::Sleep as they're doing now.
>>
>
>  if we can come up with a way to do this per above, then we can also switch
>>
> other
>
>> tests to that.
>> >
>> > > > Thanks,
>> > > > Prasad
>> > > >
>> > > >
>> > >
>> >
>>
>
> http://codereview.chromium.****org/8505047/%25253Chttp://code**
> review.chromium.org/8505047/ <http://codereview.chromium.org/8505047/>>
>
>> > > >
>>
>
>
>
>
http://codereview.chromium.**org/8505047/<http://codereview.chromium.org/8505...
>

[jam, 2011-11-14 20:24:06]

On Mon, Nov 14, 2011 at 12:15 PM, <prasadt@chromium.org> wrote:

> John,
>
> Here is the sequence of messages being traded between RenderViewHost and
> RenderView as part of the closing sequence:
>
> 1) RenderViewHost send ViewMsg_ShouldClose
> 2) RenderView sends ViewHostMsg_**RunBeforeUnloadConfirm and runs a
> nested message
> loop. So its waiting for a response.
> 3) RenderViewHost creates and runs the AppModalDialog and return after user
> handles.
> 4) RenderView now gets of the nested loop and sends back
> ViewHostMsg_ShouldClose_ACK message.
> 5) RenderViewHost as part of processing this message, resets its state
> where it
> can handle another onbeforeunload handler.
>
> What I need to do in my test is to ensure that step 5) happened. And I'm
> calling
> the notificaton with this particular name,
> NOTIFICATION_RENDER_VIEW_HOST_**RECEIVED_ON_MSG_SHOULD_CLOSE_**ACK,
> because it
> notifies that ViewHostMsg_ShouldClose_ACK is receiving. This seems
> consistent
> with the naming for other similar notifications.
>
> I hope this helps under the motivation for my approach and give you better
> context to suggest a more effective way to handle this.
>

This is why I'm suggesting doing a setTimeout in the beforeunloadhandler.
This will get dispatched in the renderer after the message loop runrolls.
This means that the  ViewHostMsg_ShouldClose_ACK  would have been sent
already. Since the IPC to switch the title will get sent after it, by the
time it's dispatched in the browser process and takes effect, 5 will have
been done already.

>
> Thanks,
> Prasad
>
>
> On 2011/11/14 19:43:01, prasadt wrote:
>
>> On 2011/11/14 19:42:14, prasadt wrote:
>> > On 2011/11/14 19:25:10, John Abd-El-Malek wrote:
>> > > On 2011/11/11 23:06:39, prasadt wrote:
>> > > > On 2011/11/11 22:40:54, John Abd-El-Malek wrote:
>> > > > > On Fri, Nov 11, 2011 at 2:26 PM, <mailto:prasadt@chromium.org>
>> wrote:
>> > > > >
>> > > > > > On 2011/11/11 21:55:30, John Abd-El-Malek wrote:
>> > > > > >
>> > > > > >  On Fri, Nov 11, 2011 at 1:28 PM, <mailto:prasadt@chromium.org>
>>
> wrote:
>
>> > > > > >>
>> > > > > >
>> > > > > >  > On 2011/11/11 21:19:19, John Abd-El-Malek wrote:
>> > > > > >> >
>> > > > > >> > http://codereview.chromium.******org/8505047/diff/10002/**
>> > > > > >> >
>> > > > > >>
>> > > > > >
>> > > > > > content/public/browser/******notification_types.h<http://**
>> > > > > >
codereview.chromium.org/****8505047/diff/10002/content/**<http://codereview.chromium.org/**8505047/diff/10002/content/**>
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
> public/browser/notification_****types.h<http://codereview.**
> chromium.org/8505047/diff/**10002/content/public/browser/**
>
notification_types.h<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h>
> >
>
>> > > > > > >
>> > > > > >
>> > > > > >> >
>> > > > > >> >> File content/public/browser/******notification_types.h
>> (right):
>> > > > > >> >>
>> > > > > >> >
>> > > > > >> >
>> > > > > >> > http://codereview.chromium.******org/8505047/diff/10002/**
>> > > > > >> >
>> > > > > >>
>> > > > > >
>> > > > > > content/public/browser/******notification_types.h#******
>> newcode356<
>> > > > > >
http://codereview.**chromium.**org/8505047/diff/**<http://chromium.org/850504...
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
> 10002/content/public/browser/****notification_types.h#****newcode356<
> http://codereview.**chromium.org/8505047/diff/**
>
10002/content/public/browser/**notification_types.h#**newcode356<http://codereview.chromium.org/8505047/diff/10002/content/public/browser/notification_types.h#newcode356>
> >
>
>> > > > > > >
>> > > > > >
>> > > > > >> >
>> > > > > >> >> content/public/browser/******notification_types.h:356:
>> > > > > >> >> NOTIFICATION_RENDER_VIEW_HOST_**
>> ****RECEIVED_ON_MSG_SHOULD_**
>> > > > > >> CLOSE_**ACK,
>> > > > > >>
>> > > > > >> >> notifications, in general, are a hacky way of getting some
>> code
>>
> to
>
>> > > call
>> > > > > >> >>
>> > > > > >> > another
>> > > > > >> >
>> > > > > >> >> part of code. in this case, it might be used by a test
>> only, but
>> > then
>> > > > > >> >> later
>> > > > > >> >> others might use it as well and refactoring that code which
>>
> sends
>
>> it
>> > > > > >> >> becomes
>> > > > > >> >> complicated.
>> > > > > >> >>
>> > > > > >> >
>> > > > > >> >  are there are no other existing notifications or callbacks
>> that
>> you
>> > > can
>> > > > > >> >> reuse
>> > > > > >> >> here?
>> > > > > >> >>
>> > > > > >> >
>> > > > > >> > John,
>> > > > > >> >
>> > > > > >> > There are no other notifications I can use as far as I can
>> tell.
>>
> I
>
>> > > > > >> looked
>> > > > > >> > around
>> > > > > >> > quite a bit.
>> > > > > >> >
>> > > > > >> > Thanks,
>> > > > > >> > Prasad
>> > > > > >> >
>> > > > > >>
>> > > > > >
>> > > > > >  I don't have this code in my head, and looking in detail to
>> investigate
>> > > > > >> alternatives would take a bit of time. What I don't understand
>> is
>>
> why
>
>> > > this
>> > > > > >> panel code is so different from the other ways we have html
>> content
>> in
>> > > > > >> windows (i.e. popups, notifications etc) that it needs a new
>> > notification
>> > > > > >> to test it. Please try harder to find a way without adding a
>> > > notification.
>> > > > > >>  From experience, notifications really get in the way of
>>
> refactoring.
>
>> > > > > >>
>> > > > > >
>> > > > > > John,
>> > > > > >
>> > > > > > The only other way that I know is to introduce a
>> Platform::Sleep()
>>
> and
>
>> > > then
>> > > > > > check for count of panels. And that's what some of the other
>> tests
>> that
>> > > > > > handle
>> > > > > > this event are doing. They do a Platform::Sleep() for 1 second.
>>
> Using
>
>> > > > > > Sleep()
>> > > > > > tends to be flaky or slow depending on what you set it to.
>> > > > > >
>> > > > > > I had this coded with a Sleep() but then did a fair amount of
>> work
>>
> to
>
>> > dig
>> > > > > > through and figure out the right place to throw this
>> notification so
>>
> I
>
>> > can
>> > > > > > avoid
>> > > > > > the Sleep() and I didn't see an existing notification that I
>> can use
>> > > > > > that'll
>> > > > > > serve my purpose.
>> > > > > >
>> > > > > > I thought notifications are a fairly standard way of doing unit
>> testing
>> > in
>> > > > > > Chrome. Would you rather have the Sleep() in test code?
>> > > > > >
>> > > > >
>> > > > > Sleep is definitely not good, as it's very flaky and slow as you
>> point
>> > > > > out. Notifications
>> > > > > being used in tests to hook in random places is not standard or
>> > > > > recommended, but it's unfortunately done in some places. This
>> makes it
>> > > > > really hard to refactor code.
>> > > > >
>> > > > > Looking at this a little more: if I'm understanding this
>> correctly,
>>
> you
>
>> > > > > want to get notified that the onbeforeunload has run. So why not
>> have
>> your
>> > > > > tests's onbeforeunload do a setTimeout with a 0 timeout, and that
>> can
>> > > > > change the title as a signal to the test. then you can use
>> > > > > ui_test_utils::TitleWatcher.
>> > > >
>> > > > That would be too early. The title would've changed before the close
>> confirm
>> > > > dialog comes up.
>> > >
>> > > Are you sure, i.e have you tested this?
>> >
>> > Yes I tested this. Go to
>>
>
http://www.corp.google.com/%**257Eprasadt/helloworld.html<http://www.corp.goo...
>
>  and
>> > then try to close it.
>> >
>> > You'll see the title change while the dialog is still up.
>> >
>> > > If I do a setTimeout in a beforeunload
>> > > handler then it doesn't get called until after the dialog is closed.
>> if
>>
> the
>
>> > > timeout function changes the title, that'll be an async message which
>> will
>> > also
>> > > be received after OnMsgShouldCloseACK.
>> >
>> > Are you proposing using a timeout? There is a lag between the dialog
>> being
>> > closed and OnMsgShouldCloseACK getting called, so the timeout will need
>> to
>>
> be
>
>> > large enough. Using a a timeout may have the same issues as
>>
> Platform::Sleep().
>
>> >
>> > Is my choice of name for the notification part of the issue? Would a
>> better
>> name
>> > in any way help with your refactoring concerns?
>> >
>> > > > Also, I need to wait for RenderViewHost::**OnMsgShouldCloseACK to
>> > > > run before I can try to close the window again, which my test does.
>> > Otherwise
>> > > > onbeforeunload handler will not get fired.
>> > > >
>> > > > There are other tests in Chrome that can profitable use this
>>
> notification
>
>> > and
>> > > > stop relying on Platform::Sleep as they're doing now.
>> > >
>> > > if we can come up with a way to do this per above, then we can also
>> switch
>> > other
>> > > tests to that.
>>
>
>  Yes. Agreed. i.e if we can.
>>
>
>  > > >
>> > > > > > Thanks,
>> > > > > > Prasad
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
> http://codereview.chromium.****org/8505047/%252525253Chttp://**
> codereview.chromium.org/**8505047/<http://codereview.chromium.org/8505047/>
> >
>
>> > > > > >
>>
>
>
>
>
http://codereview.chromium.**org/8505047/<http://codereview.chromium.org/8505...
>

[prasadt, 2011-11-14 21:50:13]

On 2011/11/14 20:24:06, John Abd-El-Malek wrote:
> On Mon, Nov 14, 2011 at 12:15 PM, <mailto:prasadt@chromium.org> wrote:
> 
> > John,
> >
> > Here is the sequence of messages being traded between RenderViewHost and
> > RenderView as part of the closing sequence:
> >
> > 1) RenderViewHost send ViewMsg_ShouldClose
> > 2) RenderView sends ViewHostMsg_**RunBeforeUnloadConfirm and runs a
> > nested message
> > loop. So its waiting for a response.
> > 3) RenderViewHost creates and runs the AppModalDialog and return after user
> > handles.
> > 4) RenderView now gets of the nested loop and sends back
> > ViewHostMsg_ShouldClose_ACK message.
> > 5) RenderViewHost as part of processing this message, resets its state
> > where it
> > can handle another onbeforeunload handler.
> >
> > What I need to do in my test is to ensure that step 5) happened. And I'm
> > calling
> > the notificaton with this particular name,
> > NOTIFICATION_RENDER_VIEW_HOST_**RECEIVED_ON_MSG_SHOULD_CLOSE_**ACK,
> > because it
> > notifies that ViewHostMsg_ShouldClose_ACK is receiving. This seems
> > consistent
> > with the naming for other similar notifications.
> >
> > I hope this helps under the motivation for my approach and give you better
> > context to suggest a more effective way to handle this.
> >
> 
> This is why I'm suggesting doing a setTimeout in the beforeunloadhandler.
> This will get dispatched in the renderer after the message loop runrolls.
> This means that the  ViewHostMsg_ShouldClose_ACK  would have been sent
> already. Since the IPC to switch the title will get sent after it, by the
> time it's dispatched in the browser process and takes effect, 5 will have
> been done already.

Ah. I missed setting title from setTimeout part. ok, my bad.

When I looked at it, I thought the timer should still get run before
ViewHostMsg_ShouldClose_ACK as the nested message loop should still process the
message. When I tried it, it seems to work right, so it looks like the timer
handler is not processed in the nested loop. Makes sense, as otherwise you could
have re-entrancy issues.

So, you're right in that renderer ViewHostMsg_ShouldClose_ACK should get
dispatched before the timeout handler. But since this involves IPC, there is no
guarantee that these will land in RenderViewHost in the same sequence, right? Or
is the order assured in this scenario somehow?

Thanks,
Prasad

[jam, 2011-11-15 00:04:43]

On Mon, Nov 14, 2011 at 1:50 PM, <prasadt@chromium.org> wrote:

> On 2011/11/14 20:24:06, John Abd-El-Malek wrote:
>
>  On Mon, Nov 14, 2011 at 12:15 PM, <mailto:prasadt@chromium.org> wrote:
>>
>
>  > John,
>> >
>> > Here is the sequence of messages being traded between RenderViewHost and
>> > RenderView as part of the closing sequence:
>> >
>> > 1) RenderViewHost send ViewMsg_ShouldClose
>> > 2) RenderView sends ViewHostMsg_****RunBeforeUnloadConfirm and runs a
>>
>> > nested message
>> > loop. So its waiting for a response.
>> > 3) RenderViewHost creates and runs the AppModalDialog and return after
>> user
>> > handles.
>> > 4) RenderView now gets of the nested loop and sends back
>> > ViewHostMsg_ShouldClose_ACK message.
>> > 5) RenderViewHost as part of processing this message, resets its state
>> > where it
>> > can handle another onbeforeunload handler.
>> >
>> > What I need to do in my test is to ensure that step 5) happened. And I'm
>> > calling
>> > the notificaton with this particular name,
>> > NOTIFICATION_RENDER_VIEW_HOST_****RECEIVED_ON_MSG_SHOULD_**CLOSE_**ACK,
>>
>> > because it
>> > notifies that ViewHostMsg_ShouldClose_ACK is receiving. This seems
>> > consistent
>> > with the naming for other similar notifications.
>> >
>> > I hope this helps under the motivation for my approach and give you
>> better
>> > context to suggest a more effective way to handle this.
>> >
>>
>
>  This is why I'm suggesting doing a setTimeout in the beforeunloadhandler.
>> This will get dispatched in the renderer after the message loop runrolls.
>> This means that the  ViewHostMsg_ShouldClose_ACK  would have been sent
>> already. Since the IPC to switch the title will get sent after it, by the
>> time it's dispatched in the browser process and takes effect, 5 will have
>> been done already.
>>
>
> Ah. I missed setting title from setTimeout part. ok, my bad.
>
> When I looked at it, I thought the timer should still get run before
> ViewHostMsg_ShouldClose_ACK as the nested message loop should still
> process the
> message. When I tried it, it seems to work right, so it looks like the
> timer
> handler is not processed in the nested loop. Makes sense, as otherwise you
> could
> have re-entrancy issues.
>
> So, you're right in that renderer ViewHostMsg_ShouldClose_ACK should get
> dispatched before the timeout handler. But since this involves IPC, there
> is no
> guarantee that these will land in RenderViewHost in the same sequence,
> right? Or
> is the order assured in this scenario somehow?
>

IPCs to the browser process' UI thread are all asynchronous, so they're
dispatched in the same order that they're sent. So the IPC that gets sent
to change the title would be guaranteed to be dispatched
after ViewHostMsg_ShouldClose_ACK.

>
> Thanks,
> Prasad
>
>
http://codereview.chromium.**org/8505047/<http://codereview.chromium.org/8505...
>

[prasadt, 2011-11-15 04:34:41]

On 2011/11/15 00:04:43, John Abd-El-Malek wrote:
> On Mon, Nov 14, 2011 at 1:50 PM, <mailto:prasadt@chromium.org> wrote:
> 
> > On 2011/11/14 20:24:06, John Abd-El-Malek wrote:
> >
> >  On Mon, Nov 14, 2011 at 12:15 PM, <mailto:prasadt@chromium.org> wrote:
> >>
> >
> >  > John,
> >> >
> >> > Here is the sequence of messages being traded between RenderViewHost and
> >> > RenderView as part of the closing sequence:
> >> >
> >> > 1) RenderViewHost send ViewMsg_ShouldClose
> >> > 2) RenderView sends ViewHostMsg_****RunBeforeUnloadConfirm and runs a
> >>
> >> > nested message
> >> > loop. So its waiting for a response.
> >> > 3) RenderViewHost creates and runs the AppModalDialog and return after
> >> user
> >> > handles.
> >> > 4) RenderView now gets of the nested loop and sends back
> >> > ViewHostMsg_ShouldClose_ACK message.
> >> > 5) RenderViewHost as part of processing this message, resets its state
> >> > where it
> >> > can handle another onbeforeunload handler.
> >> >
> >> > What I need to do in my test is to ensure that step 5) happened. And I'm
> >> > calling
> >> > the notificaton with this particular name,
> >> > NOTIFICATION_RENDER_VIEW_HOST_****RECEIVED_ON_MSG_SHOULD_**CLOSE_**ACK,
> >>
> >> > because it
> >> > notifies that ViewHostMsg_ShouldClose_ACK is receiving. This seems
> >> > consistent
> >> > with the naming for other similar notifications.
> >> >
> >> > I hope this helps under the motivation for my approach and give you
> >> better
> >> > context to suggest a more effective way to handle this.
> >> >
> >>
> >
> >  This is why I'm suggesting doing a setTimeout in the beforeunloadhandler.
> >> This will get dispatched in the renderer after the message loop runrolls.
> >> This means that the  ViewHostMsg_ShouldClose_ACK  would have been sent
> >> already. Since the IPC to switch the title will get sent after it, by the
> >> time it's dispatched in the browser process and takes effect, 5 will have
> >> been done already.
> >>
> >
> > Ah. I missed setting title from setTimeout part. ok, my bad.
> >
> > When I looked at it, I thought the timer should still get run before
> > ViewHostMsg_ShouldClose_ACK as the nested message loop should still
> > process the
> > message. When I tried it, it seems to work right, so it looks like the
> > timer
> > handler is not processed in the nested loop. Makes sense, as otherwise you
> > could
> > have re-entrancy issues.
> >
> > So, you're right in that renderer ViewHostMsg_ShouldClose_ACK should get
> > dispatched before the timeout handler. But since this involves IPC, there
> > is no
> > guarantee that these will land in RenderViewHost in the same sequence,
> > right? Or
> > is the order assured in this scenario somehow?
> >
> 
> IPCs to the browser process' UI thread are all asynchronous, so they're
> dispatched in the same order that they're sent. So the IPC that gets sent
> to change the title would be guaranteed to be dispatched
> after ViewHostMsg_ShouldClose_ACK.

I updated the test to use timeout. Please take a look.

[jam, 2011-11-15 06:24:24]

lgtm

[jennb, 2011-11-15 18:21:00]

LGTM

Nice test.