DevTools: Do not close devtools if there are dirty files in workspace

Devtools workspace feature means that a developer can lose
unsaved changes if the inspected page or browser gets closed.
This CL adds a warning dialog which is displayed in these cases by
devtools if the workspace is dirty.

The dialog is displayed by chaining a Devtools beforeunload 
handler before the content's beforeunload handler.
Plumbing is added to support said chaining.

For a more detailed overview, see comments in devtools_window.h

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

[vsevik, 2013-09-11 11:51:12]

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:309: return window ? window : NULL;
return window

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:827: CHECK(closing_target_ !=
DEVTOOLS_CLOSING_TARGET_NONE);
According to
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
you should not use CHECK here and below. Please use DCHECK.

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:912: void
DevToolsWindow::ActivateContents(content::WebContents* contents) {
I am not sure about this method, but why is it so different from the one above?

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:923: closing_target_ =
DEVTOOLS_CLOSING_TARGET_DEVTOOLS;
Looks like this field is redundant.

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:1266:
web_contents_->GetRenderViewHost()->ClosePage();
IS this correct in case of undocked devtools?
why don't you delete this?
why don't we inline this method now that there is only one usage?
Please check beforeunload does not happen here.

[lushnikov, 2013-09-11 14:03:09]

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:309: return window ? window : NULL;
On 2013/09/11 11:51:13, vsevik wrote:
> return window

Done.

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:827: CHECK(closing_target_ !=
DEVTOOLS_CLOSING_TARGET_NONE);
On 2013/09/11 11:51:13, vsevik wrote:
> According to
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> you should not use CHECK here and below. Please use DCHECK.

Done.

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:923: closing_target_ =
DEVTOOLS_CLOSING_TARGET_DEVTOOLS;
On 2013/09/11 11:51:13, vsevik wrote:
> Looks like this field is redundant.

Done.

https://codereview.chromium.org/23835007/diff/7001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_window.cc:1266:
web_contents_->GetRenderViewHost()->ClosePage();
On 2013/09/11 11:51:13, vsevik wrote:
> IS this correct in case of undocked devtools?
> why don't you delete this?
> why don't we inline this method now that there is only one usage?
> Please check beforeunload does not happen here.

The deletion of DevToolsWindow will happen in ::Observe method

[lushnikov, 2013-09-11 17:07:20]

PTAL

[lushnikov, 2013-10-28 13:26:44]

Vsevik, please take a look on this patch from the DevTools point of view

[vsevik, 2013-11-05 08:36:16]

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/de...
chrome/browser/devtools/devtools_window.cc:601: DevToolsWindow *window =
AsDevToolsWindow(contents->GetRenderViewHost());
remove this

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/de...
chrome/browser/devtools/devtools_window.cc:625: // Propogate to DevTools opened
on DevTools if any
propagate

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/ui...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/ui...
chrome/browser/ui/fast_unload_controller.cc:203:
DevToolsWindow::NeedToFireBeforeUnload(contents))
contents->NeedToFireBeforeUnload ||
DevToolsWindow::NeedToFireBeforeUnload(contents))

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/ui...
chrome/browser/ui/fast_unload_controller.cc:407: DevToolsWindow* devtools_window
=
Can we extract static method DevToolsWindow::PageCancelClose

[lushnikov, 2013-11-05 12:12:07]

Fixed nits

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/de...
chrome/browser/devtools/devtools_window.cc:601: DevToolsWindow *window =
AsDevToolsWindow(contents->GetRenderViewHost());
On 2013/11/05 08:36:17, vsevik wrote:
> remove this

Done.

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/de...
chrome/browser/devtools/devtools_window.cc:625: // Propogate to DevTools opened
on DevTools if any
On 2013/11/05 08:36:17, vsevik wrote:
> propagate

Done.

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/ui...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/ui...
chrome/browser/ui/fast_unload_controller.cc:203:
DevToolsWindow::NeedToFireBeforeUnload(contents))
On 2013/11/05 08:36:17, vsevik wrote:
> contents->NeedToFireBeforeUnload ||
> DevToolsWindow::NeedToFireBeforeUnload(contents))

Done.

https://chromiumcodereview.appspot.com/23835007/diff/424001/chrome/browser/ui...
chrome/browser/ui/fast_unload_controller.cc:407: DevToolsWindow* devtools_window
=
On 2013/11/05 08:36:17, vsevik wrote:
> Can we extract static method DevToolsWindow::PageCancelClose

Done.

[vsevik, 2013-11-05 12:23:29]

devtools lgtm

