Close web contents modal dialogs on content load start

Reland "Close web contents modal dialogs on content load start"

Address the failure of some web contents modal dialogs to close when
interstitial WebUI is displayed by closing the dialogs on content load start.
Remove existing ad hoc support in dialogs for closing on page loading in favor
of a uniform approach.

I'm using load start as the trigger for dialog close as this seems to be the
point in time at which the user first perceives the page to be changing.

Notes on the print preview changes: with this change the dialog is closed when
load starts and the initiator tab gets removed via RemovePreviewDialog, so there
is no longer a need to handle NOTIFICATION_NAV_ENTRY_COMITTED for the initiator
tab.  The PrintViewManager::PrintPreviewDone() DCHECK is removed since now
PrintViewManager::RenderViewGone() may be invoked and print_preview_state_ reset
due to WebContents closure before the dialog is destroyed and PrintPreviewDone()
is invoked.

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

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

[Mike Wittman, 2013-06-20 18:11:50]

Hi Demetrios, can you take a look at the printing and web_modal parts of this
and make sure the changes make sense for the print preview dialog?

[dpapad, 2013-06-20 18:17:07]

On 2013/06/20 18:11:50, Mike Wittman wrote:
> Hi Demetrios, can you take a look at the printing and web_modal parts of this
> and make sure the changes make sense for the print preview dialog?

Hi Mike,

I have been working outside Chrome for more than 1 1/2 year now, so I am not the
right person for this. I suggest
contacting @thestig (he will probably know where to re-direct you).

[Mike Wittman, 2013-06-20 18:20:56]

Hi Lei, PTAL and/or redirect to someone who can.

[Lei Zhang, 2013-06-20 19:28:55]

It's been a while, but I can take a look after I get my M29 stuff done. No rush
on this right?

[Mike Wittman, 2013-06-20 19:46:40]

On 2013/06/20 19:28:55, Lei Zhang wrote:
> It's been a while, but I can take a look after I get my M29 stuff done. No
rush
> on this right?

No rush. This change should wait until after the branch point anyway.

[Mike Wittman, 2013-06-26 21:49:33]

ping

[Lei Zhang, 2013-06-27 00:31:18]

You probably want another reviewer for the non-printing bits. I'm not at all
familiar with that code.

Since you mentioned this is views-only, let me try to do a Linux + views build
and play with it a bit. Hopefully I won't have to go to my Windows machine.

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_preview_dialog_controller.cc (left):

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_preview_dialog_controller.cc:392:
RemoveInitiatorTab(contents);
Once we do this, then maybe we don't even need watch for
NOTIFICATION_NAV_ENTRY_COMMITTED for initiator tabs.

[Mike Wittman, 2013-06-27 00:58:11]

On 2013/06/27 00:31:18, Lei Zhang wrote:
> You probably want another reviewer for the non-printing bits. I'm not at all
> familiar with that code.

I'm planning to send to Ben for general review as long the print preview part
looks sane.

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_preview_dialog_controller.cc (left):

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_preview_dialog_controller.cc:392:
RemoveInitiatorTab(contents);
On 2013/06/27 00:31:18, Lei Zhang wrote:
> Once we do this, then maybe we don't even need watch for
> NOTIFICATION_NAV_ENTRY_COMMITTED for initiator tabs.

Yes, that's true. It would require a bit more complexity around
AddObservers/RemoveObservers which currently do not distinguish between tab
types for WebContents.

[Lei Zhang, 2013-06-27 02:28:45]

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_view_manager.cc (left):

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_view_manager.cc:156: DCHECK_NE(NOT_PREVIEWING,
print_preview_state_);
If I put this DCHECK back, I can't seem to trigger it, at least on Linux views.
That is, PrintPreviewDone() always gets called for me before RenderViewGone().
Is there a specific set of steps that reliably triggers this for you? I can try
those steps, or try on Windows.

[Lei Zhang, 2013-06-27 02:29:37]

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_preview_dialog_controller.cc (left):

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_preview_dialog_controller.cc:392:
RemoveInitiatorTab(contents);
On 2013/06/27 00:58:11, Mike Wittman wrote:
> On 2013/06/27 00:31:18, Lei Zhang wrote:
> > Once we do this, then maybe we don't even need watch for
> > NOTIFICATION_NAV_ENTRY_COMMITTED for initiator tabs.
> 
> Yes, that's true. It would require a bit more complexity around
> AddObservers/RemoveObservers which currently do not distinguish between tab
> types for WebContents.

