Make extension callbacks with auto-dismissing dialogs

Make extension callbacks with auto-dismissing dialogs

BUG=716794

[Avi (use Gerrit), 2017-04-29 22:21:42]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 22:21:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 23:07:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-29 23:07:46]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-04-29 23:15:29]

avi@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Avi (use Gerrit), 2017-04-30 03:40:25]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-30 03:40:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-30 06:08:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-30 06:09:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-05-01 00:18:01]

The JavaScriptDialogExtensionsClient is used to increment the keepalive count
for an extension event page (and, as you discovered, as a place to hook into for
tests - but that's less functionally important).  It looks like the
JavaScriptDialogTabHelper is only created for tabs, though, and I don't *think*
we call TabHelpers::AttachTabHelpers for background pages normally.  If that's
the case, we'd need to add a different
JavaScriptDialogTabHelper::CreateForWebContents call somewhere, like
ChromeExtensionHostDelegate::OnExtensionHostCreated().

That said, would not having the JavascriptDialogTabHelper created for a web
contents that can create dialogs cause other problems as well?

[Avi (use Gerrit), 2017-05-01 00:38:13]

On 2017/05/01 00:18:01, Devlin wrote:
> The JavaScriptDialogExtensionsClient is used to increment the keepalive count
> for an extension event page (and, as you discovered, as a place to hook into
for
> tests - but that's less functionally important).  It looks like the
> JavaScriptDialogTabHelper is only created for tabs, though, and I don't
*think*
> we call TabHelpers::AttachTabHelpers for background pages normally.  If that's
> the case, we'd need to add a different
> JavaScriptDialogTabHelper::CreateForWebContents call somewhere, like
> ChromeExtensionHostDelegate::OnExtensionHostCreated().
> 
> That said, would not having the JavascriptDialogTabHelper created for a web
> contents that can create dialogs cause other problems as well?

As a bit of explanation:

Yes, the JavaScriptDialogTabHelper is only created for tabs, and when a
WebContents that's used as a tab calls
WebContentsDelegate::GetJavaScriptDialogManager(), its delegate is the Browser
and the Browser object returns the JavaScriptDialogTabHelper.

What is the delegate for a background page? That delegate is what returns the
JavaScriptDialogManager used for a page.

[Devlin, 2017-05-01 04:55:08]

On 2017/05/01 00:38:13, Avi (ping after 24h) wrote:
> On 2017/05/01 00:18:01, Devlin wrote:
> > The JavaScriptDialogExtensionsClient is used to increment the keepalive
count
> > for an extension event page (and, as you discovered, as a place to hook into
> for
> > tests - but that's less functionally important).  It looks like the
> > JavaScriptDialogTabHelper is only created for tabs, though, and I don't
> *think*
> > we call TabHelpers::AttachTabHelpers for background pages normally.  If
that's
> > the case, we'd need to add a different
> > JavaScriptDialogTabHelper::CreateForWebContents call somewhere, like
> > ChromeExtensionHostDelegate::OnExtensionHostCreated().
> > 
> > That said, would not having the JavascriptDialogTabHelper created for a web
> > contents that can create dialogs cause other problems as well?
> 
> As a bit of explanation:
> 
> Yes, the JavaScriptDialogTabHelper is only created for tabs, and when a
> WebContents that's used as a tab calls
> WebContentsDelegate::GetJavaScriptDialogManager(), its delegate is the Browser
> and the Browser object returns the JavaScriptDialogTabHelper.
> 
> What is the delegate for a background page? That delegate is what returns the
> JavaScriptDialogManager used for a page.

The WebContentsDelegate for a background page is the ExtensionHost, which
(through a bit of redirection) returns the app_modal::JavaScriptDialogManager
singleton instance.  This singleton looks like it *does* call into the
extensions client equivalent.

So, if my interpretation is correct, the new tab helper implementation is
completely circumvented for background page alerts.  On the upside, this means
the keepalive count and other logic is, hopefully, correct.  But does this cause
problems for anything else?

[Avi (use Gerrit), 2017-05-01 13:49:31]

On 2017/05/01 04:55:08, Devlin wrote:
> On 2017/05/01 00:38:13, Avi (ping after 24h) wrote:
> > On 2017/05/01 00:18:01, Devlin wrote:
> > > The JavaScriptDialogExtensionsClient is used to increment the keepalive
> count
> > > for an extension event page (and, as you discovered, as a place to hook
into
> > for
> > > tests - but that's less functionally important).  It looks like the
> > > JavaScriptDialogTabHelper is only created for tabs, though, and I don't
> > *think*
> > > we call TabHelpers::AttachTabHelpers for background pages normally.  If
> that's
> > > the case, we'd need to add a different
> > > JavaScriptDialogTabHelper::CreateForWebContents call somewhere, like
> > > ChromeExtensionHostDelegate::OnExtensionHostCreated().
> > > 
> > > That said, would not having the JavascriptDialogTabHelper created for a
web
> > > contents that can create dialogs cause other problems as well?
> > 
> > As a bit of explanation:
> > 
> > Yes, the JavaScriptDialogTabHelper is only created for tabs, and when a
> > WebContents that's used as a tab calls
> > WebContentsDelegate::GetJavaScriptDialogManager(), its delegate is the
Browser
> > and the Browser object returns the JavaScriptDialogTabHelper.
> > 
> > What is the delegate for a background page? That delegate is what returns
the
> > JavaScriptDialogManager used for a page.
> 
> The WebContentsDelegate for a background page is the ExtensionHost, which
> (through a bit of redirection) returns the app_modal::JavaScriptDialogManager
> singleton instance.  This singleton looks like it *does* call into the
> extensions client equivalent.
> 
> So, if my interpretation is correct, the new tab helper implementation is
> completely circumvented for background page alerts.  On the upside, this means
> the keepalive count and other logic is, hopefully, correct.  But does this
cause
> problems for anything else?

No, and that is the intention.

Closing this review as it is indeed the wrong approach.