https://chromiumcodereview.appspot.com/23835007/diff/494001/chrome/browser/de...
File chrome/browser/devtools/devtools_window.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/494001/chrome/browser/de...
chrome/browser/devtools/devtools_window.cc:619: void
DevToolsWindow::PageCancelClose(content::WebContents* contents) {
PageClosingCanceled

[vsevik, 2013-11-05 12:44:16]

Let's make several tests with the following scenario:
1) Open certain type of devtools (docked/undocked)
2) Try to close inspected page / dev tools with a certain type of closing (tab,
window, etc.)
3) Cancel unloa din modla dialog
4) Try to close the same way again but accepting unload this time.

This will ensure test coverage of all possible cases.

[lushnikov, 2013-11-05 14:52:53]

@vsevik: improved test coverage

[lushnikov, 2013-11-05 14:58:11]

Here's an overview of what's going on
in this patch: http://goo.gl/dlHsSy

@playmobil: please, take a look on changes in fast_unload_controller. Does they
work for you?

@sky: this patch binds DevTools beforeunload event to inspected webcontents'
beforeunload. How do you feel regarding this change?

[sky, 2013-11-05 16:37:09]

https://chromiumcodereview.appspot.com/23835007/diff/514004/chrome/browser/ui...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/514004/chrome/browser/ui...
chrome/browser/ui/tabs/tab_strip_model.cc:1206: if
(DevToolsWindow::GetInstanceForInspectedRenderViewHost(
If you are going to tie the two together I think it better done else where, and
not explicitly in the tabstrip.

[lushnikov, 2013-11-05 17:27:21]

https://chromiumcodereview.appspot.com/23835007/diff/514004/chrome/browser/ui...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/514004/chrome/browser/ui...
chrome/browser/ui/tabs/tab_strip_model.cc:1206: if
(DevToolsWindow::GetInstanceForInspectedRenderViewHost(
On 2013/11/05 16:37:10, sky wrote:
> If you are going to tie the two together I think it better done else where,
and
> not explicitly in the tabstrip.

We don't really tie anything here. This part of tab strip attempts to close
webcontents via a fast path, which is fully managed by content layer, and which
we would like to skip for those webcontents which have devtools opened.
 
We would like to keep all of this logic in the chrome layer without
touching content, and I didn't find any better place in chrome-layer  to skip
this behaviour. I would appreciate any suggestions regarding this case!

[lushnikov, 2013-11-06 12:28:13]

@creis: please take a look on this patch as unload-process guru

[lushnikov, 2013-11-06 17:01:40]

https://chromiumcodereview.appspot.com/23835007/diff/514004/chrome/browser/ui...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/514004/chrome/browser/ui...
chrome/browser/ui/tabs/tab_strip_model.cc:1206: if
(DevToolsWindow::GetInstanceForInspectedRenderViewHost(
On 2013/11/05 17:27:22, lushnikov wrote:
> On 2013/11/05 16:37:10, sky wrote:
> > If you are going to tie the two together I think it better done else where,
> and
> > not explicitly in the tabstrip.
> 
> We don't really tie anything here. This part of tab strip attempts to close
> webcontents via a fast path, which is fully managed by content layer, and
which
> we would like to skip for those webcontents which have devtools opened.
>  
> We would like to keep all of this logic in the chrome layer without
> touching content, and I didn't find any better place in chrome-layer  to skip
> this behaviour. I would appreciate any suggestions regarding this case!

Removed explicit DevTools presence in tab_strip_model

[jeremy, 2013-11-07 14:18:33]

At a first glance the general approach looks fine, but this needs way more
comments.  A couple more general remarks:

* The meaning of FastShutdown() is unclear - how about something like
NeedsDevToolsBeforeUnloadWarmingDialog() or something else that clearly ties it
to DevTools?
* nit '.' at end of comments.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_sanity_browsertest.cc:218: "function() { return
'Foo'; });"));
Sorry for the dumb nit but would assigning e.returnValue be more correct here?

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_sanity_browsertest.cc:591: // try to close
second DevTools
nit:
Try to close second DevTools.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:601: // Handling devtools on devtools
case
nit - '.'

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:133: static bool
HandleBeforeUnload(content::WebContents*, bool, bool*);
This needs a comment, how about something like:

In order to preserve any edits the user may have made in devtools, the
beforeunload event is hooked - devtools gets the first shot at handling
beforeunload and presents a dialog to the user, if the user accepts the dialog
then the script is given a chance to handle it i.e. 2 dialogs may be displayed:
one from the devtools asking the user to confirm that they're ok with their
edits going away and another from the webpage as the result of it's beforeunload
handler.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:133: static bool
HandleBeforeUnload(content::WebContents*, bool, bool*);
Why no variable names in header?  Also, these functions need comments...

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:75: content::WebContents* contents)
{
Can you please provide a comment on what fastshutdown means exactly and point to
it from the various delegates.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:84: // then we would like to run its
beforeunload handlers first.
Please point to the comment that explains why.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:177: // inspected tab closing
will get -> gets

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:208:
DevToolsWindow::NeedToFireBeforeUnload(contents)))
This expression is already kind of crazy - can we split this out into a boolean
and then just check that boolean inside the if?

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:340: if
(!DevToolsWindow::InterceptPageBeforeUnload(contents))
I don't really think this method name conveys what is happening, but I don't
have any ideas for a better name, sorry :(

This needs a comment though, something like:

First let Devtools put up a warning dialog if needed, then give the renderview a
chance to put up a dialog.  See ... for details.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:412:
DevToolsWindow::PageClosingCanceled(tab_needing_before_unload_ack_);
*OnPageCloseCanceled() ?

[lushnikov, 2013-11-07 17:18:09]

Jeremy, thank you for your review!

Talking about "FastShutdown" naming and all these "CanFastShutdown.." plumbing
methods - they are called this way so that tab_strip_model could abstract from
embedder,
and changing name to something-with-DevTools will probably break the whole point
of it. If I got it right, @sky really wanted me to avoid use of word "DevTools"
here.


>>* The meaning of FastShutdown() is unclear - how about something like
NeedsDevToolsBeforeUnloadWarmingDialog() or something else that clearly ties it
to DevTools?

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_sanity_browsertest.cc:218: "function() { return
'Foo'; });"));
On 2013/11/07 14:18:34, jeremy wrote:
> Sorry for the dumb nit but would assigning e.returnValue be more correct here?

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_sanity_browsertest.cc:591: // try to close
second DevTools
On 2013/11/07 14:18:34, jeremy wrote:
> nit:
> Try to close second DevTools.

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:601: // Handling devtools on devtools
case
On 2013/11/07 14:18:34, jeremy wrote:
> nit - '.'

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:133: static bool
HandleBeforeUnload(content::WebContents*, bool, bool*);
On 2013/11/07 14:18:34, jeremy wrote:
> This needs a comment, how about something like:
> 
> In order to preserve any edits the user may have made in devtools, the
> beforeunload event is hooked - devtools gets the first shot at handling
> beforeunload and presents a dialog to the user, if the user accepts the dialog
> then the script is given a chance to handle it i.e. 2 dialogs may be
displayed:
> one from the devtools asking the user to confirm that they're ok with their
> edits going away and another from the webpage as the result of it's
beforeunload
> handler.

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:133: static bool
HandleBeforeUnload(content::WebContents*, bool, bool*);
On 2013/11/07 14:18:34, jeremy wrote:
> Why no variable names in header?  Also, these functions need comments...

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:75: content::WebContents* contents)
{
On 2013/11/07 14:18:34, jeremy wrote:
> Can you please provide a comment on what fastshutdown means exactly and point
to
> it from the various delegates.

Do you think this one is needed? Looks like it could be easily figured out from
the code - there's only a single call of this method, which brings us right to
the fast shutdown code in content layer.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:84: // then we would like to run its
beforeunload handlers first.
On 2013/11/07 14:18:34, jeremy wrote:
> Please point to the comment that explains why.

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:177: // inspected tab closing
On 2013/11/07 14:18:34, jeremy wrote:
> will get -> gets

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:208:
DevToolsWindow::NeedToFireBeforeUnload(contents)))
On 2013/11/07 14:18:34, jeremy wrote:
> This expression is already kind of crazy - can we split this out into a
boolean
> and then just check that boolean inside the if?

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:340: if
(!DevToolsWindow::InterceptPageBeforeUnload(contents))
On 2013/11/07 14:18:34, jeremy wrote:
> I don't really think this method name conveys what is happening, but I don't
> have any ideas for a better name, sorry :(
> 
> This needs a comment though, something like:
> 
> First let Devtools put up a warning dialog if needed, then give the renderview
a
> chance to put up a dialog.  See ... for details.

Done.

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:412:
DevToolsWindow::PageClosingCanceled(tab_needing_before_unload_ack_);
On 2013/11/07 14:18:34, jeremy wrote:
> *OnPageCloseCanceled() ?

Done.

[jeremy, 2013-11-10 12:50:28]

Sorry for bugging you about comments and naming but I'm trying to get this to a
point where it's easier to review.

Can you please also rewrite the commit message to be a little more clear,
something along the lines of:

"""
Devtools workspace feature means that a developer can lose unsaved changes if a
page is reloaded.  This CL adds a warning dialog which is displayed by devtools
if the workspace is dirty.

The dialog is displayed by chaining a Devtools beforeunload handler before the
content's beforeunload handler.  Plumbing is added to support said chaining.

For a more detailed overview, see: ...
"""

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:75: content::WebContents* contents)
{
There are way too many moving parts here and for someone unfamiliar with the
code this is very hard to follow.   Having a descriptive method name helps
reduce the cognitive load when reading through the code.

I'm sure we can find a generic term which has nothing to do with DevTools but
conveys what's going on, how about something like BeforeUnloadInterceptNeeded()
or somesunch?

https://codereview.chromium.org/23835007/diff/774001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:85: if
(DevToolsWindow::InterceptPageBeforeUnload(contents)) {
2 things are unclear from this callsite:
* Does InterceptPageBeforeUnload() actually display the dialog?
* Is this a sync or an async operation?

Suggest changing the function name and adding a comment to make this clear.

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_sanity_browsertest.cc:536: // we try to close
inspected page.
nit: "the inspected page", and the same fix on other comments.

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:137: static bool
HandleBeforeUnload(content::WebContents* contents,
What's missing from the comments in these functions is information about the
flow of a beforeunload event, how is it passed through these functions? 

I think that most of the comment for InterceptPageBeforeUnload() should be moved
up to a comment that prefaces these functions and explains things in more
detail.

Variables also need comments.

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:149: // beforeunload events for
inspected page.
I find this and the above function name a bit generic in that it's not clear
what part they play in the flow.  I'd vote for more descriptive names (sorry, no
suggestions at the moment).

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:157: static bool
NeedToFireBeforeUnload(content::WebContents* contents);
Again, method name doesn't convey what's going on here.

NeedsToInterceptBeforeUnload()? HasBeforeUnloadHanlder()?

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:160: // by used.
used -> user

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/ui/fast_u...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:206: bool fire_beforeunload =
contents->NeedToFireBeforeUnload() ||
Thanks! 
nit: how about calling this should_fire_beforeunload?

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/ui/unload...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/ui/unload...
chrome/browser/ui/unload_controller.cc:166: bool fire_beforeunload =
contents->NeedToFireBeforeUnload() ||
nit: Again - should_fire...

[lushnikov, 2013-11-11 15:22:48]

>I'm sure we can find a generic term which has nothing to do with DevTools but
>conveys what's going on, how about something like BeforeUnloadInterceptNeeded()
>or somesunch?

In idea to make the relation between this method and
TabStripModelDelegate::RunUnloadListenerBeforeClosing more obvious, renamed
TabStripModelDelegate::CanFastShutdownWebContents ->
TabStripModel::ShouldRunUnloadListenerBeforeClosing

In Browser.h and unload controllers, renamed CanFastShutdownWebContents into
ShouldRunUnloadEventsHelper to align with Browser::RunUnloadEventsHelper method.

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_sanity_browsertest.cc:536: // we try to close
inspected page.
On 2013/11/10 12:50:30, jeremy wrote:
> nit: "the inspected page", and the same fix on other comments.

Done.

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:137: static bool
HandleBeforeUnload(content::WebContents* contents,
On 2013/11/10 12:50:30, jeremy wrote:
> What's missing from the comments in these functions is information about the
> flow of a beforeunload event, how is it passed through these functions? 
> 
> I think that most of the comment for InterceptPageBeforeUnload() should be
moved
> up to a comment that prefaces these functions and explains things in more
> detail.
Done. 

> Variables also need comments.
Arguments are exactly the same as they are in |BeforeUnloadFired| callback; I
updated the comment so that (hopefully) it will be more clear.

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:149: // beforeunload events for
inspected page.
On 2013/11/10 12:50:30, jeremy wrote:
> I find this and the above function name a bit generic in that it's not clear
> what part they play in the flow.  I'd vote for more descriptive names (sorry,
no
> suggestions at the moment).

Do the additional comments bring some light on motivation behind this naming?

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:157: static bool
NeedToFireBeforeUnload(content::WebContents* contents);
On 2013/11/10 12:50:30, jeremy wrote:
> Again, method name doesn't convey what's going on here.
> 
> NeedsToInterceptBeforeUnload()? HasBeforeUnloadHanlder()?

I pick "NeedsToInterceptBeforeUnload" :)

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:160: // by used.
On 2013/11/10 12:50:30, jeremy wrote:
> used -> user

Done.

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/ui/fast_u...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/ui/fast_u...
chrome/browser/ui/fast_unload_controller.cc:206: bool fire_beforeunload =
contents->NeedToFireBeforeUnload() ||
On 2013/11/10 12:50:30, jeremy wrote:
> Thanks! 
> nit: how about calling this should_fire_beforeunload?

Done.

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/ui/unload...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/844001/chrome/browser/ui/unload...
chrome/browser/ui/unload_controller.cc:166: bool fire_beforeunload =
contents->NeedToFireBeforeUnload() ||
On 2013/11/10 12:50:30, jeremy wrote:
> nit: Again - should_fire...

Done.

[jeremy, 2013-11-12 10:52:46]

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_sanity_browsertest.cc:165: void Wait();
nit: consider renaming to SpinMessageLoop()

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:133: // BeforeUnload handling
////////////////////////////////////////////////////
nit: beforeunload interception ?

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:143: // DevTools window. When the
|contents| with DevTools opened on them are
nit: "the devtools", also capitalization of devtools should be the smae
throughout comments - again, minor nit.

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:148: //    which by the means of
|HandleBeforeUnload| helper method always hits
I'm not clear on the flow here - InterceptPageBeforeUnload() fires the
beforeunload event for the Devtools window itself, the synthesized beforeunload
event for DevTools window is then handled by HandleBeforeUnload() ?

I'm also not clear on everything after the comma.

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:155: //     close.
This is completely invisible from the web page's perspective, right?  the web
page itself never sees the beforeunload event in this scenario?

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:157: //     event on |contents|.
add - "and everything proceeds as it normally would without the Devtools
interception".

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:158: // 4. Whenever delegate of the
|contents| gets |BeforeUnloadFired| callback
"If the user cancels the dialog put up by the WebContents, then |contents|'s
|BeforeUnloadFired| callback is called with the proceed argument set to false,
this causes |DevToolsWindow::OnPageCloseCancelled| to be called."

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:163: // its frontend. Instead, an
instance of browser does, and thus
*"does" -> "is set as the delegate"

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:169: // Returns true if routing was
successful.
Comment needs to document parameters and return value.

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:176: // flow itself. In this case
clients should not fire beforeunload
"manage beforeunload event flow itself" -> "intercept beforeunload events as
described above".

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:180: // Returns true if DevTools
browser window could be closed.
I assume this function returns true if there are no pending user edits and the
devtools window can be closed immediately? Can you rewrite the comment to that
effect?

https://codereview.chromium.org/23835007/diff/1124001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:191: void
SetDockSideForTest(DevToolsDockSide dock_side);
Thank you for writing up these comments, they make things much much clearer!!!

[lushnikov, 2013-11-12 13:37:36]

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_sanity_browsertest.cc:165: void Wait();
On 2013/11/12 10:52:46, jeremy wrote:
> nit: consider renaming to SpinMessageLoop()

This is pretty similar to content::WindowedNotificationObserver, thus the naming
is implied.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
File chrome/browser/devtools/devtools_window.h (right):

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:133: // BeforeUnload handling
////////////////////////////////////////////////////
On 2013/11/12 10:52:46, jeremy wrote:
> nit: beforeunload interception ?

Done.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:143: // DevTools window. When the
|contents| with DevTools opened on them are
On 2013/11/12 10:52:46, jeremy wrote:
> nit: "the devtools", also capitalization of devtools should be the smae
> throughout comments - again, minor nit.

Done.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:148: //    which by the means of
|HandleBeforeUnload| helper method always hits
On 2013/11/12 10:52:46, jeremy wrote:
> I'm not clear on the flow here - InterceptPageBeforeUnload() fires the
> beforeunload event for the Devtools window itself, the synthesized
beforeunload
> event for DevTools window is then handled by HandleBeforeUnload() ?
> 
> I'm also not clear on everything after the comma.

Rewrote the comment with more details; should look much better now.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:155: //     close.
On 2013/11/12 10:52:46, jeremy wrote:
> This is completely invisible from the web page's perspective, right?  the web
> page itself never sees the beforeunload event in this scenario?

That's right.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:157: //     event on |contents|.
On 2013/11/12 10:52:46, jeremy wrote:
> add - "and everything proceeds as it normally would without the Devtools
> interception".

Done.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:158: // 4. Whenever delegate of the
|contents| gets |BeforeUnloadFired| callback
On 2013/11/12 10:52:46, jeremy wrote:
> "If the user cancels the dialog put up by the WebContents, then |contents|'s
> |BeforeUnloadFired| callback is called with the proceed argument set to false,
> this causes |DevToolsWindow::OnPageCloseCancelled| to be called."

Slightly corrected detail; Done.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:163: // its frontend. Instead, an
instance of browser does, and thus
On 2013/11/12 10:52:46, jeremy wrote:
> *"does" -> "is set as the delegate"

Done.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:169: // Returns true if routing was
successful.
On 2013/11/12 10:52:46, jeremy wrote:
> Comment needs to document parameters and return value.

Done.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:176: // flow itself. In this case
clients should not fire beforeunload
On 2013/11/12 10:52:46, jeremy wrote:
> "manage beforeunload event flow itself" -> "intercept beforeunload events as
> described above".

Done.

https://chromiumcodereview.appspot.com/23835007/diff/1124001/chrome/browser/d...
chrome/browser/devtools/devtools_window.h:180: // Returns true if DevTools
browser window could be closed.
On 2013/11/12 10:52:46, jeremy wrote:
> I assume this function returns true if there are no pending user edits and the
> devtools window can be closed immediately? Can you rewrite the comment to that
> effect?

I renamed this method and put a more descriptive comment to avoid the confusion,
thanks.

[jeremy, 2013-11-13 10:52:33]

Thank you for your patience explaining this all to me!

Suggestions for comments inline.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:605: // Handling devtools on devtools
case.
// Handle case of DevTools inspecting another DevTools instance by passing the
call down to the DevTools instance being inspected.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:625: if
(browser->tab_strip_model()->empty())
// When FastUnloadController is used, we may detached from the browser window at
this point which means we've already fired beforeunload.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:884: if (!inspected_page_is_closing_)
{
// Docked devtools window closed directly.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:890: } else {
Closing as result of event from devtools content.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:140: // user to confirm that they're
ok with their edits going away and another
*devtools edits

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:146: //    method
|DevToolsWindow::InterceptPageBeforeUnload| gets called.
How about something like:
1. |DevToolsWindow::InterceptPageBeforeUnload| is called by |contents| and
indicates whether DevTools needs to intercept the beforeunload event.

If InterceptPageBeforeUnload() returns true then:
<If it returns false, I assume none of the below functions get called? Even
OnPageCloseCancelled?>

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:150: //    argument describing user
decision if he wants to close DevTools.
remove "with the boolean" till end of sentence

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:156: //    in
|DevToolsWindow::BeforeUnloadFired| as well.
For the rest of this, how about something like:
If Devtools is docked then it's set as the window delegate so
|DevToolsWindow::BeforeUnloadFired is called directly|.

If DevToos is undocked it's not set as the delegate then the call to
BeforeUnloadFired is proxied through HandleBeforeUnload() rather than getting
called directly.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:162: //     close.
If |DevToolsWindow::BeforeUnloadFired| is called with proceed=false it calls
through to the content's BeforeUnloadFired() dialog, which from the WebContents
perspective looks the same as the Content's own beforeunload dialog having had
it's 'cancel' button clicked.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:183: // event and if it will manage
unload process on its own.
* |proceed| - true if the user clicked 'ok' in the beforeunload dialog, false
otherwise.
* |proceed_to_fire_unload| - On output, whether we should continue to fire the
unload event or stop things here.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:191: // event on |contents| themselfes
as DevTools window will take care of it.
//  Sends a beforeunload event to the Devtools frontend if needed, returns true
if the content's event was intercepted by devtools false if not and the caller
should continue handling the event as usual.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/fast_...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:75: content::WebContents* contents)
{
// If |contents| is being inspected, DevTools needs to intercept beforeunload
events.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:146: if (browser_->is_devtools() &&
// Special case for when we quit an application.  The Devtools window can close
if it's beforeunload event has already fired which will happen due to the
interception of it's content's beforeunload.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:179: // inspected tab closing.
// The DevTools browser gets its beforeunload events as the results of
intercepting events from the inspected tab, so don't send them here as well.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/unloa...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/unloa...
chrome/browser/ui/unload_controller.cc:110: if (browser_->is_devtools() &&
Same comment as in FastUnloadController

[lushnikov, 2013-11-13 13:39:07]

Thank you Jeremy for the time you've spent on reviewing this!

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:605: // Handling devtools on devtools
case.
On 2013/11/13 10:52:34, jeremy wrote:
> // Handle case of DevTools inspecting another DevTools instance by passing the
> call down to the DevTools instance being inspected.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:625: if
(browser->tab_strip_model()->empty())
On 2013/11/13 10:52:34, jeremy wrote:
> // When FastUnloadController is used, we may detached from the browser window
at
> this point which means we've already fired beforeunload.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:884: if (!inspected_page_is_closing_)
{
On 2013/11/13 10:52:34, jeremy wrote:
> // Docked devtools window closed directly.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:890: } else {
On 2013/11/13 10:52:34, jeremy wrote:
> Closing as result of event from devtools content.

Reworded, done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:140: // user to confirm that they're
ok with their edits going away and another
On 2013/11/13 10:52:34, jeremy wrote:
> *devtools edits

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:146: //    method
|DevToolsWindow::InterceptPageBeforeUnload| gets called.
On 2013/11/13 10:52:34, jeremy wrote:
> How about something like:
> 1. |DevToolsWindow::InterceptPageBeforeUnload| is called by |contents| and
> indicates whether DevTools needs to intercept the beforeunload event.
> 
> If InterceptPageBeforeUnload() returns true then:
> <If it returns false, I assume none of the below functions get called? Even
> OnPageCloseCancelled?>

Reworded and put smth like this

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:150: //    argument describing user
decision if he wants to close DevTools.
On 2013/11/13 10:52:34, jeremy wrote:
> remove "with the boolean" till end of sentence

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:156: //    in
|DevToolsWindow::BeforeUnloadFired| as well.
On 2013/11/13 10:52:34, jeremy wrote:
> For the rest of this, how about something like:
> If Devtools is docked then it's set as the window delegate so
> |DevToolsWindow::BeforeUnloadFired is called directly|.
> 
> If DevToos is undocked it's not set as the delegate then the call to
> BeforeUnloadFired is proxied through HandleBeforeUnload() rather than getting
> called directly.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:162: //     close.
On 2013/11/13 10:52:34, jeremy wrote:
> If |DevToolsWindow::BeforeUnloadFired| is called with proceed=false it calls
> through to the content's BeforeUnloadFired() dialog, which from the
WebContents
> perspective looks the same as the Content's own beforeunload dialog having had
> it's 'cancel' button clicked.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:183: // event and if it will manage
unload process on its own.
On 2013/11/13 10:52:34, jeremy wrote:
> * |proceed| - true if the user clicked 'ok' in the beforeunload dialog, false
> otherwise.
> * |proceed_to_fire_unload| - On output, whether we should continue to fire the
> unload event or stop things here.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:191: // event on |contents| themselfes
as DevTools window will take care of it.
On 2013/11/13 10:52:34, jeremy wrote:
> //  Sends a beforeunload event to the Devtools frontend if needed, returns
true
> if the content's event was intercepted by devtools false if not and the caller
> should continue handling the event as usual.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/fast_...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:75: content::WebContents* contents)
{
On 2013/11/13 10:52:34, jeremy wrote:
> // If |contents| is being inspected, DevTools needs to intercept beforeunload
> events.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:146: if (browser_->is_devtools() &&
On 2013/11/13 10:52:34, jeremy wrote:
> // Special case for when we quit an application.  The Devtools window can
close
> if it's beforeunload event has already fired which will happen due to the
> interception of it's content's beforeunload.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:179: // inspected tab closing.
On 2013/11/13 10:52:34, jeremy wrote:
> // The DevTools browser gets its beforeunload events as the results of
> intercepting events from the inspected tab, so don't send them here as well.

Done.

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/unloa...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/1194001/chrome/browser/ui/unloa...
chrome/browser/ui/unload_controller.cc:110: if (browser_->is_devtools() &&
On 2013/11/13 10:52:34, jeremy wrote:
> Same comment as in FastUnloadController

Done.

[jeremy, 2013-11-14 08:53:13]

LGTM with comments.

The new special cases in UnloadController strike me as very unfortunate, but I'm
not sure there's a better way to do this :/

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_sanity_browsertest.cc:626: // Try to exit
application.
This bit tests the special case in ShouldCloseWindow(), right?

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:140: // user to confirm that they're
ok with their devtools edits going away and
nit: devtools -> DevTools

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:148: //    will take place; otherwise
only step 4 will be reached.
reached -> reached and none of the corresponding functions in steps 2 & 3 will
get called.

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:162: // 3b. If
|DevToolsWindow::BeforeUnloadFired| was called with |proceed|
Replace everything up to ',' with:
If |proceed| = true, then it fires ...

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:360: bool inspected_page_is_closing_;
Can you add a comment explaining what this does?

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/ui/unloa...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/ui/unloa...
chrome/browser/ui/unload_controller.cc:273: // we would like devtools to call
its own beforeunload handlers first,
nit: devtools capitalization here and throughout *unload_controller files.

[lushnikov, 2013-11-14 09:49:37]

Thank you for your patience!

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_sanity_browsertest.cc:626: // Try to exit
application.
On 2013/11/14 08:53:13, jeremy wrote:
> This bit tests the special case in ShouldCloseWindow(), right?

Exactly!

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:140: // user to confirm that they're
ok with their devtools edits going away and
On 2013/11/14 08:53:13, jeremy wrote:
> nit: devtools -> DevTools

Converted everything to "devtools" instead as this seems to be the case in other
places

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:148: //    will take place; otherwise
only step 4 will be reached.
On 2013/11/14 08:53:13, jeremy wrote:
> reached -> reached and none of the corresponding functions in steps 2 & 3 will
> get called.

Done.

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:162: // 3b. If
|DevToolsWindow::BeforeUnloadFired| was called with |proceed|
On 2013/11/14 08:53:13, jeremy wrote:
> Replace everything up to ',' with:
> If |proceed| = true, then it fires ...

Done.

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:360: bool inspected_page_is_closing_;
On 2013/11/14 08:53:13, jeremy wrote:
> Can you add a comment explaining what this does?

Done.

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/ui/unloa...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/ui/unloa...
chrome/browser/ui/unload_controller.cc:273: // we would like devtools to call
its own beforeunload handlers first,
On 2013/11/14 08:53:13, jeremy wrote:
> nit: devtools capitalization here and throughout *unload_controller files.

Oops, just checked other usages of "devtools" word throughout the code - they
all are without capitalization. Fixed here and everywhere in my comments to
correspond with them.

[lushnikov, 2013-11-14 09:49:38]

Thank you for your patience!

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_sanity_browsertest.cc:626: // Try to exit
application.
On 2013/11/14 08:53:13, jeremy wrote:
> This bit tests the special case in ShouldCloseWindow(), right?

Exactly!

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:140: // user to confirm that they're
ok with their devtools edits going away and
On 2013/11/14 08:53:13, jeremy wrote:
> nit: devtools -> DevTools

Converted everything to "devtools" instead as this seems to be the case in other
places

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:148: //    will take place; otherwise
only step 4 will be reached.
On 2013/11/14 08:53:13, jeremy wrote:
> reached -> reached and none of the corresponding functions in steps 2 & 3 will
> get called.

Done.

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:162: // 3b. If
|DevToolsWindow::BeforeUnloadFired| was called with |proceed|
On 2013/11/14 08:53:13, jeremy wrote:
> Replace everything up to ',' with:
> If |proceed| = true, then it fires ...

Done.

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.h:360: bool inspected_page_is_closing_;
On 2013/11/14 08:53:13, jeremy wrote:
> Can you add a comment explaining what this does?

Done.

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/ui/unloa...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/23835007/diff/1284001/chrome/browser/ui/unloa...
chrome/browser/ui/unload_controller.cc:273: // we would like devtools to call
its own beforeunload handlers first,
On 2013/11/14 08:53:13, jeremy wrote:
> nit: devtools capitalization here and throughout *unload_controller files.

Oops, just checked other usages of "devtools" word throughout the code - they
all are without capitalization. Fixed here and everywhere in my comments to
correspond with them.

[sky, 2013-11-14 18:50:03]

LGTM

https://codereview.chromium.org/23835007/diff/1774001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/23835007/diff/1774001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:398: static bool
ShouldRunUnloadEventsHelper(content::WebContents* contents);
Why does this needs to be in browser (browser is huge)?

[lushnikov, 2013-11-14 22:29:47]

@sky, thanks for review!

https://codereview.chromium.org/23835007/diff/1774001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/23835007/diff/1774001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:398: static bool
ShouldRunUnloadEventsHelper(content::WebContents* contents);
On 2013/11/14 18:50:04, sky wrote:
> Why does this needs to be in browser (browser is huge)?

This is a complimentary method to |RunUnloadEventsHelper| (declared below).

[commit-bot: I haz the power, 2013-11-14 23:37:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23835007/1774001

[commit-bot: I haz the power, 2013-11-15 01:04:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23835007/1774001

[commit-bot: I haz the power, 2013-11-15 03:19:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23835007/1774001

[commit-bot: I haz the power, 2013-11-15 03:45:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23835007/1774001

[commit-bot: I haz the power, 2013-11-15 07:37:49]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-15 08:31:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23835007/1774001

[commit-bot: I haz the power, 2013-11-15 11:09:46]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 13:29:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23835007/1774001

[commit-bot: I haz the power, 2013-11-15 17:52:24]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 18:55:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23835007/1774001

[commit-bot: I haz the power, 2013-11-15 21:47:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23835007/1774001

[commit-bot: I haz the power, 2013-11-16 00:06:36]

Change committed as 235475