Yes it would. If you don't want to, then just put a TODO(thestig) and I can take
care of it in the future.

[Mike Wittman, 2013-06-27 17:34:04]

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_preview_dialog_controller.cc (left):

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_preview_dialog_controller.cc:392:
RemoveInitiatorTab(contents);
On 2013/06/27 02:29:37, Lei Zhang wrote:
> On 2013/06/27 00:58:11, Mike Wittman wrote:
> > On 2013/06/27 00:31:18, Lei Zhang wrote:
> > > Once we do this, then maybe we don't even need watch for
> > > NOTIFICATION_NAV_ENTRY_COMMITTED for initiator tabs.
> > 
> > Yes, that's true. It would require a bit more complexity around
> > AddObservers/RemoveObservers which currently do not distinguish between tab
> > types for WebContents.
> 
> Yes it would. If you don't want to, then just put a TODO(thestig) and I can
take
> care of it in the future.

Not a problem, added the additional logic to not observe
NOTIFICATION_NAV_ENTRY_COMMITTED for the initiator tab.

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_view_manager.cc (left):

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_view_manager.cc:156: DCHECK_NE(NOT_PREVIEWING,
print_preview_state_);
On 2013/06/27 02:28:46, Lei Zhang wrote:
> If I put this DCHECK back, I can't seem to trigger it, at least on Linux
views.
> That is, PrintPreviewDone() always gets called for me before RenderViewGone().
> Is there a specific set of steps that reliably triggers this for you? I can
try
> those steps, or try on Windows.

I didn't experience this DCHECK firing while running Chrome, but it does so
reliably with the
PrintPreviewDialogControllerBrowserTest.NavigateFromInitiatorTab browser test.

[Lei Zhang, 2013-06-27 21:36:18]

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_view_manager.cc (left):

https://codereview.chromium.org/17500003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_view_manager.cc:156: DCHECK_NE(NOT_PREVIEWING,
print_preview_state_);
On 2013/06/27 17:34:04, Mike Wittman wrote:
> On 2013/06/27 02:28:46, Lei Zhang wrote:
> > If I put this DCHECK back, I can't seem to trigger it, at least on Linux
> views.
> > That is, PrintPreviewDone() always gets called for me before
RenderViewGone().
> > Is there a specific set of steps that reliably triggers this for you? I can
> try
> > those steps, or try on Windows.
> 
> I didn't experience this DCHECK firing while running Chrome, but it does so
> reliably with the
> PrintPreviewDialogControllerBrowserTest.NavigateFromInitiatorTab browser test.

No luck for me on Linux-views. I'll fire up the Windoze box and get back to
you...

https://codereview.chromium.org/17500003/diff/18001/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_dialog_controller.cc (right):

https://codereview.chromium.org/17500003/diff/18001/chrome/browser/printing/p...
chrome/browser/printing/print_preview_dialog_controller.cc:364: DCHECK(contents
== preview_dialog);
nit: DCHECK_EQ(preview_dialog, content);

https://codereview.chromium.org/17500003/diff/18001/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_dialog_controller.h (right):

https://codereview.chromium.org/17500003/diff/18001/chrome/browser/printing/p...
chrome/browser/printing/print_preview_dialog_controller.h:81: enum TabType {
nit: the preview is not actually a tab, so PREVIEW_DIALOG instead of PREVIEW_TAB
and ContentType instead of TabType?

[Mike Wittman, 2013-06-27 23:39:38]

https://codereview.chromium.org/17500003/diff/18001/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_dialog_controller.cc (right):

https://codereview.chromium.org/17500003/diff/18001/chrome/browser/printing/p...
chrome/browser/printing/print_preview_dialog_controller.cc:364: DCHECK(contents
== preview_dialog);
On 2013/06/27 21:36:18, Lei Zhang wrote:
> nit: DCHECK_EQ(preview_dialog, content);

Done.

https://codereview.chromium.org/17500003/diff/18001/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_dialog_controller.h (right):

https://codereview.chromium.org/17500003/diff/18001/chrome/browser/printing/p...
chrome/browser/printing/print_preview_dialog_controller.h:81: enum TabType {
On 2013/06/27 21:36:18, Lei Zhang wrote:
> nit: the preview is not actually a tab, so PREVIEW_DIALOG instead of
PREVIEW_TAB
> and ContentType instead of TabType?

Done.

With the web contents modal dialog work, the initiator also doesn't have to
correspond to a WebContents in a tab.  I've added another patchset with a mass
change of terminology from "initiator tab" to just "initiator." We can keep that
or kill it.

[Lei Zhang, 2013-06-28 19:53:57]

On 2013/06/27 21:36:18, Lei Zhang wrote:
> No luck for me on Linux-views. I'll fire up the Windoze box and get back to
> you...

Even on Windows, the test runs a bit inconsistently. It succeeds sometimes like
Linux-views, but more often, fails with that DCHECK. It looks like things can be
a bit racy due to when the print preview renderer sends messages to the browser.
I'll need to fix that in a separate CL.

[Lei Zhang, 2013-06-29 00:24:17]

Thanks for doing more cleanup!

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/b...
File chrome/browser/printing/background_printing_manager.cc (right):

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/b...
chrome/browser/printing/background_printing_manager.cc:72:
dialog_controller->GetInitiator(preview_dialog);
nit: fits on the previous line.

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_dialog_controller.cc (right):

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/p...
chrome/browser/printing/print_preview_dialog_controller.cc:100: initiator_ =
initiator;
Just put this in the initializer list? I don't know why we didn't to start with.

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_dialog_controller.h (right):

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/p...
chrome/browser/printing/print_preview_dialog_controller.h:25: // For print
preview, the tab that initiates the printing operation is the
If there's non-tabs that can be initiators, can you change this part about "the
tab" ?

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/ui/webui/p...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:161: const char
kInitiatorTitle[] = "initiatorTabTitle";
Change the value and the usage of the value in
chrome/browser/resources/print_preview/native_layer.js too?

[Mike Wittman, 2013-07-01 17:16:12]

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/b...
File chrome/browser/printing/background_printing_manager.cc (right):

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/b...
chrome/browser/printing/background_printing_manager.cc:72:
dialog_controller->GetInitiator(preview_dialog);
On 2013/06/29 00:24:18, Lei Zhang wrote:
> nit: fits on the previous line.

Done.

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_dialog_controller.cc (right):

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/p...
chrome/browser/printing/print_preview_dialog_controller.cc:100: initiator_ =
initiator;
On 2013/06/29 00:24:18, Lei Zhang wrote:
> Just put this in the initializer list? I don't know why we didn't to start
with.

Done.

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_dialog_controller.h (right):

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/printing/p...
chrome/browser/printing/print_preview_dialog_controller.h:25: // For print
preview, the tab that initiates the printing operation is the
On 2013/06/29 00:24:18, Lei Zhang wrote:
> If there's non-tabs that can be initiators, can you change this part about
"the
> tab" ?

Done.

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/ui/webui/p...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/17500003/diff/29001/chrome/browser/ui/webui/p...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:161: const char
kInitiatorTitle[] = "initiatorTabTitle";
On 2013/06/29 00:24:18, Lei Zhang wrote:
> Change the value and the usage of the value in
> chrome/browser/resources/print_preview/native_layer.js too?

Done, and updated comments in native_layer.js as well.

[Lei Zhang, 2013-07-02 07:31:47]

I have a new CL to fix the test: https://codereview.chromium.org/18422004/

It works fine on Linux for me, but I'll need to check and see how well it works
on Windows.

[Lei Zhang, 2013-07-02 21:21:23]

lgtm, though you'll need to resolve a lot of conflicts in
print_preview_dialog_controller_browsertest.cc once my test fix lands.

[Mike Wittman, 2013-07-02 22:25:46]

Hi Ben, please review
chrome/browser/ui/tab_modal_confirm_dialog_delegate.{cc,h}
components/web_modal/web_contents_modal_dialog_manager.{cc,h}

All other changes in this CL are related to the print preview dialog and have
been reviewed by Lei.

[Ben Goodger (Google), 2013-07-10 19:21:07]

lgtm

[commit-bot: I haz the power, 2013-07-10 22:42:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/17500003/49001

[commit-bot: I haz the power, 2013-07-10 23:53:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-11 00:05:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/17500003/71001

[commit-bot: I haz the power, 2013-07-11 10:46:13]

Change committed as 211058

[Mike Wittman, 2013-07-12 23:30:31]

I reverted this change in r211420 because it broke the autofill dialog, which
relies on being able to navigate the WebContents behind the web contents modal
dialog. Patch set 9 is a fix for this.

Ben: can you take a brief look at the changes in Patch set 9 for these files:
  chrome/browser/ui/views/autofill/autofill_dialog_views.cc
  components/web_modal/web_contents_modal_dialog_manager.{cc,h}

Ilya: please review Patch set 9, especially the autofill and cocoa changes.

Thanks!

[Ilya Sherman, 2013-07-12 23:58:27]

LGTM % nits, though it's usually preferred to create a new CL, uploading the
committed (and subsequently reverted) code as patch set 1, and then uploading
diffs on top of the original commit as patch set 2.

https://codereview.chromium.org/17500003/diff/91001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/17500003/diff/91001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2020: //
content::WebContentsObserver implementation.
nit: Please leave a blank line after this one.

https://codereview.chromium.org/17500003/diff/91001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2027:
net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES))
nit: Please use curly braces, since the if-stmt condition spans multiple lines.

https://codereview.chromium.org/17500003/diff/91001/components/web_modal/web_...
File components/web_modal/web_contents_modal_dialog_manager.cc (right):

https://codereview.chromium.org/17500003/diff/91001/components/web_modal/web_...
components/web_modal/web_contents_modal_dialog_manager.cc:126: for (i =
child_dialogs_.begin(); i != child_dialogs_.end(); i++) {
nit: ++i

[Mike Wittman, 2013-07-13 00:16:34]

On 2013/07/12 23:58:27, Ilya Sherman wrote:
> LGTM % nits, though it's usually preferred to create a new CL, uploading the
> committed (and subsequently reverted) code as patch set 1, and then uploading
> diffs on top of the original commit as patch set 2.

Thanks, good to know. I wasn't sure of the protocol for amending a reverted CL.

https://codereview.chromium.org/17500003/diff/91001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/17500003/diff/91001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2020: //
content::WebContentsObserver implementation.
On 2013/07/12 23:58:28, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/17500003/diff/91001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2027:
net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES))
On 2013/07/12 23:58:28, Ilya Sherman wrote:
> nit: Please use curly braces, since the if-stmt condition spans multiple
lines.

Done.

https://codereview.chromium.org/17500003/diff/91001/components/web_modal/web_...
File components/web_modal/web_contents_modal_dialog_manager.cc (right):

https://codereview.chromium.org/17500003/diff/91001/components/web_modal/web_...
components/web_modal/web_contents_modal_dialog_manager.cc:126: for (i =
child_dialogs_.begin(); i != child_dialogs_.end(); i++) {
On 2013/07/12 23:58:28, Ilya Sherman wrote:
> nit: ++i

Done.

[Mike Wittman, 2013-07-17 00:54:48]

Ben: please review the additional patch set 11, in addition to the changes in
Patch set 9 for these files:
  chrome/browser/ui/views/autofill/autofill_dialog_views.cc
  components/web_modal/web_contents_modal_dialog_manager.{cc,h}

Patch set 11 disables closing the dialog on web contents load when the repost
form warning dialog is accepted, to avoid a destruction-ordering crash on OS X.

[Ben Goodger (Google), 2013-07-17 16:48:15]

lgtm

https://codereview.chromium.org/17500003/diff/112001/components/web_modal/web...
File components/web_modal/web_contents_modal_dialog_manager.h (right):

https://codereview.chromium.org/17500003/diff/112001/components/web_modal/web...
components/web_modal/web_contents_modal_dialog_manager.h:50: void
SetInhibitCloseOnLoadStart(NativeWebContentsModalDialog dialog,
SetPreventClose.. ?

[Mike Wittman, 2013-07-17 17:57:49]

https://codereview.chromium.org/17500003/diff/112001/components/web_modal/web...
File components/web_modal/web_contents_modal_dialog_manager.h (right):

https://codereview.chromium.org/17500003/diff/112001/components/web_modal/web...
components/web_modal/web_contents_modal_dialog_manager.h:50: void
SetInhibitCloseOnLoadStart(NativeWebContentsModalDialog dialog,
On 2013/07/17 16:48:15, Ben Goodger (Google) wrote:
> SetPreventClose.. ?

sgtm, done.

[commit-bot: I haz the power, 2013-07-17 19:07:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/17500003/126032

[commit-bot: I haz the power, 2013-07-17 20:13:09]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-07-17 20:52:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/17500003/126032

[commit-bot: I haz the power, 2013-07-17 21:07:24]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-18 00:11:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/17500003/155002

[commit-bot: I haz the power, 2013-07-18 11:37:29]

Change committed as 212329