Convert app banners to use Mojo.

Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo and moves
all banner code from chrome/renderer to Blink. It also substantially
improves the layout test coverage for the BeforeInstallPromptEvent,
fixing various renderer crash bugs that are covered by the tests.

The key change is that the browser-side AppBannerManager makes a Mojo
request to Blink, which is intercepted by the WebLocalFrame and rerouted
to the AppBannerController. The AppBannerManager passes a bound
InterfaceRequest/Ptr in the Mojo request, which are used to create a
bound BeforeInstallPromptEvent. This ensures that when a
BeforeInstallPromptEvent is created, it already has a browser connection,
and does not need another asynchronous (and possibly racey) call to
establish full two-way communication with the browser process.

Several files in Blink's modules/app_banner directories that were solely
required for the AppBannerClient layer in chrome/renderer are deleted.
The existing layout tests are simplified by removing request_ids and
eliminating the AppBannerClient from ContentRendererClients. This
requires a dependency on content/public/common from
components/test_runner to allow a shim mojom::AppBannerService to be
injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704, 655877, 655902
Committed: https://crrev.com/09f7b579849cc8725455d0468b03a0237a6b624d
Cr-Commit-Position: refs/heads/master@{#428238}

[dominickn, 2016-10-05 07:47:40]

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

[commit-bot: I haz the power, 2016-10-05 07:47:53]

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, 2016-10-05 09:10:26]

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

[commit-bot: I haz the power, 2016-10-05 09:10:27]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dominickn, 2016-10-06 00:22:53]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient
abstraction in chrome/renderer and having the browser process and Blink
communicate directly. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered
by the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent.

Several files in Blink's public/platform/modules/app_banner directory
are also deleted, as these were solely required for AppBannerClient.
Layout test support is also simplified by removing AppBannerClient from
ContentRendererClients and replacing it with a shim AppBannerService.

Finally, dominickn@chromium.org is added to OWNERS for the Blink-side
app banner code.

BUG=499704
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient
abstraction in chrome/renderer and having the browser process and Blink
communicate directly. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered
by the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent.

Several files in Blink's modules/app_banner directories are also deleted,
as these were solely required for AppBannerClient. Layout test support
is also simplified by removing AppBannerClient from
ContentRendererClients and replacing it with a shim AppBannerService.
This requires a dependency on content/public/common from
components/test_runner.

Finally, dominickn@chromium.org is added to OWNERS for the Blink-side
app banner code.

BUG=499704
==========

[dominickn, 2016-10-06 00:22:53]

dominickn@chromium.org changed reviewers:
+ sammc@chromium.org

[dominickn, 2016-10-06 00:22:59]

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

[dominickn, 2016-10-06 00:23:10]

+sammc for initial sanity check. Thanks!

[commit-bot: I haz the power, 2016-10-06 00:23:39]

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, 2016-10-06 03:41:10]

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

[commit-bot: I haz the power, 2016-10-06 03:41:11]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sam McNally, 2016-10-06 07:17:44]

https://codereview.chromium.org/2393513004/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2393513004/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:528: mojo::String referrer) {
const std::string&

https://codereview.chromium.org/2393513004/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2393513004/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:91: // when
Reflow.

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:21:
mojom::AppBannerPromptReply* reply) {
Could the reply be a return value instead?

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:67:
mojom::blink::AppBannerServicePtr m_bannerService;
You'll need a prefinalizer to clean up the mojo connections. See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webusb...
for an example of this.

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/BUILD.gn:632:
"platform/modules/app_banner/app_banner.mojom",
Add your mojom to "new_wrapper_types_mojo_bindings" instead. Then your generated
bindings will use std::string instead of mojo::String.

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/app_banner/app_banner.mojom
(right):

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/app_banner/app_banner.mojom:15:
BannerPromptRequest(int32 request_id, string platform) =>
Two space indent.

[dominickn, 2016-10-06 09:28:43]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient
abstraction in chrome/renderer and having the browser process and Blink
communicate directly. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered
by the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent.

Several files in Blink's modules/app_banner directories are also deleted,
as these were solely required for AppBannerClient. Layout test support
is also simplified by removing AppBannerClient from
ContentRendererClients and replacing it with a shim AppBannerService.
This requires a dependency on content/public/common from
components/test_runner.

Finally, dominickn@chromium.org is added to OWNERS for the Blink-side
app banner code.

BUG=499704
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered by
the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent.

Several files in Blink's modules/app_banner directories are also deleted,
as these were solely required for AppBannerClient. Layout test support
is also simplified by removing the need for request_ids and the
AppBannerClient from ContentRendererClients and replacing it with a
shim AppBannerService. This requires a dependency on
content/public/common from components/test_runner.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

[dominickn, 2016-10-06 09:32:13]

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

[commit-bot: I haz the power, 2016-10-06 09:32:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-06 09:33:03]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered by
the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent.

Several files in Blink's modules/app_banner directories are also deleted,
as these were solely required for AppBannerClient. Layout test support
is also simplified by removing the need for request_ids and the
AppBannerClient from ContentRendererClients and replacing it with a
shim AppBannerService. This requires a dependency on
content/public/common from components/test_runner.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered by
the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent and inform the browser whether
the event has preventDefault() called on it.

Several files in Blink's modules/app_banner directories are also deleted,
as these were solely required for AppBannerClient. Layout test support
is also simplified by removing the need for request_ids and the
AppBannerClient from ContentRendererClients and replacing it with a
shim AppBannerService. This requires a dependency on
content/public/common from components/test_runner.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

[dominickn, 2016-10-06 09:43:43]

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

[commit-bot: I haz the power, 2016-10-06 09:43:55]

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, 2016-10-06 11:45:02]

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

[commit-bot: I haz the power, 2016-10-06 11:45:03]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dominickn, 2016-10-06 12:38:53]

Patchset #3 (id:40001) has been deleted

[dominickn, 2016-10-06 12:42:38]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered by
the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent and inform the browser whether
the event has preventDefault() called on it.

Several files in Blink's modules/app_banner directories are also deleted,
as these were solely required for AppBannerClient. Layout test support
is also simplified by removing the need for request_ids and the
AppBannerClient from ContentRendererClients and replacing it with a
shim AppBannerService. This requires a dependency on
content/public/common from components/test_runner.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered by
the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent and inform the browser whether
the event has preventDefault() called on it.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests. This
is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing the need for request_ids and the
AppBannerClient from ContentRendererClients. This requires a
dependency on content/public/common from components/test_runner
to allow a shim mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

[dominickn, 2016-10-06 12:42:46]

Patchset #4 (id:80001) has been deleted

[dominickn, 2016-10-06 12:49:04]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the initialisation of the banner pipeline, triggered by
the browser process calling to the ChromeRenderFrameObserver so it
can create a BeforeInstallPromptEvent and inform the browser whether
the event has preventDefault() called on it.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests. This
is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing the need for request_ids and the
AppBannerClient from ContentRendererClients. This requires a
dependency on content/public/common from components/test_runner
to allow a shim mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the trigger for creating a BeforeInstallPromptEvent
in ChromeRenderFrameObserver, so it can and inform the browser whether
the event has preventDefault() called on it.

The key change is having the browser-side AppBannerManager pass
a bound InterfaceRequest and InterfacePointer to
ChromeRenderFrameObserver, which decomposes them into the
underlying mojo::ScopedMessagePipeHandles and passes them to Blink.
The handles are reconstituted into the generated mojo types in
BeforeInstallPromptEvent, which can the use them to connect directly to
the browser process.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests. This
is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing the need for request_ids and the
AppBannerClient from ContentRendererClients. This requires a
dependency on content/public/common from components/test_runner
to allow a shim mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

[dominickn, 2016-10-06 12:49:12]

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

[commit-bot: I haz the power, 2016-10-06 12:49:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-06 12:56:54]

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

[commit-bot: I haz the power, 2016-10-06 12:57:10]

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, 2016-10-06 14:24:34]

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

[commit-bot: I haz the power, 2016-10-06 14:24:35]

Dry run: This issue passed the CQ dry run.

[pkotwicz1, 2016-10-06 15:45:27]

pkotwicz@google.com changed reviewers:
+ pkotwicz@google.com

[pkotwicz1, 2016-10-06 15:45:28]

https://codereview.chromium.org/2393513004/diff/120001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2393513004/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:131: : "play";
Drive by: It is probably worth calling out this change in the CL description

[Sam McNally, 2016-10-06 22:48:48]

lgtm

https://codereview.chromium.org/2393513004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:49:
m_binding.Close();
Reset m_bannerService too.

[dominickn, 2016-10-06 22:58:12]

https://codereview.chromium.org/2393513004/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2393513004/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:528: mojo::String referrer) {
On 2016/10/06 07:17:43, Sam McNally wrote:
> const std::string&

Done.

https://codereview.chromium.org/2393513004/diff/20001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2393513004/diff/20001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:91: // when
On 2016/10/06 07:17:43, Sam McNally wrote:
> Reflow.

Done.

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:21:
mojom::AppBannerPromptReply* reply) {
On 2016/10/06 07:17:43, Sam McNally wrote:
> Could the reply be a return value instead?

I'd rather leave it so that it's consistently an output argument the whole way
through, given it has to be sent back through a callback anyway.

Also, making it a return value means that I have to include the mojom-blink.h
file in WebLocalFrame.h, instead of just forward declaring it.

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:67:
mojom::blink::AppBannerServicePtr m_bannerService;
On 2016/10/06 07:17:44, Sam McNally wrote:
> You'll need a prefinalizer to clean up the mojo connections. See
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webusb...
> for an example of this.

Done.

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/BUILD.gn:632:
"platform/modules/app_banner/app_banner.mojom",
On 2016/10/06 07:17:44, Sam McNally wrote:
> Add your mojom to "new_wrapper_types_mojo_bindings" instead. Then your
generated
> bindings will use std::string instead of mojo::String.

Done.

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/app_banner/app_banner.mojom
(right):

https://codereview.chromium.org/2393513004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/app_banner/app_banner.mojom:15:
BannerPromptRequest(int32 request_id, string platform) =>
On 2016/10/06 07:17:44, Sam McNally wrote:
> Two space indent.

Done.

https://codereview.chromium.org/2393513004/diff/120001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2393513004/diff/120001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:131: : "play";
On 2016/10/06 15:45:28, pkotwicz1 wrote:
> Drive by: It is probably worth calling out this change in the CL description

This fixes an inconsistency where the AppBannerInfoBarDelegate sent "play" when
BeforeInstallPrompt resolved and the Manager sent "android" to init the event. I
didn't call out this cleanup because the string sent when the event resolves is
explicitly tested and the init string is basically never touched (and it's never
specified what exactly it will be).

[dominickn, 2016-10-06 23:00:59]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the trigger for creating a BeforeInstallPromptEvent
in ChromeRenderFrameObserver, so it can and inform the browser whether
the event has preventDefault() called on it.

The key change is having the browser-side AppBannerManager pass
a bound InterfaceRequest and InterfacePointer to
ChromeRenderFrameObserver, which decomposes them into the
underlying mojo::ScopedMessagePipeHandles and passes them to Blink.
The handles are reconstituted into the generated mojo types in
BeforeInstallPromptEvent, which can the use them to connect directly to
the browser process.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests. This
is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing the need for request_ids and the
AppBannerClient from ContentRendererClients. This requires a
dependency on content/public/common from components/test_runner
to allow a shim mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the trigger for creating a BeforeInstallPromptEvent
in ChromeRenderFrameObserver, so it can and inform the browser whether
the event has preventDefault() called on it.

The key change is that the browser-side AppBannerManager passes a
bound InterfaceRequest/Ptr to ChromeRenderFrameObserver, which
decomposes them into mojo::ScopedMessagePipeHandles to pass to
Blink without requiring the explicit banner mojom types. The handles
are reconstituted into the generated Blink-side mojo types in
BeforeInstallPromptEvent, which uses them to connect directly to the
browser process.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests. This
is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing request_ids and eliminating the AppBannerClient
from ContentRendererClients. This requires a dependency on
content/public/common from components/test_runner to allow a shim
mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

[dominickn, 2016-10-06 23:28:18]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org, dfalcantara@chromium.org, esprehn@chromium.org,
mlamouri@chromium.org

[dominickn, 2016-10-06 23:32:47]

dominickn@chromium.org changed reviewers:
+ jochen@chromium.org

[dominickn, 2016-10-06 23:38:09]

dominickn@chromium.org changed reviewers:
+ dcheng@chromium.org

[dominickn, 2016-10-06 23:38:10]

Thanks!

+benwells, +dfalcantara: PTAL at chrome/browser/banners and
chrome/browser/android/banners

+mlamouri: PTAL at third_party/WebKit/.../app_banner and the removal of
chrome/renderer/banners

+esprehn: PTAL at third_party/WebKit/ + mojoification FYI

+jochen: PTAL at components/test_runner and content/

+dcheng: mojo security review and render_messages.h

https://codereview.chromium.org/2393513004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:49:
m_binding.Close();
On 2016/10/06 22:48:48, Sam McNally wrote:
> Reset m_bannerService too.

Done.

[dominickn, 2016-10-06 23:38:18]

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

[commit-bot: I haz the power, 2016-10-06 23:38:45]

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, 2016-10-06 23:42:50]

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

[commit-bot: I haz the power, 2016-10-06 23:42:51]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dominickn, 2016-10-07 00:02:39]

dominickn@chromium.org changed reviewers:
+ rockot@chromium.org

[dominickn, 2016-10-07 00:02:40]

Also +rockot for mojo/public.

[dominickn, 2016-10-07 00:03:08]

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

[commit-bot: I haz the power, 2016-10-07 00:03:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-07 00:12:21]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the trigger for creating a BeforeInstallPromptEvent
in ChromeRenderFrameObserver, so it can and inform the browser whether
the event has preventDefault() called on it.

The key change is that the browser-side AppBannerManager passes a
bound InterfaceRequest/Ptr to ChromeRenderFrameObserver, which
decomposes them into mojo::ScopedMessagePipeHandles to pass to
Blink without requiring the explicit banner mojom types. The handles
are reconstituted into the generated Blink-side mojo types in
BeforeInstallPromptEvent, which uses them to connect directly to the
browser process.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests. This
is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing request_ids and eliminating the AppBannerClient
from ContentRendererClients. This requires a dependency on
content/public/common from components/test_runner to allow a shim
mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the trigger for creating a BeforeInstallPromptEvent
in ChromeRenderFrameObserver, so it can and inform the browser whether
the event has preventDefault() called on it.

The key change is that the browser-side AppBannerManager passes a
bound InterfaceRequest/Ptr to ChromeRenderFrameObserver, which
decomposes them into mojo::ScopedMessagePipeHandles to pass to
Blink without requiring the explicit banner mojom types. The handles
are reconstituted into the generated Blink-side mojo types in
BeforeInstallPromptEvent, which uses them to connect directly to the
browser process.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests.
This is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing request_ids and eliminating the AppBannerClient
from ContentRendererClients. This requires a dependency on
content/public/common from components/test_runner to allow a shim
mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

[Ken Rockot(use gerrit already), 2016-10-07 00:15:31]

On 2016/10/07 at 00:02:40, dominickn wrote:
> Also +rockot for mojo/public.

I don't see any changes to mojo/public

[dominickn, 2016-10-07 00:19:39]

On 2016/10/07 00:15:31, Ken Rockot wrote:
> On 2016/10/07 at 00:02:40, dominickn wrote:
> > Also +rockot for mojo/public.
> 
> I don't see any changes to mojo/public

For clarity, I got a presubmit message saying:

You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
    '+mojo/public',

This path was added as DEPS in components/test_runner and
WebKit/Source/modules/app_banner

[Ken Rockot(use gerrit already), 2016-10-07 00:23:29]

Oops, sorry I didn't see the DEPS change. That lgtm

[esprehn, 2016-10-07 00:39:52]

This is doing very low level mojo stuff we've never done in blink before, I'm a
bit concerned about doing that, and I'd prefer if we could do the actual onion
soup work here instead of hacking this up. If we do really need this I think we
need to figure out some abstractions for platform/mojo to hide this, passing
around the low level handles and manually creating proxies and such is something
I want to avoid.

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:12:
#include "public/platform/WebString.h"
it's very strange to use WebString inside modules or core, I'm not sure I see
where it's being used below?

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:23:
m_bannerService(mojo::MakeProxy(mojom::blink::AppBannerServicePtrInfo(
Nothing uses mojo::MakeProxy inside blink today, why does this API require this
low level thing and other ones didn't?

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:25:
mojom::blink::AppBannerService::Version_))),
We never pass a Version_ anywhere else in blink, what's special here?

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:97:
void BeforeInstallPromptEvent::BannerAccepted(const WTF::String& platform) {
Don't need the WTF:: on wtf things

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:77:
mojo::Binding<mojom::blink::AppBannerEvent> m_binding;
What is a Binding vs a ServicePtr? Why do we need one?

[dominickn, 2016-10-07 00:56:13]

esprehn: see ps1 for a version which does less low level mojo stuff in Blink,
i.e. no ScopedMessagePipeHandles. It preserves the current WebLocalFrame
interface at the cost of requiring the BeforeInstallPromptEvent to do a registry
lookup and IPC back to the browser process.

I think the key difference with this feature is that it contains all of the
following:

a) initiated by the browser process
b) requires multiple browser -> renderer and renderer -> browser communication
(rather than fire once and get a response)
c) needs to know if an event is preventDefaulted (which requires the return
value from DOMWindow::dispatchEvent in core/)

The downside with ps1 is that:

1. sammc pointed out a potential race between BeforeInstallPromptEvent calling
AppBannerService::SetEvent and AppBannerManager calling
AppBannerEvent::BannerAccepted/Dismissed. I could potentially work around this
by blocking on that method being called, but it's a little ugly.

2. AppBannerManager has to register itself to be visible to Blink on every
render frame

In ps7, we instead have:

1. AppBannerManager entirely drives the banner process, so it doesn't need to
register itself at all.

2. The race is avoided because the AppBannerManager binds the appropriate Mojo
pointer and sends it via ChromeRenderFrameObserver to Blink, so when the event
is created it already has the pipe it needs to talk to the browser process.

3. The binding represents one end of a Mojo pipe, while the Ptr represents the
other end. In BeforeInstallPromptEvent, the binding is the pipe which the
AppBannerManager uses to tell the event to resolve the userChoice promise. The
Ptr is the pipe which the Event uses to tell the Manager that the banner should
be shown now after it was preventDefault'd.

My understanding is that this feature is probably going to need *some* amount of
low-level Mojo primitive in Blink, because it's initiated by the browser
process, needs to know whether or not an Event was preventDefault'd, and has to
have back and forth communication. If more knowledgable people can find a way to
avoid this that would be great. :)

[commit-bot: I haz the power, 2016-10-07 02:12:59]

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

[commit-bot: I haz the power, 2016-10-07 02:13:01]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-10-07 07:03:17]

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_frame_observer.cc (right):

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_render_frame_observer.cc:114:
observer->banner_binding_.reset(
Nit: I think this can be replaced with Binding::Close() and then
Binding::Bind(std::move(request))

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_render_frame_observer.cc:257: // the other side.
Maybe put a TODO here that we should be able to get rid of this eventually
(hopefully?)

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_frame_observer.h (right):

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_render_frame_observer.h:36: static void BindBannerClient(
Nit: any reason this needs to be static?

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.h (right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.h:29: static
void willShowInstallBannerPrompt(
Or a TODO here would work as well (maybe better, since that'll cover test runner
cleanup as well)

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:25:
mojom::blink::AppBannerService::Version_))),
On 2016/10/07 00:39:51, esprehn wrote:
> We never pass a Version_ anywhere else in blink, what's special here?

Before we get here, is it possible to turn it back into a InterfaceRequest by
calling InterfaceRequest::Bind on the passed message pipe handle? That might
help this code look a bit more straightforward.

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:38:
LocalFrame* frame,
Nit: LocalFrame, as |frame| can't be null here.

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:77:
mojo::Binding<mojom::blink::AppBannerEvent> m_binding;
On 2016/10/07 00:39:52, esprehn wrote:
> What is a Binding vs a ServicePtr? Why do we need one?

ServicePtr is an InterfacePtr and manages a message pipe to talk to a remote
object.

Binding manages the other end of a message pipe, so other things can talk to
this object remotely.

[haraken, 2016-10-07 07:09:39]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-10-07 07:09:40]

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:31:
USING_PRE_FINALIZER(BeforeInstallPromptEvent, dispose);

The reason we need to add the pre-finalizer is that mojo::Binding holds a raw
pointer to BeforeInstallPromptEvent. It's a bit unfortunate that we have to add
a pre-finalizer for all GCed objects that have mojo::Binding. Is there any
option to make mojo::Binding hold WeakPersistent instead of a raw poniter? Then
we can remove the pre-finalizer.

[Ken Rockot(use gerrit already), 2016-10-07 07:42:55]

On Oct 7, 2016 12:09 AM, <haraken@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
> (right):
>
>
https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:31:
> USING_PRE_FINALIZER(BeforeInstallPromptEvent, dispose);
>
> The reason we need to add the pre-finalizer is that mojo::Binding holds
> a raw pointer to BeforeInstallPromptEvent. It's a bit unfortunate that
> we have to add a pre-finalizer for all GCed objects that have
> mojo::Binding. Is there any option to make mojo::Binding hold
> WeakPersistent instead of a raw poniter? Then we can remove the
> pre-finalizer.

No, but this seems like a reasonable idea (maybe not blocking this CL). I
can propose something to address this soon.

>
> https://codereview.chromium.org/2393513004/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-07 07:42:55]

On Oct 7, 2016 12:09 AM, <haraken@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
> (right):
>
>
https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:31:
> USING_PRE_FINALIZER(BeforeInstallPromptEvent, dispose);
>
> The reason we need to add the pre-finalizer is that mojo::Binding holds
> a raw pointer to BeforeInstallPromptEvent. It's a bit unfortunate that
> we have to add a pre-finalizer for all GCed objects that have
> mojo::Binding. Is there any option to make mojo::Binding hold
> WeakPersistent instead of a raw poniter? Then we can remove the
> pre-finalizer.

No, but this seems like a reasonable idea (maybe not blocking this CL). I
can propose something to address this soon.

>
> https://codereview.chromium.org/2393513004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-10-08 03:13:31]

I'll look this over again next week, I'd really like to see some abstraction
added here in platform/mojo though so the low level stuff going on here is more
encapsulated. I worry this patch is very subtle and future folks who maintain
this code aren't going to know what's up.

[Sam McNally, 2016-10-10 03:48:08]

On 2016/10/08 03:13:31, esprehn wrote:
> I'll look this over again next week, I'd really like to see some abstraction
> added here in platform/mojo though so the low level stuff going on here is
more
> encapsulated. I worry this patch is very subtle and future folks who maintain
> this code aren't going to know what's up.

This messiness is a result of two things:
1. What's needed is for the renderer to expose a mojo interface
(AppBannerClient) to the browser. The way to do this is to add the interface
factory to an InterfaceRegistry, which isn't currently exposed to blink. In this
case, this results in the code being split between blink and //chrome/renderer;
//chrome/renderer receives the initial mojo connection and then forwards almost
everything to blink.
2. The mojo interface exposed by the renderer receives an InterfacePtr and an
InterfaceRequest.

This combination requires passing handles through blink's public API. Because of
the separate blink and chromium generated mojo bindings, we don't have a nice
type that we can use in public blink headers. That leaves
mojo::ScopedMessagePipeHandle.

I think the fix for this is to expose InterfaceRegistry to blink; then all the
renderer app banner code can live together. I should have a CL for that out
today.

[benwells, 2016-10-12 03:00:17]

Love the mojo, so much more readable and way less plumbing.

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (left):

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:485: if (request_id !=
event_request_id_)
I assume this check here was to guarantee something to prevent races. How come
it is safe to delete it now?

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:35:
base::LazyInstance<base::TimeDelta> gTimeDeltaForTesting =
Unrelated nit: you could get rid of this LazyInstance by just have int
gDaysDeltaForTesting and constructing the TimeDelta as needed.

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:133: if (binding_.is_bound())
Why does the service have to be invalidated after each request?

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.cc (right):

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.cc:45: // metric here because
the banner isn't necessarily dismissed.
Is this an orthogonal fix?

[dominickn, 2016-10-13 00:18:16]

sammc has a CL to add a blink InterfaceRegistry at crrev.com/2401333002. This
would allow WebLocalFrame to implement a Mojo service that the browser process
could then connect to. That would allow us to avoid having
mojo::ScopedMessagePipeHandles in the Blink public API and fully Onion Soup this
feature. Would people prefer having that?

rockot has also implement the ability for Mojo bindings to provide a custom
pointer type rather than a raw pointer, so I think I can now remove the
dispose() method in BeforeInstallPromptEvent.

Finally, mgiuca@ is implementing a related "appinstalled" event at
crrev.com/2379923002, so there may be some folder restructuring in either that
or this CL.

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (left):

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:485: if (request_id !=
event_request_id_)
On 2016/10/12 03:00:17, benwells (slow) wrote:
> I assume this check here was to guarantee something to prevent races. How come
> it is safe to delete it now?

This is implicitly handled because OnBannerPromptReply can only be called as the
reply from calling client_->BannerPromptRequest, which sets event_request_id_
for this request. If we terminate the banner flow before the reply comes back,
Stop() calls InvalidateWeakPtrs which will prevent this method from being run.

Previously with IPCs, it was possible for the IPC to still be sent even though
the AppBannerManager had moved on to a new request because there wasn't a way to
tell the renderer to stop.

It may even be possible to get rid of the event_request_ids entirely from the
browser side too, though I don't want to do that in this CL.

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:35:
base::LazyInstance<base::TimeDelta> gTimeDeltaForTesting =
On 2016/10/12 03:00:17, benwells (slow) wrote:
> Unrelated nit: you could get rid of this LazyInstance by just have int
> gDaysDeltaForTesting and constructing the TimeDelta as needed.

Done.

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:133: if (binding_.is_bound())
On 2016/10/12 03:00:17, benwells (slow) wrote:
> Why does the service have to be invalidated after each request?

AppBannerManager only ever manages one in-flight request at a time (we stop in
RequestAppBanner if we're already checking this URL). So if we've issued a
binding to an event previously, that binding is invalid once we get here.

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_desktop.cc (right):

https://codereview.chromium.org/2393513004/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_desktop.cc:45: // metric here because
the banner isn't necessarily dismissed.
On 2016/10/12 03:00:17, benwells (slow) wrote:
> Is this an orthogonal fix?

The Stop() or the no dismiss? The Stop() fix is sort of orthogonal. It's because
the user could accept the banner, but hit Cancel on the bookmark app install
dialog. That will call to here with |extension| = nullptr, and in that case we
should clear out the current state. It's less important now that
AppBannerManager::binding_ is reset every time a new request starts, but it's
good for cleanliness.

The no dismiss is orthogonal and aligns desktop with Android.

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_frame_observer.cc (right):

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_render_frame_observer.cc:114:
observer->banner_binding_.reset(
On 2016/10/07 07:03:16, dcheng wrote:
> Nit: I think this can be replaced with Binding::Close() and then
> Binding::Bind(std::move(request))

Done.

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_frame_observer.h (right):

https://codereview.chromium.org/2393513004/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_render_frame_observer.h:36: static void BindBannerClient(
On 2016/10/07 07:03:16, dcheng wrote:
> Nit: any reason this needs to be static?

Nope! Done.

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:12:
#include "public/platform/WebString.h"
On 2016/10/07 00:39:51, esprehn wrote:
> it's very strange to use WebString inside modules or core, I'm not sure I see
> where it's being used below?

Done.

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:97:
void BeforeInstallPromptEvent::BannerAccepted(const WTF::String& platform) {
On 2016/10/07 00:39:51, esprehn wrote:
> Don't need the WTF:: on wtf things

Done.

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:31:
USING_PRE_FINALIZER(BeforeInstallPromptEvent, dispose);
On 2016/10/07 07:09:40, haraken wrote:
> 
> The reason we need to add the pre-finalizer is that mojo::Binding holds a raw
> pointer to BeforeInstallPromptEvent. It's a bit unfortunate that we have to
add
> a pre-finalizer for all GCed objects that have mojo::Binding. Is there any
> option to make mojo::Binding hold WeakPersistent instead of a raw poniter?
Then
> we can remove the pre-finalizer.
> 

I believe this has landed now.

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:38:
LocalFrame* frame,
On 2016/10/07 07:03:16, dcheng wrote:
> Nit: LocalFrame, as |frame| can't be null here.

Do you mean LocalFrame&?

[dominickn, 2016-10-14 01:47:50]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the trigger for creating a BeforeInstallPromptEvent
in ChromeRenderFrameObserver, so it can and inform the browser whether
the event has preventDefault() called on it.

The key change is that the browser-side AppBannerManager passes a
bound InterfaceRequest/Ptr to ChromeRenderFrameObserver, which
decomposes them into mojo::ScopedMessagePipeHandles to pass to
Blink without requiring the explicit banner mojom types. The handles
are reconstituted into the generated Blink-side mojo types in
BeforeInstallPromptEvent, which uses them to connect directly to the
browser process.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests.
This is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing request_ids and eliminating the AppBannerClient
from ContentRendererClients. This requires a dependency on
content/public/common from components/test_runner to allow a shim
mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the trigger for creating a BeforeInstallPromptEvent
in ChromeRenderFrameObserver, so it can and inform the browser whether
the event has preventDefault() called on it.

The key change is that the browser-side AppBannerManager passes a
bound InterfaceRequest/Ptr to ChromeRenderFrameObserver, which
decomposes them into mojo::ScopedMessagePipeHandles to pass to
Blink without requiring the explicit banner mojom types. The handles
are reconstituted into the generated Blink-side mojo types in
BeforeInstallPromptEvent, which uses them to connect directly to the
browser process.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests.
This is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing request_ids and eliminating the AppBannerClient
from ContentRendererClients. This requires a dependency on
content/public/common from components/test_runner to allow a shim
mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704,655877
==========

[dominickn, 2016-10-17 03:55:16]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo. It also
mostly Onion-Soupifies app banners by removing the AppBannerClient in
chrome/renderer and having the browser process and Blink communicate
directly where possible. The only remaining app banner code in
chrome/renderer is the trigger for creating a BeforeInstallPromptEvent
in ChromeRenderFrameObserver, so it can and inform the browser whether
the event has preventDefault() called on it.

The key change is that the browser-side AppBannerManager passes a
bound InterfaceRequest/Ptr to ChromeRenderFrameObserver, which
decomposes them into mojo::ScopedMessagePipeHandles to pass to
Blink without requiring the explicit banner mojom types. The handles
are reconstituted into the generated Blink-side mojo types in
BeforeInstallPromptEvent, which uses them to connect directly to the
browser process.

Completely Onion-Souping app banners will require the capability for
registering Mojo services from Blink, which is not currently possible. It
will also require a substantial rewrite of the app banner layout tests.
This is left as future work.

Several files in Blink's modules/app_banner directories that were solely
required for AppBannerClient are deleted. The existing layout tests are
simplified by removing request_ids and eliminating the AppBannerClient
from ContentRendererClients. This requires a dependency on
content/public/common from components/test_runner to allow a shim
mojom::AppBannerService to be injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704,655877
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo and moves
all banner code from chrome/renderer to Blink. It also substantially
improves the layout test coverage for the BeforeInstallPromptEvent,
fixing various renderer crash bugs that are covered by the tests.

The key change is that the browser-side AppBannerManager makes a Mojo
request to Blink, which is intercepted by the WebLocalFrame and rerouted
to the AppBannerController. The AppBannerManager passes a bound
InterfaceRequest/Ptr in the Mojo request, which are used to create a
bound BeforeInstallPromptEvent. This ensures that when a
BeforeInstallPromptEvent is created, it already has a browser connection,
and does not need another asynchronous (and possibly racey) call to
establish full two-way communication with the browser process.

Several files in Blink's modules/app_banner directories that were solely
required for the AppBannerClient layer in chrome/renderer are deleted.
The existing layout tests are simplified by removing request_ids and
eliminating the AppBannerClient from ContentRendererClients. This
requires a dependency on content/public/common from
components/test_runner to allow a shim mojom::AppBannerService to be
injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704,655877,655902
==========

[dominickn, 2016-10-17 04:30:34]

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

[commit-bot: I haz the power, 2016-10-17 04:30:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-17 05:38:45]

Please take another look everyone: this is now completely Onion Souped thanks to
the new blink InterfaceRegistry. There is no remaining banner code in
chrome/renderer at all. I've also beefed up the LayoutTests for banners.

Thanks sammc for the InterfaceRegistry. :)

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:31:
USING_PRE_FINALIZER(BeforeInstallPromptEvent, dispose);
On 2016/10/13 00:18:15, dominickn wrote:
> On 2016/10/07 07:09:40, haraken wrote:
> > 
> > The reason we need to add the pre-finalizer is that mojo::Binding holds a
raw
> > pointer to BeforeInstallPromptEvent. It's a bit unfortunate that we have to
> add
> > a pre-finalizer for all GCed objects that have mojo::Binding. Is there any
> > option to make mojo::Binding hold WeakPersistent instead of a raw poniter?
> Then
> > we can remove the pre-finalizer.
> > 
> 
> I believe this has landed now.

Follow up: we have both an InterfacePtr and a binding to dispose of here. The
StructTraits option with rockot added handles the latter case, but we still need
to unbind the InterfacePtr. Leaving the dispose() here until we have the right
support.

https://codereview.chromium.org/2393513004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:77:
mojo::Binding<mojom::blink::AppBannerEvent> m_binding;
On 2016/10/07 07:03:16, dcheng wrote:
> On 2016/10/07 00:39:52, esprehn wrote:
> > What is a Binding vs a ServicePtr? Why do we need one?
> 
> ServicePtr is an InterfacePtr and manages a message pipe to talk to a remote
> object.
> 
> Binding manages the other end of a message pipe, so other things can talk to
> this object remotely.

As dcheng said, we need both of these here so that a) the AppBannerService can
talk to this object, and b) we can talk to the AppBannerService.

[commit-bot: I haz the power, 2016-10-17 06:02:16]

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

[commit-bot: I haz the power, 2016-10-17 06:02:18]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dominickn, 2016-10-17 10:06:48]

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

[commit-bot: I haz the power, 2016-10-17 10:07:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-17 10:09:36]

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

[commit-bot: I haz the power, 2016-10-17 10:09:56]

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, 2016-10-17 10:12:52]

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

[commit-bot: I haz the power, 2016-10-17 10:12:53]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dominickn, 2016-10-17 10:17:10]

Patchset #11 (id:240001) has been deleted

[dominickn, 2016-10-17 10:17:28]

Patchset #10 (id:220001) has been deleted

[dominickn, 2016-10-17 10:21:55]

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

[commit-bot: I haz the power, 2016-10-17 10:22:11]

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, 2016-10-17 12:30:42]

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

[commit-bot: I haz the power, 2016-10-17 12:30:43]

Dry run: This issue passed the CQ dry run.

[benwells, 2016-10-18 06:57:39]

lgtm

[mlamouri (slow - plz ping), 2016-10-18 16:55:08]

Too much code for my slow brain. I will trust the other reviewers that things
are right :)

Just one question: why not doing a JS mock for the service instead of adding it
to the TestRunner?

[dominickn, 2016-10-18 23:36:55]

On 2016/10/18 16:55:08, mlamouri wrote:
> Too much code for my slow brain. I will trust the other reviewers that things
> are right :)
> 
> Just one question: why not doing a JS mock for the service instead of adding
it
> to the TestRunner?

I didn't want to churn the LayoutTests too much in this CL because the event API
isn't changing. I basically just replaced the AppBannerClient mock with the
AppBannerService mock so that the tests didn't need to change at all.

A future CL will change the tests to have a JS mock for the service.

[dcheng, 2016-10-19 06:15:59]

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:25:
LocalFrame* frame,
I don't think we'll ever pass in a non-null |frame| here. Let's just pass it by
reference.

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:31:
std::unique_ptr<AppBannerController>(new AppBannerController(frame)),
Nit: wrapUnique from wtf/PtrUtil.h

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:91: if
(target()) {
How come we didn't need this null check before?

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:104:
"", AppBannerPromptResult::Outcome::Dismissed));
Nit: emptyAtom instead of ""

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:64:
LocalFrame*,
Let's make LocalFrame* a reference here (and in create(...)) as well.

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1626:
frame()->interfaceRegistry()->addInterface(
It feels a bit surprising to have this here. At some level, this feels kind of
like a supplement which should be associated with a context, since we're
dispatching events at a particular context, not a frame (one LocalFrame can be
reused for lots of different ExecutionContexts).

(I don't have a concrete suggestion yet, it's just something I noticed)

[dominickn, 2016-10-19 08:17:00]

Thanks!

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:25:
LocalFrame* frame,
On 2016/10/19 06:15:58, dcheng wrote:
> I don't think we'll ever pass in a non-null |frame| here. Let's just pass it
by
> reference.

This method is invoked by WTF::bind. Can it take a frame by reference like that?
(I couldn't figure it out)

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:31:
std::unique_ptr<AppBannerController>(new AppBannerController(frame)),
On 2016/10/19 06:15:58, dcheng wrote:
> Nit: wrapUnique from wtf/PtrUtil.h

Done.

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:91: if
(target()) {
On 2016/10/19 06:15:58, dcheng wrote:
> How come we didn't need this null check before?

We did need it before: crbug.com/655902 :)

Calling var e = new BeforeInstallPromptEvent({}); e.preventDefault() crashed the
renderer because the event had no target. The null check ensures we don't try to
UseCounter in that case, and there's a regression test added.

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:104:
"", AppBannerPromptResult::Outcome::Dismissed));
On 2016/10/19 06:15:58, dcheng wrote:
> Nit: emptyAtom instead of ""

Done.

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h
(right):

https://codereview.chromium.org/2393513004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.h:64:
LocalFrame*,
On 2016/10/19 06:15:58, dcheng wrote:
> Let's make LocalFrame* a reference here (and in create(...)) as well.

Done.

[mlamouri (slow - plz ping), 2016-10-19 09:42:45]

On 2016/10/18 at 23:36:55, dominickn wrote:
> On 2016/10/18 16:55:08, mlamouri wrote:
> > Too much code for my slow brain. I will trust the other reviewers that
things
> > are right :)
> > 
> > Just one question: why not doing a JS mock for the service instead of adding
it
> > to the TestRunner?
> 
> I didn't want to churn the LayoutTests too much in this CL because the event
API isn't changing. I basically just replaced the AppBannerClient mock with the
AppBannerService mock so that the tests didn't need to change at all.
> 
> A future CL will change the tests to have a JS mock for the service.

Given how much stuff you are moving around in test_runner/ and layout_test/, I'm
not entirely convinced but okay.

lgtm

[haraken, 2016-10-19 09:57:05]

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.h (right):

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.h:31:
WeakPersistent<LocalFrame> m_frame;

Why does this need to be weak?

Does AppBannerController outlive LocalFrame?

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapWeakPersistent(frame())));

I'm just curious but what happens if we use wrapPersistent(frame()) here?

If it leaks the frame, I'm worried that people may misuse wrapPersistent and
leak the frame in the future.

Maybe should we have logic to clear the interface registry when the frame gets
detached?

[dominickn, 2016-10-19 23:05:22]

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

[commit-bot: I haz the power, 2016-10-19 23:06:08]

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, 2016-10-20 01:04:37]

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

[commit-bot: I haz the power, 2016-10-20 01:04:39]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-10-20 04:54:58]

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.h (right):

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.h:31:
WeakPersistent<LocalFrame> m_frame;
On 2016/10/19 09:57:05, haraken wrote:
> 
> Why does this need to be weak?
> 
> Does AppBannerController outlive LocalFrame?

Changed to Persistent.

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapWeakPersistent(frame())));
On 2016/10/19 09:57:05, haraken wrote:
> 
> I'm just curious but what happens if we use wrapPersistent(frame()) here?
> 
> If it leaks the frame, I'm worried that people may misuse wrapPersistent and
> leak the frame in the future.
> 
> Maybe should we have logic to clear the interface registry when the frame gets
> detached?

Changed to wrapPersistent. Seems okay...

[haraken, 2016-10-20 08:43:18]

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapWeakPersistent(frame())));
On 2016/10/20 04:54:58, dominickn wrote:
> On 2016/10/19 09:57:05, haraken wrote:
> > 
> > I'm just curious but what happens if we use wrapPersistent(frame()) here?
> > 
> > If it leaks the frame, I'm worried that people may misuse wrapPersistent and
> > leak the frame in the future.
> > 
> > Maybe should we have logic to clear the interface registry when the frame
gets
> > detached?
> 
> Changed to wrapPersistent. Seems okay...

Are you sure that the persistents are cleared before the frame gets detached?

If the persistents are not cleared, it will cause memory leaks.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:40: if
(!m_frame) {

Who clears m_frame when the frame gets detached?

I guess we need to make AppBannerController DOMWindowProperty. Then you can use
frame(). frame() becomes null after the frame gets detached.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:58: if
(m_userChoice && m_binding.is_bound())

I'm just curious but what does 'is_bound' mean? Does it mean that it's not yet
closed?

[dominickn, 2016-10-20 11:41:34]

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapWeakPersistent(frame())));
On 2016/10/20 08:43:18, haraken wrote:
> On 2016/10/20 04:54:58, dominickn wrote:
> > On 2016/10/19 09:57:05, haraken wrote:
> > > 
> > > I'm just curious but what happens if we use wrapPersistent(frame()) here?
> > > 
> > > If it leaks the frame, I'm worried that people may misuse wrapPersistent
and
> > > leak the frame in the future.
> > > 
> > > Maybe should we have logic to clear the interface registry when the frame
> gets
> > > detached?
> > 
> > Changed to wrapPersistent. Seems okay...
> 
> Are you sure that the persistents are cleared before the frame gets detached?
> 
> If the persistents are not cleared, it will cause memory leaks.

Do you have a leak bot of some kind to point me towards? I don't know this stuff
nearly well enough to answer your question. Looking at crrev.com/2401333002, the
InterfaceRegistry lives on a WebFrameClient, but about as far as I can
competently answer.

Is there some reason that this CL can't use wrapWeakPersistent? If there is a
case for misusing wrapPersistent, then I think the fix for that should happen
separately to this.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:40: if
(!m_frame) {
On 2016/10/20 08:43:18, haraken wrote:
> 
> Who clears m_frame when the frame gets detached?
> 
> I guess we need to make AppBannerController DOMWindowProperty. Then you can
use
> frame(). frame() becomes null after the frame gets detached.
> 

I don't think that's necessary. AppBannerController's lifetime is strongly
connected to that of the request (this is what mojo::MakeStrongBinding does in
the ::create method). That means that when the other end of the mojo pipe is
closed, this object is freed.

The AppBannerManager clears its end of the mojo pipe immediately when |callback|
is run in this method, which frees this object. So this object only exists
between the AppBannerManager calling SendBannerPromptRequest, and the completion
of callback.Run(). That should free the persistent reference here.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp
(right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:58: if
(m_userChoice && m_binding.is_bound())
On 2016/10/20 08:43:18, haraken wrote:
> 
> I'm just curious but what does 'is_bound' mean? Does it mean that it's not yet
> closed?

Yes, is_bound() means that the mojo::Binding has had Bind() called on it to
connect it to something and the connection is open. This happens in the main
constructor, which initialises the binding with a mojo request. If this object
was created in raw Javascript with new BeforeInstallEventPrompt(), the binding
would not be bound. It's an error to create one of these events and try to
trigger an app banner with it; you must only use the one that is created by the
AppBannerManager making the Mojo call.

[haraken, 2016-10-20 15:51:23]

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapWeakPersistent(frame())));
On 2016/10/20 11:41:34, dominickn wrote:
> On 2016/10/20 08:43:18, haraken wrote:
> > On 2016/10/20 04:54:58, dominickn wrote:
> > > On 2016/10/19 09:57:05, haraken wrote:
> > > > 
> > > > I'm just curious but what happens if we use wrapPersistent(frame())
here?
> > > > 
> > > > If it leaks the frame, I'm worried that people may misuse wrapPersistent
> and
> > > > leak the frame in the future.
> > > > 
> > > > Maybe should we have logic to clear the interface registry when the
frame
> > gets
> > > > detached?
> > > 
> > > Changed to wrapPersistent. Seems okay...
> > 
> > Are you sure that the persistents are cleared before the frame gets
detached?
> > 
> > If the persistents are not cleared, it will cause memory leaks.
> 
> Do you have a leak bot of some kind to point me towards? I don't know this
stuff
> nearly well enough to answer your question. Looking at crrev.com/2401333002,
the
> InterfaceRegistry lives on a WebFrameClient, but about as far as I can
> competently answer.

We have a leak detector bot on ToT. If you leak a frame, it will be caught.

> Is there some reason that this CL can't use wrapWeakPersistent? If there is a
> case for misusing wrapPersistent, then I think the fix for that should happen
> separately to this.

It totally depends on what you want to do. If you're expecting that the frame
should be alive while the Mojo callback is outstanding, it should be Persistent.
Otherwise (if you're okay with not running the Mojo callback after the frame is
gone), it should be WeakPersistent.

Also I agree with you that my question is rather about the semantics of
InterfaceRegistry than what you do in this CL. Maybe sammc@ has some thought.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:25: void
AppBannerController::create(

Shall we rename this to createMojoBinding? In Blink, XXX::create is expected to
return a newly created object of XXX.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:40: if
(!m_frame) {
On 2016/10/20 11:41:34, dominickn wrote:
> On 2016/10/20 08:43:18, haraken wrote:
> > 
> > Who clears m_frame when the frame gets detached?
> > 
> > I guess we need to make AppBannerController DOMWindowProperty. Then you can
> use
> > frame(). frame() becomes null after the frame gets detached.
> > 
> 
> I don't think that's necessary. AppBannerController's lifetime is strongly
> connected to that of the request (this is what mojo::MakeStrongBinding does in
> the ::create method). That means that when the other end of the mojo pipe is
> closed, this object is freed.
> 
> The AppBannerManager clears its end of the mojo pipe immediately when
|callback|
> is run in this method, which frees this object. So this object only exists
> between the AppBannerManager calling SendBannerPromptRequest, and the
completion
> of callback.Run(). That should free the persistent reference here.

Thanks for the clarification. Makes sense.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapPersistent(frame())));

I'm just curious but why does addInterface take a WTF::Function instead of the
created InterfacePtr?

[dominickn, 2016-10-20 23:39:27]

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

[commit-bot: I haz the power, 2016-10-20 23:40:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-20 23:41:18]

esprehn, jochen, dcheng: ping

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapWeakPersistent(frame())));
On 2016/10/20 15:51:23, haraken wrote:
> On 2016/10/20 11:41:34, dominickn wrote:
> > On 2016/10/20 08:43:18, haraken wrote:
> > > On 2016/10/20 04:54:58, dominickn wrote:
> > > > On 2016/10/19 09:57:05, haraken wrote:
> > > > > 
> > > > > I'm just curious but what happens if we use wrapPersistent(frame())
> here?
> > > > > 
> > > > > If it leaks the frame, I'm worried that people may misuse
wrapPersistent
> > and
> > > > > leak the frame in the future.
> > > > > 
> > > > > Maybe should we have logic to clear the interface registry when the
> frame
> > > gets
> > > > > detached?
> > > > 
> > > > Changed to wrapPersistent. Seems okay...
> > > 
> > > Are you sure that the persistents are cleared before the frame gets
> detached?
> > > 
> > > If the persistents are not cleared, it will cause memory leaks.
> > 
> > Do you have a leak bot of some kind to point me towards? I don't know this
> stuff
> > nearly well enough to answer your question. Looking at crrev.com/2401333002,
> the
> > InterfaceRegistry lives on a WebFrameClient, but about as far as I can
> > competently answer.
> 
> We have a leak detector bot on ToT. If you leak a frame, it will be caught.
> 
> > Is there some reason that this CL can't use wrapWeakPersistent? If there is
a
> > case for misusing wrapPersistent, then I think the fix for that should
happen
> > separately to this.
> 
> It totally depends on what you want to do. If you're expecting that the frame
> should be alive while the Mojo callback is outstanding, it should be
Persistent.
> Otherwise (if you're okay with not running the Mojo callback after the frame
is
> gone), it should be WeakPersistent.
> 
> Also I agree with you that my question is rather about the semantics of
> InterfaceRegistry than what you do in this CL. Maybe sammc@ has some thought.
> 

The correct thing to do then is to make it weakPersistent, it doesn't make sense
for this to run with no frame there (or if the frame has already been freed).
This shouldn't keep the frame alive.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:25: void
AppBannerController::create(
On 2016/10/20 15:51:23, haraken wrote:
> 
> Shall we rename this to createMojoBinding? In Blink, XXX::create is expected
to
> return a newly created object of XXX.

Renamed to bindMojoRequest, since that seems to be the standard naming for these
things.

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapPersistent(frame())));
On 2016/10/20 15:51:23, haraken wrote:
> 
> I'm just curious but why does addInterface take a WTF::Function instead of the
> created InterfacePtr?

addInterface registers a WTF::Function(request) callback to be run when it
receives a request of the type expected by the callback. When you call
addInterface, you don't have an InterfaceRequest (InterfacePtr is the "other
end" of the Mojo pipe; when you register an interface the callback expects an
InterfaceRequest). All you're doing in addInterface is saying, "run this
callback when you receive that type of request".

AppBannerController::create is bound with the frame and expects one additional
argument, which is a mojo::InterfaceRequest<AppBannerClient>. When that request
comes in (via the AppBannerManager calling SendBannerPromptRequest), the
WTF::Function is invoked to lazily create an AppBannerController, passing in the
InterfaceRequest.

[commit-bot: I haz the power, 2016-10-21 03:27:03]

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

[commit-bot: I haz the power, 2016-10-21 03:27:05]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-10-21 08:59:45]

Thanks for being persistent. WebKit/ LGTM

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapPersistent(frame())));
On 2016/10/20 23:41:18, dominickn wrote:
> On 2016/10/20 15:51:23, haraken wrote:
> > 
> > I'm just curious but why does addInterface take a WTF::Function instead of
the
> > created InterfacePtr?
> 
> addInterface registers a WTF::Function(request) callback to be run when it
> receives a request of the type expected by the callback. When you call
> addInterface, you don't have an InterfaceRequest (InterfacePtr is the "other
> end" of the Mojo pipe; when you register an interface the callback expects an
> InterfaceRequest). All you're doing in addInterface is saying, "run this
> callback when you receive that type of request".
> 
> AppBannerController::create is bound with the frame and expects one additional
> argument, which is a mojo::InterfaceRequest<AppBannerClient>. When that
request
> comes in (via the AppBannerManager calling SendBannerPromptRequest), the
> WTF::Function is invoked to lazily create an AppBannerController, passing in
the
> InterfaceRequest.

Sorry for being noisy, but where is the expected request type specified?
addInterface just takes a WTF::Function and it doesn't know when to invoke the
function.

https://codereview.chromium.org/2393513004/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:40: if
(!m_frame) {

if (!m_frame || !m_frame->document() || !m_frame->domWindow())

?

[jochen (gone - plz use gerrit), 2016-10-21 11:04:55]

content lgtm

[dominickn, 2016-10-21 12:25:15]

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

[commit-bot: I haz the power, 2016-10-21 12:25:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-21 12:26:57]

Thanks!

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapPersistent(frame())));
On 2016/10/21 08:59:45, haraken wrote:
> On 2016/10/20 23:41:18, dominickn wrote:
> > On 2016/10/20 15:51:23, haraken wrote:
> > > 
> > > I'm just curious but why does addInterface take a WTF::Function instead of
> the
> > > created InterfacePtr?
> > 
> > addInterface registers a WTF::Function(request) callback to be run when it
> > receives a request of the type expected by the callback. When you call
> > addInterface, you don't have an InterfaceRequest (InterfacePtr is the "other
> > end" of the Mojo pipe; when you register an interface the callback expects
an
> > InterfaceRequest). All you're doing in addInterface is saying, "run this
> > callback when you receive that type of request".
> > 
> > AppBannerController::create is bound with the frame and expects one
additional
> > argument, which is a mojo::InterfaceRequest<AppBannerClient>. When that
> request
> > comes in (via the AppBannerManager calling SendBannerPromptRequest), the
> > WTF::Function is invoked to lazily create an AppBannerController, passing in
> the
> > InterfaceRequest.
> 
> Sorry for being noisy, but where is the expected request type specified?
> addInterface just takes a WTF::Function and it doesn't know when to invoke the
> function.

The short answer is template magic. :)

The long answer is that the WTF::Function must have an
InterfaceRequest<Interface> parameter in its signature (see
AppBannerController:bindMojoRequest). The Interface template class is the
generated mojo interface class, which is blink::mojom::AppBannerController in
this case. Mojo always injects a static, unique Name_ member into the generated
classes; addInterface simply maps blink::mojom::AppBannerController::Name_ to
the WTF::Function which is passed in.

So now we query the interface registry. That passes in an
InterfaceRequest<blink::mojom::AppBannerController>, so the registry just looks
up whether there is a WTF::Function mapped to
blink::mojom::AppBannerController::Name_, and runs it passing the request. This
creates a concrete AppBannerController and binds the pipe, allowing the other
end of the pipe to call methods on the AppBannerController. This is mostly
implemented inside the services / mojo layer and is pretty much all
automatically generated. Does that make sense?

If you're interested, the actual impl is
https://cs.chromium.org/chromium/src/services/service_manager/public/cpp/inte...
and
https://cs.chromium.org/chromium/src/services/service_manager/public/cpp/lib/...

https://codereview.chromium.org/2393513004/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:40: if
(!m_frame) {
On 2016/10/21 08:59:45, haraken wrote:
> 
> if (!m_frame || !m_frame->document() || !m_frame->domWindow())
> 
> ?

Done.

[commit-bot: I haz the power, 2016-10-21 14:00:53]

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

[commit-bot: I haz the power, 2016-10-21 14:00:54]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-10-23 22:42:15]

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
WTF::bind(&AppBannerController::create, wrapPersistent(frame())));
On 2016/10/21 12:26:57, dominickn wrote:
> On 2016/10/21 08:59:45, haraken wrote:
> > On 2016/10/20 23:41:18, dominickn wrote:
> > > On 2016/10/20 15:51:23, haraken wrote:
> > > > 
> > > > I'm just curious but why does addInterface take a WTF::Function instead
of
> > the
> > > > created InterfacePtr?
> > > 
> > > addInterface registers a WTF::Function(request) callback to be run when it
> > > receives a request of the type expected by the callback. When you call
> > > addInterface, you don't have an InterfaceRequest (InterfacePtr is the
"other
> > > end" of the Mojo pipe; when you register an interface the callback expects
> an
> > > InterfaceRequest). All you're doing in addInterface is saying, "run this
> > > callback when you receive that type of request".
> > > 
> > > AppBannerController::create is bound with the frame and expects one
> additional
> > > argument, which is a mojo::InterfaceRequest<AppBannerClient>. When that
> > request
> > > comes in (via the AppBannerManager calling SendBannerPromptRequest), the
> > > WTF::Function is invoked to lazily create an AppBannerController, passing
in
> > the
> > > InterfaceRequest.
> > 
> > Sorry for being noisy, but where is the expected request type specified?
> > addInterface just takes a WTF::Function and it doesn't know when to invoke
the
> > function.
> 
> The short answer is template magic. :)
> 
> The long answer is that the WTF::Function must have an
> InterfaceRequest<Interface> parameter in its signature (see
> AppBannerController:bindMojoRequest). The Interface template class is the
> generated mojo interface class, which is blink::mojom::AppBannerController in
> this case. Mojo always injects a static, unique Name_ member into the
generated
> classes; addInterface simply maps blink::mojom::AppBannerController::Name_ to
> the WTF::Function which is passed in.
> 
> So now we query the interface registry. That passes in an
> InterfaceRequest<blink::mojom::AppBannerController>, so the registry just
looks
> up whether there is a WTF::Function mapped to
> blink::mojom::AppBannerController::Name_, and runs it passing the request.
This
> creates a concrete AppBannerController and binds the pipe, allowing the other
> end of the pipe to call methods on the AppBannerController. This is mostly
> implemented inside the services / mojo layer and is pretty much all
> automatically generated. Does that make sense?
> 
> If you're interested, the actual impl is
>
https://cs.chromium.org/chromium/src/services/service_manager/public/cpp/inte...
> and
>
https://cs.chromium.org/chromium/src/services/service_manager/public/cpp/lib/...

Thanks, makes sense.

I'd personally prefer a more explicit binding API but delegate the decision to
Mojo experts :)

[gone, 2016-10-24 17:24:08]

Changes to c/b/a/banners lgtm

[dcheng, 2016-10-24 19:52:52]

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:23:
AppBannerController::AppBannerController(LocalFrame* frame) : m_frame(frame) {}
Nit: reference here though.

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:28: if
(!frame)
OK, let's just DCHECK() this. WeakPersistent should be handled just like any
other weak pointer (e.g. the callback should never be invoked if the weak
persistent is gone)

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:40: if
(!m_frame || !m_frame->document() || !m_frame->domWindow()) {
m_frame should never be null here; otherwise, it means we messed up our
bookkeeping earlier.

That being said... should it actually be a Persistent? Should a
AppBannerController by itself be enough to keep the frame alive? It seems like
it shouldn't.

Also, it's not necessary to null check both document() and domWindow(): if the
frame has a document(), it must have a domWindow().

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
&AppBannerController::bindMojoRequest, wrapWeakPersistent(frame())));
I still feel pretty uncertain about this. From what I can tell, app banner is
tied to the active document, and if we navigate, we want it go away. It's hard
for me to tell if there's a possible race here:

1. Browser decides to ask the renderer if an app banner should be shown.
2. The renderer commits a navigation.
3. Since the interface is frame-bound, we now dispatch the event on the wrong
document.

If this was context-bound somehow, it'd be much easier to determine correctness.

[dominickn, 2016-10-24 23:17:58]

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp
(right):

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:23:
AppBannerController::AppBannerController(LocalFrame* frame) : m_frame(frame) {}
On 2016/10/24 19:52:51, dcheng wrote:
> Nit: reference here though.

Done.

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:28: if
(!frame)
On 2016/10/24 19:52:51, dcheng wrote:
> OK, let's just DCHECK() this. WeakPersistent should be handled just like any
> other weak pointer (e.g. the callback should never be invoked if the weak
> persistent is gone)

Done.

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.cpp:40: if
(!m_frame || !m_frame->document() || !m_frame->domWindow()) {
On 2016/10/24 19:52:51, dcheng wrote:
> m_frame should never be null here; otherwise, it means we messed up our
> bookkeeping earlier.
> 
> That being said... should it actually be a Persistent? Should a
> AppBannerController by itself be enough to keep the frame alive? It seems like
> it shouldn't.

Changed (back) to WeakPersistent.

> Also, it's not necessary to null check both document() and domWindow(): if the
> frame has a document(), it must have a domWindow().

Keeping the m_frame and document() checks because it's a WeakPersistent again.

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
&AppBannerController::bindMojoRequest, wrapWeakPersistent(frame())));
On 2016/10/24 19:52:52, dcheng wrote:
> I still feel pretty uncertain about this. From what I can tell, app banner is
> tied to the active document, and if we navigate, we want it go away. It's hard
> for me to tell if there's a possible race here:
> 
> 1. Browser decides to ask the renderer if an app banner should be shown.
> 2. The renderer commits a navigation.
> 3. Since the interface is frame-bound, we now dispatch the event on the wrong
> document.
> 
> If this was context-bound somehow, it'd be much easier to determine
correctness.

My understanding is that this hasn't changed from previously though right? App
banners have always been frame bound because we previously called through via a
RenderFrameObserver talking directly to the frame.

Functionally, we shouldn't ever show a banner to a site incorrectly, because the
browser process wipes its state on navigations. The only risk is that we
navigate from one PWA to a second, the second one has a listener for
beforeinstallprompt, and we send that event mistakenly. I don't know how likely
that race is to occur (seems tricky to trigger it), but does this CL actually
increase the risk of that from before? And if not, can we investigate changing
that in a follow up?

[esprehn, 2016-10-25 03:17:43]

lgtm with some comments about OWNERS.

btw super excited to see us publishing a service from inside blink, that's very
cool.

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/AppBannerController.h (right):

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/AppBannerController.h:19: : public
mojom::blink::AppBannerController {
We'll probably want to add a way for Mojo services to be GarbageCollected. :)

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/OWNERS (right):

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/OWNERS:1: dominickn@chromium.org
Generally I've been telling people we're not going to add OWNERS to modules
unless they're familiar with blink and web platform. How comfortable are you
with WTF, Oilpan, and web platform process? :)

[dcheng, 2016-10-25 05:12:20]

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
&AppBannerController::bindMojoRequest, wrapWeakPersistent(frame())));
On 2016/10/24 23:17:58, dominickn wrote:
> On 2016/10/24 19:52:52, dcheng wrote:
> > I still feel pretty uncertain about this. From what I can tell, app banner
is
> > tied to the active document, and if we navigate, we want it go away. It's
hard
> > for me to tell if there's a possible race here:
> > 
> > 1. Browser decides to ask the renderer if an app banner should be shown.
> > 2. The renderer commits a navigation.
> > 3. Since the interface is frame-bound, we now dispatch the event on the
wrong
> > document.
> > 
> > If this was context-bound somehow, it'd be much easier to determine
> correctness.
> 
> My understanding is that this hasn't changed from previously though right? App
> banners have always been frame bound because we previously called through via
a
> RenderFrameObserver talking directly to the frame.

That's entirely possible.

> 
> Functionally, we shouldn't ever show a banner to a site incorrectly, because
the
> browser process wipes its state on navigations. The only risk is that we
> navigate from one PWA to a second, the second one has a listener for
> beforeinstallprompt, and we send that event mistakenly. I don't know how
likely
> that race is to occur (seems tricky to trigger it), but does this CL actually
> increase the risk of that from before? And if not, can we investigate changing
> that in a follow up?

I agree that we won't incorrectly show the banner.

However, my main concern is about setting a precedent that will be be difficult
to remove from Blink in the long run. If it's possible to start with
abstractions that make this class of error harder, that would really benefit all
the other Onion Soup work.

As for what I think we should do... I'm thinking that most of the current
InterfaceRegistry stuff on LocalFrame should actually be exposed via
ExecutionContext (?), and the pipes automatically closed when the
ExecutionContext is destroyed.

[Sam McNally, 2016-10-25 05:56:23]

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
&AppBannerController::bindMojoRequest, wrapWeakPersistent(frame())));
On 2016/10/25 05:12:20, dcheng wrote:
> On 2016/10/24 23:17:58, dominickn wrote:
> > On 2016/10/24 19:52:52, dcheng wrote:
> > > I still feel pretty uncertain about this. From what I can tell, app banner
> is
> > > tied to the active document, and if we navigate, we want it go away. It's
> hard
> > > for me to tell if there's a possible race here:
> > > 
> > > 1. Browser decides to ask the renderer if an app banner should be shown.
> > > 2. The renderer commits a navigation.
> > > 3. Since the interface is frame-bound, we now dispatch the event on the
> wrong
> > > document.
> > > 
> > > If this was context-bound somehow, it'd be much easier to determine
> > correctness.
> > 
> > My understanding is that this hasn't changed from previously though right?
App
> > banners have always been frame bound because we previously called through
via
> a
> > RenderFrameObserver talking directly to the frame.
> 
> That's entirely possible.
> 
> > 
> > Functionally, we shouldn't ever show a banner to a site incorrectly, because
> the
> > browser process wipes its state on navigations. The only risk is that we
> > navigate from one PWA to a second, the second one has a listener for
> > beforeinstallprompt, and we send that event mistakenly. I don't know how
> likely
> > that race is to occur (seems tricky to trigger it), but does this CL
actually
> > increase the risk of that from before? And if not, can we investigate
changing
> > that in a follow up?
> 
> I agree that we won't incorrectly show the banner.
> 
> However, my main concern is about setting a precedent that will be be
difficult
> to remove from Blink in the long run. If it's possible to start with
> abstractions that make this class of error harder, that would really benefit
all
> the other Onion Soup work.
> 
> As for what I think we should do... I'm thinking that most of the current
> InterfaceRegistry stuff on LocalFrame should actually be exposed via
> ExecutionContext (?), and the pipes automatically closed when the
> ExecutionContext is destroyed.

I think I originally wanted to hang what was at the time a combined
InterfaceRegistry/InterfaceProvider off ExecutionContext (mostly so that code
running in either a worker or a frame could not have to worry about where to
make mojo requests). A fair amount of the time it does feel like a particular
execution context would be more appropriate than a frame, but I don't think
there's any counterpart on the browser side. There's nothing that supports
automatically closing all the pipes when an ExecutionContext is destroyed. For
that, whatever holds the mojo pipe handle needs to attach its lifetime to the
ExecutionContext.

[haraken, 2016-10-25 07:26:07]

On 2016/10/25 05:56:23, Sam McNally wrote:
>
https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
> &AppBannerController::bindMojoRequest, wrapWeakPersistent(frame())));
> On 2016/10/25 05:12:20, dcheng wrote:
> > On 2016/10/24 23:17:58, dominickn wrote:
> > > On 2016/10/24 19:52:52, dcheng wrote:
> > > > I still feel pretty uncertain about this. From what I can tell, app
banner
> > is
> > > > tied to the active document, and if we navigate, we want it go away.
It's
> > hard
> > > > for me to tell if there's a possible race here:
> > > > 
> > > > 1. Browser decides to ask the renderer if an app banner should be shown.
> > > > 2. The renderer commits a navigation.
> > > > 3. Since the interface is frame-bound, we now dispatch the event on the
> > wrong
> > > > document.
> > > > 
> > > > If this was context-bound somehow, it'd be much easier to determine
> > > correctness.
> > > 
> > > My understanding is that this hasn't changed from previously though right?
> App
> > > banners have always been frame bound because we previously called through
> via
> > a
> > > RenderFrameObserver talking directly to the frame.
> > 
> > That's entirely possible.
> > 
> > > 
> > > Functionally, we shouldn't ever show a banner to a site incorrectly,
because
> > the
> > > browser process wipes its state on navigations. The only risk is that we
> > > navigate from one PWA to a second, the second one has a listener for
> > > beforeinstallprompt, and we send that event mistakenly. I don't know how
> > likely
> > > that race is to occur (seems tricky to trigger it), but does this CL
> actually
> > > increase the risk of that from before? And if not, can we investigate
> changing
> > > that in a follow up?
> > 
> > I agree that we won't incorrectly show the banner.
> > 
> > However, my main concern is about setting a precedent that will be be
> difficult
> > to remove from Blink in the long run. If it's possible to start with
> > abstractions that make this class of error harder, that would really benefit
> all
> > the other Onion Soup work.
> > 
> > As for what I think we should do... I'm thinking that most of the current
> > InterfaceRegistry stuff on LocalFrame should actually be exposed via
> > ExecutionContext (?), and the pipes automatically closed when the
> > ExecutionContext is destroyed.
> 
> I think I originally wanted to hang what was at the time a combined
> InterfaceRegistry/InterfaceProvider off ExecutionContext (mostly so that code
> running in either a worker or a frame could not have to worry about where to
> make mojo requests). A fair amount of the time it does feel like a particular
> execution context would be more appropriate than a frame, but I don't think
> there's any counterpart on the browser side. There's nothing that supports
> automatically closing all the pipes when an ExecutionContext is destroyed. For
> that, whatever holds the mojo pipe handle needs to attach its lifetime to the
> ExecutionContext.

ExecutionContext::contextDestroyed and Frame::detach are called at the same
time. Can we close all the mojo pipes when Frame::detach is called (if you want
to keep attaching InterfaceRegistry/InterfaceProvider to Frame)?

In any case, the fact that workers don't have a concept that corresponds to
Frame make a bunch of things complicated. In long term, it would make sense to
introduce something like WorkerFrame for implementational convenience. (I'm not
saying we should support real Frames for workers.)

[dcheng, 2016-10-25 07:27:53]

On 2016/10/25 07:26:07, haraken wrote:
> On 2016/10/25 05:56:23, Sam McNally wrote:
> >
>
https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
> > &AppBannerController::bindMojoRequest, wrapWeakPersistent(frame())));
> > On 2016/10/25 05:12:20, dcheng wrote:
> > > On 2016/10/24 23:17:58, dominickn wrote:
> > > > On 2016/10/24 19:52:52, dcheng wrote:
> > > > > I still feel pretty uncertain about this. From what I can tell, app
> banner
> > > is
> > > > > tied to the active document, and if we navigate, we want it go away.
> It's
> > > hard
> > > > > for me to tell if there's a possible race here:
> > > > > 
> > > > > 1. Browser decides to ask the renderer if an app banner should be
shown.
> > > > > 2. The renderer commits a navigation.
> > > > > 3. Since the interface is frame-bound, we now dispatch the event on
the
> > > wrong
> > > > > document.
> > > > > 
> > > > > If this was context-bound somehow, it'd be much easier to determine
> > > > correctness.
> > > > 
> > > > My understanding is that this hasn't changed from previously though
right?
> > App
> > > > banners have always been frame bound because we previously called
through
> > via
> > > a
> > > > RenderFrameObserver talking directly to the frame.
> > > 
> > > That's entirely possible.
> > > 
> > > > 
> > > > Functionally, we shouldn't ever show a banner to a site incorrectly,
> because
> > > the
> > > > browser process wipes its state on navigations. The only risk is that we
> > > > navigate from one PWA to a second, the second one has a listener for
> > > > beforeinstallprompt, and we send that event mistakenly. I don't know how
> > > likely
> > > > that race is to occur (seems tricky to trigger it), but does this CL
> > actually
> > > > increase the risk of that from before? And if not, can we investigate
> > changing
> > > > that in a follow up?
> > > 
> > > I agree that we won't incorrectly show the banner.
> > > 
> > > However, my main concern is about setting a precedent that will be be
> > difficult
> > > to remove from Blink in the long run. If it's possible to start with
> > > abstractions that make this class of error harder, that would really
benefit
> > all
> > > the other Onion Soup work.
> > > 
> > > As for what I think we should do... I'm thinking that most of the current
> > > InterfaceRegistry stuff on LocalFrame should actually be exposed via
> > > ExecutionContext (?), and the pipes automatically closed when the
> > > ExecutionContext is destroyed.
> > 
> > I think I originally wanted to hang what was at the time a combined
> > InterfaceRegistry/InterfaceProvider off ExecutionContext (mostly so that
code
> > running in either a worker or a frame could not have to worry about where to
> > make mojo requests). A fair amount of the time it does feel like a
particular
> > execution context would be more appropriate than a frame, but I don't think
> > there's any counterpart on the browser side. There's nothing that supports
> > automatically closing all the pipes when an ExecutionContext is destroyed.
For
> > that, whatever holds the mojo pipe handle needs to attach its lifetime to
the
> > ExecutionContext.
> 
> ExecutionContext::contextDestroyed and Frame::detach are called at the same
> time. Can we close all the mojo pipes when Frame::detach is called (if you
want
> to keep attaching InterfaceRegistry/InterfaceProvider to Frame)?

contextDestroyed() is called more often though, since it's called every time a
provisional load commits.

> 
> In any case, the fact that workers don't have a concept that corresponds to
> Frame make a bunch of things complicated. In long term, it would make sense to
> introduce something like WorkerFrame for implementational convenience. (I'm
not
> saying we should support real Frames for workers.)

[haraken, 2016-10-25 07:30:44]

On 2016/10/25 07:27:53, dcheng wrote:
> On 2016/10/25 07:26:07, haraken wrote:
> > On 2016/10/25 05:56:23, Sam McNally wrote:
> > >
> >
>
https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1627:
> > > &AppBannerController::bindMojoRequest, wrapWeakPersistent(frame())));
> > > On 2016/10/25 05:12:20, dcheng wrote:
> > > > On 2016/10/24 23:17:58, dominickn wrote:
> > > > > On 2016/10/24 19:52:52, dcheng wrote:
> > > > > > I still feel pretty uncertain about this. From what I can tell, app
> > banner
> > > > is
> > > > > > tied to the active document, and if we navigate, we want it go away.
> > It's
> > > > hard
> > > > > > for me to tell if there's a possible race here:
> > > > > > 
> > > > > > 1. Browser decides to ask the renderer if an app banner should be
> shown.
> > > > > > 2. The renderer commits a navigation.
> > > > > > 3. Since the interface is frame-bound, we now dispatch the event on
> the
> > > > wrong
> > > > > > document.
> > > > > > 
> > > > > > If this was context-bound somehow, it'd be much easier to determine
> > > > > correctness.
> > > > > 
> > > > > My understanding is that this hasn't changed from previously though
> right?
> > > App
> > > > > banners have always been frame bound because we previously called
> through
> > > via
> > > > a
> > > > > RenderFrameObserver talking directly to the frame.
> > > > 
> > > > That's entirely possible.
> > > > 
> > > > > 
> > > > > Functionally, we shouldn't ever show a banner to a site incorrectly,
> > because
> > > > the
> > > > > browser process wipes its state on navigations. The only risk is that
we
> > > > > navigate from one PWA to a second, the second one has a listener for
> > > > > beforeinstallprompt, and we send that event mistakenly. I don't know
how
> > > > likely
> > > > > that race is to occur (seems tricky to trigger it), but does this CL
> > > actually
> > > > > increase the risk of that from before? And if not, can we investigate
> > > changing
> > > > > that in a follow up?
> > > > 
> > > > I agree that we won't incorrectly show the banner.
> > > > 
> > > > However, my main concern is about setting a precedent that will be be
> > > difficult
> > > > to remove from Blink in the long run. If it's possible to start with
> > > > abstractions that make this class of error harder, that would really
> benefit
> > > all
> > > > the other Onion Soup work.
> > > > 
> > > > As for what I think we should do... I'm thinking that most of the
current
> > > > InterfaceRegistry stuff on LocalFrame should actually be exposed via
> > > > ExecutionContext (?), and the pipes automatically closed when the
> > > > ExecutionContext is destroyed.
> > > 
> > > I think I originally wanted to hang what was at the time a combined
> > > InterfaceRegistry/InterfaceProvider off ExecutionContext (mostly so that
> code
> > > running in either a worker or a frame could not have to worry about where
to
> > > make mojo requests). A fair amount of the time it does feel like a
> particular
> > > execution context would be more appropriate than a frame, but I don't
think
> > > there's any counterpart on the browser side. There's nothing that supports
> > > automatically closing all the pipes when an ExecutionContext is destroyed.
> For
> > > that, whatever holds the mojo pipe handle needs to attach its lifetime to
> the
> > > ExecutionContext.
> > 
> > ExecutionContext::contextDestroyed and Frame::detach are called at the same
> > time. Can we close all the mojo pipes when Frame::detach is called (if you
> want
> > to keep attaching InterfaceRegistry/InterfaceProvider to Frame)?
> 
> contextDestroyed() is called more often though, since it's called every time a
> provisional load commits.

Yeah, makes sense. Also, Frame::detach() is not called when a window navigates,
but contextDestroyed() is called.

[dominickn, 2016-10-25 08:01:18]

Does this mean that the resolution on the frame vs. ExecutionContext argument
really can't be resolved here and needs some major refactoring to be feasible?
This CL has been a very long slog already. ;)

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/app_banner/OWNERS (right):

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/app_banner/OWNERS:1: dominickn@chromium.org
On 2016/10/25 03:17:43, esprehn wrote:
> Generally I've been telling people we're not going to add OWNERS to modules
> unless they're familiar with blink and web platform. How comfortable are you
> with WTF, Oilpan, and web platform process? :)

I own all the other banner code, and have made pretty much all the substantive
changes here over the last year. I feel comfortable with what comes in and out
of this module, and I keep up with blink-dev and the architectural changes and
processes. This CL is pretty cutting edge given that it's the first one to
expose a service from Blink. However, if you're strongly opposed I'll remove
myself before landing. :)

[haraken, 2016-10-25 08:06:31]

On 2016/10/25 08:01:18, dominickn wrote:
> Does this mean that the resolution on the frame vs. ExecutionContext argument
> really can't be resolved here and needs some major refactoring to be feasible?
> This CL has been a very long slog already. ;)

The Frame/ExecutionContext refactoring is a quarter of work, so we shouldn't
block this CL by that refactoring.

Specifically, what is missing from the current CL? The ability to close all mojo
pipes when contextDestroyed() is called?

[dcheng, 2016-10-26 05:05:27]

On 2016/10/25 08:06:31, haraken wrote:
> On 2016/10/25 08:01:18, dominickn wrote:
> > Does this mean that the resolution on the frame vs. ExecutionContext
argument
> > really can't be resolved here and needs some major refactoring to be
feasible?
> > This CL has been a very long slog already. ;)

Well, I think it's pretty clear that it would be ideal for this interface (and
probably others) to be associated with an ExecutionContext / Document rather
than a Frame, and automatically disconnected on navigation. For example, one
other area where it might be useful is to block loading during navigation: one
could imagine that the loader interfaces for the old Document are disconnected
while a document unloads, to avoid processing resource loads for an inactive
document.

> 
> The Frame/ExecutionContext refactoring is a quarter of work, so we shouldn't
> block this CL by that refactoring.
> 
> Specifically, what is missing from the current CL? The ability to close all
mojo
> pipes when contextDestroyed() is called?

Since we're porting this to new infrastructure, I don't feel super great about
moving ahead, because we're investing a lot of work into converting the IPCs to
Mojo, and then we'll have to come back and make a second pass to fix up all the
things that should have been context-associated instead of frame-associated. I'd
prefer to just have the right building blocks to start. It's a bit unfortunate
that this CL is exposing a lot of these different issues, but I think the
long-term benefit will be worth it.

[dominickn, 2016-10-26 05:33:19]

On 2016/10/26 05:05:27, dcheng wrote:
> On 2016/10/25 08:06:31, haraken wrote:
> > On 2016/10/25 08:01:18, dominickn wrote:
> > > Does this mean that the resolution on the frame vs. ExecutionContext
> argument
> > > really can't be resolved here and needs some major refactoring to be
> feasible?
> > > This CL has been a very long slog already. ;)
> 
> Well, I think it's pretty clear that it would be ideal for this interface (and
> probably others) to be associated with an ExecutionContext / Document rather
> than a Frame, and automatically disconnected on navigation. For example, one
> other area where it might be useful is to block loading during navigation: one
> could imagine that the loader interfaces for the old Document are disconnected
> while a document unloads, to avoid processing resource loads for an inactive
> document.
> 
> > 
> > The Frame/ExecutionContext refactoring is a quarter of work, so we shouldn't
> > block this CL by that refactoring.
> > 
> > Specifically, what is missing from the current CL? The ability to close all
> mojo
> > pipes when contextDestroyed() is called?
> 
> Since we're porting this to new infrastructure, I don't feel super great about
> moving ahead, because we're investing a lot of work into converting the IPCs
to
> Mojo, and then we'll have to come back and make a second pass to fix up all
the
> things that should have been context-associated instead of frame-associated.
I'd
> prefer to just have the right building blocks to start. It's a bit unfortunate
> that this CL is exposing a lot of these different issues, but I think the
> long-term benefit will be worth it.

Unless there is a concrete plan, a team commitment, and reasonable expectation
of success, I don't think it's appropriate to block on hypothetical refactoring
- particularly refactoring that is complex enough to take a quarter of work.

There has already been a substantial amount of work sunk into this CL from
multiple people. I'd be surprised if the additional work to fit in a
document/executionContext scoping wasn't trivial in comparison.

App banners are currently scoped to frames, and after conversion they are still
scoped to frames. We aren't regressing in any way that I can detect. So it
doesn't seem right that we should let this CL rot for an indeterminate amount of
time so that it can be made perfect in one go.

Additionally, if this push back is likely to happen for other features, we
should stop asking people to convert more features to Mojo.

[dcheng, 2016-10-26 06:03:25]

On 2016/10/26 05:33:19, dominickn wrote:
> On 2016/10/26 05:05:27, dcheng wrote:
> > On 2016/10/25 08:06:31, haraken wrote:
> > > On 2016/10/25 08:01:18, dominickn wrote:
> > > > Does this mean that the resolution on the frame vs. ExecutionContext
> > argument
> > > > really can't be resolved here and needs some major refactoring to be
> > feasible?
> > > > This CL has been a very long slog already. ;)
> > 
> > Well, I think it's pretty clear that it would be ideal for this interface
(and
> > probably others) to be associated with an ExecutionContext / Document rather
> > than a Frame, and automatically disconnected on navigation. For example, one
> > other area where it might be useful is to block loading during navigation:
one
> > could imagine that the loader interfaces for the old Document are
disconnected
> > while a document unloads, to avoid processing resource loads for an inactive
> > document.
> > 
> > > 
> > > The Frame/ExecutionContext refactoring is a quarter of work, so we
shouldn't
> > > block this CL by that refactoring.
> > > 
> > > Specifically, what is missing from the current CL? The ability to close
all
> > mojo
> > > pipes when contextDestroyed() is called?
> > 
> > Since we're porting this to new infrastructure, I don't feel super great
about
> > moving ahead, because we're investing a lot of work into converting the IPCs
> to
> > Mojo, and then we'll have to come back and make a second pass to fix up all
> the
> > things that should have been context-associated instead of frame-associated.
> I'd
> > prefer to just have the right building blocks to start. It's a bit
unfortunate
> > that this CL is exposing a lot of these different issues, but I think the
> > long-term benefit will be worth it.
> 
> Unless there is a concrete plan, a team commitment, and reasonable expectation
> of success, I don't think it's appropriate to block on hypothetical
refactoring
> - particularly refactoring that is complex enough to take a quarter of work.

I'm not talking about refactoring the rest of Blink to fix frame/context
distinctions there, which is clearly out of scope. I just want the (relatively
new) Mojo in Blink to express these concepts.

> 
> There has already been a substantial amount of work sunk into this CL from
> multiple people. I'd be surprised if the additional work to fit in a
> document/executionContext scoping wasn't trivial in comparison.

Simple strawman:
- ContextScopedInterfacePtr is an InterfacePtr wrapper which has-a/is-a
ContextLifecycleObserver and closes the message pipe on context destruction.
- ContextScopedBinding is a Binding which does the same thing.

Context scoped interfaces would be vended out by an InterfaceProvider attached
to ExecutionContext, and similarly, InterfaceRegistry on ExecutionContext would
create impls using a context scoped binding. (Maybe) the
InterfaceProvider/InterfaceRegistry on LocalFrame would be deprecated and
removed.

> 
> App banners are currently scoped to frames, and after conversion they are
still
> scoped to frames. We aren't regressing in any way that I can detect. So it
> doesn't seem right that we should let this CL rot for an indeterminate amount
of
> time so that it can be made perfect in one go.
> 
> Additionally, if this push back is likely to happen for other features, we
> should stop asking people to convert more features to Mojo.

[Sam McNally, 2016-10-26 06:34:39]

On 2016/10/26 06:03:25, dcheng wrote:
> On 2016/10/26 05:33:19, dominickn wrote:
> > On 2016/10/26 05:05:27, dcheng wrote:
> > > On 2016/10/25 08:06:31, haraken wrote:
> > > > On 2016/10/25 08:01:18, dominickn wrote:
> > > > > Does this mean that the resolution on the frame vs. ExecutionContext
> > > argument
> > > > > really can't be resolved here and needs some major refactoring to be
> > > feasible?
> > > > > This CL has been a very long slog already. ;)
> > > 
> > > Well, I think it's pretty clear that it would be ideal for this interface
> (and
> > > probably others) to be associated with an ExecutionContext / Document
rather
> > > than a Frame, and automatically disconnected on navigation. For example,
one
> > > other area where it might be useful is to block loading during navigation:
> one
> > > could imagine that the loader interfaces for the old Document are
> disconnected
> > > while a document unloads, to avoid processing resource loads for an
inactive
> > > document.
> > > 
> > > > 
> > > > The Frame/ExecutionContext refactoring is a quarter of work, so we
> shouldn't
> > > > block this CL by that refactoring.
> > > > 
> > > > Specifically, what is missing from the current CL? The ability to close
> all
> > > mojo
> > > > pipes when contextDestroyed() is called?
> > > 
> > > Since we're porting this to new infrastructure, I don't feel super great
> about
> > > moving ahead, because we're investing a lot of work into converting the
IPCs
> > to
> > > Mojo, and then we'll have to come back and make a second pass to fix up
all
> > the
> > > things that should have been context-associated instead of
frame-associated.
> > I'd
> > > prefer to just have the right building blocks to start. It's a bit
> unfortunate
> > > that this CL is exposing a lot of these different issues, but I think the
> > > long-term benefit will be worth it.
> > 
> > Unless there is a concrete plan, a team commitment, and reasonable
expectation
> > of success, I don't think it's appropriate to block on hypothetical
> refactoring
> > - particularly refactoring that is complex enough to take a quarter of work.
> 
> I'm not talking about refactoring the rest of Blink to fix frame/context
> distinctions there, which is clearly out of scope. I just want the (relatively
> new) Mojo in Blink to express these concepts.
> 
> > 
> > There has already been a substantial amount of work sunk into this CL from
> > multiple people. I'd be surprised if the additional work to fit in a
> > document/executionContext scoping wasn't trivial in comparison.
> 
> Simple strawman:
> - ContextScopedInterfacePtr is an InterfacePtr wrapper which has-a/is-a
> ContextLifecycleObserver and closes the message pipe on context destruction.
> - ContextScopedBinding is a Binding which does the same thing.

This feels a bit heavyweight compared to what it provides. Anything that wants
to be per-ExecutionContext could use ContextLifecycleObserver itself and drop
mojo connections (and perform other cleanup) as appropriate.
> 
> Context scoped interfaces would be vended out by an InterfaceProvider attached
> to ExecutionContext, and similarly, InterfaceRegistry on ExecutionContext
would
> create impls using a context scoped binding. (Maybe) the
> InterfaceProvider/InterfaceRegistry on LocalFrame would be deprecated and
> removed.

This sounds reasonable, but there isn't anything on the browser side for them to
be connected to. Without an ExecutionContext counterpart in the browser process,
this doesn't solve any problems beyond what using a ContextLifecycleObserver
would, and introducing such a concept would require a non-trivial restructuring
of the browser-side content APIs. Blocking mojo conversions on refactoring the
world seems excessive.
> 
> > 
> > App banners are currently scoped to frames, and after conversion they are
> still
> > scoped to frames. We aren't regressing in any way that I can detect. So it
> > doesn't seem right that we should let this CL rot for an indeterminate
amount
> of
> > time so that it can be made perfect in one go.
> > 
> > Additionally, if this push back is likely to happen for other features, we
> > should stop asking people to convert more features to Mojo.

[esprehn, 2016-10-26 06:54:31]

How is the browser side related here? I don't think we need to generalize
this to there yet.

Let's focus on automatically managing the lifetime of interface connections
inside blink. That seems pretty reasonable to me and I'd definitely prefer
we do this with a special Ptr than expect everyone to manually do it.
Processing responses on a shut down frame is probably always wrong, so
having the system manage this for you makes sense.

Given the number of features in blink, and the large number of teams that
own them, we need to manage the "right path" with code, asserts, and
patterns that guide you. We can't expect folks to use the
ContextLifecycleObservers and to know how to handle navigation. We have
lots of experience in modules where just letting every team do it doesn't
work. The standard library needs to do it, in this case that means
platform/mojo must handle the best practices.

I'm fine landing this patch for now since it's been hanging out so long,
but I also think the request from dcheng is reasonable and that as the
maintainers of this component it'd be really nice if you would work
together and implementation this better abstraction before moving on to
more IPCs after this. :)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-10-26 07:01:46]

How is the browser side related here? I don't think we need to generalize
this to there yet.

Let's focus on automatically managing the lifetime of interface connections
inside blink. That seems pretty reasonable to me and I'd definitely prefer
we do this with a special Ptr than expect everyone to manually do it.
Processing responses on a shut down frame is probably always wrong, so
having the system manage this for you makes sense.

Given the number of features in blink, and the large number of teams that
own them, we need to manage the "right path" with code, asserts, and
patterns that guide you. We can't expect folks to use the
ContextLifecycleObservers and to know how to handle navigation. We have
lots of experience in modules where just letting every team do it doesn't
work. The standard library needs to do it, in this case that means
platform/mojo must handle the best practices.

I'm fine landing this patch for now since it's been hanging out so long,
but I also think the request from dcheng is reasonable and that as the
maintainers of this component it'd be really nice if you would work
together and implementation this better abstraction before moving on to
more IPCs after this. :)

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2016-10-26 08:29:54]

On 2016/10/26 07:01:46, esprehn wrote:
> How is the browser side related here? I don't think we need to generalize
> this to there yet.
> 
> Let's focus on automatically managing the lifetime of interface connections
> inside blink. That seems pretty reasonable to me and I'd definitely prefer
> we do this with a special Ptr than expect everyone to manually do it.
> Processing responses on a shut down frame is probably always wrong, so
> having the system manage this for you makes sense.
> 
> Given the number of features in blink, and the large number of teams that
> own them, we need to manage the "right path" with code, asserts, and
> patterns that guide you. We can't expect folks to use the
> ContextLifecycleObservers and to know how to handle navigation. We have
> lots of experience in modules where just letting every team do it doesn't
> work. The standard library needs to do it, in this case that means
> platform/mojo must handle the best practices.
> 
> I'm fine landing this patch for now since it's been hanging out so long,
> but I also think the request from dcheng is reasonable and that as the
> maintainers of this component it'd be really nice if you would work
> together and implementation this better abstraction before moving on to
> more IPCs after this. :)

+1

[dominickn, 2016-10-26 08:49:27]

On 2016/10/26 07:01:46, esprehn wrote:
> How is the browser side related here? I don't think we need to generalize
> this to there yet.

My understanding is that the interfaces are exposed to the browser via a
RenderFrameHost, and we need a way for the browser to be able to look up an
interface on an ExecutionContext, not a Frame, in order to address the race
condition that dcheng initially brought up. sammc?
 
> Let's focus on automatically managing the lifetime of interface connections
> inside blink. That seems pretty reasonable to me and I'd definitely prefer
> we do this with a special Ptr than expect everyone to manually do it.
> Processing responses on a shut down frame is probably always wrong, so
> having the system manage this for you makes sense.
> 
> Given the number of features in blink, and the large number of teams that
> own them, we need to manage the "right path" with code, asserts, and
> patterns that guide you. We can't expect folks to use the
> ContextLifecycleObservers and to know how to handle navigation. We have
> lots of experience in modules where just letting every team do it doesn't
> work. The standard library needs to do it, in this case that means
> platform/mojo must handle the best practices.
> 
> I'm fine landing this patch for now since it's been hanging out so long,
> but I also think the request from dcheng is reasonable and that as the
> maintainers of this component it'd be really nice if you would work
> together and implementation this better abstraction before moving on to
> more IPCs after this. :)

This definitely sounds like a good plan, and I'm totally on board to make
changes in banners as these improvements come along. It just didn't quite make
sense to block this CL on them because whatever new changes are required
shouldn't be that complex on top of this.

I think we agree that more abstraction work is necessary to make Blink-exposed
services more sane. Like I said, perhaps we should discourage any other
browser<->Blink conversions until that plumbing is ready. Then app banners can
be updated as necessary, minimising the amount of churn and ensuring the best
environment possible for everyone else.

[dcheng, 2016-10-27 00:44:09]

I'm OK landing this and fixing the abstractions in a followup. Thanks.

mojo lgtm

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1626:
frame()->interfaceRegistry()->addInterface(WTF::bind(
Please add a TODO here that this interface should really be Document-scoped.

[dominickn, 2016-10-27 02:07:51]

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

[commit-bot: I haz the power, 2016-10-27 02:08:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-27 02:08:43]

Thanks everyone. :)

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2393513004/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1626:
frame()->interfaceRegistry()->addInterface(WTF::bind(
On 2016/10/27 00:44:09, dcheng wrote:
> Please add a TODO here that this interface should really be Document-scoped.

Done.

[commit-bot: I haz the power, 2016-10-27 04:34:25]

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

[commit-bot: I haz the power, 2016-10-27 04:34:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dominickn, 2016-10-27 23:10:24]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-10-27 23:10:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
rockot@chromium.org, benwells@chromium.org, mlamouri@chromium.org,
jochen@chromium.org, haraken@chromium.org, dfalcantara@chromium.org,
esprehn@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2393513004/#ps380001
(title: "Add TODO")

[commit-bot: I haz the power, 2016-10-27 23:11:23]

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, 2016-10-28 01:44:43]

Committed patchset #16 (id:380001)

[commit-bot: I haz the power, 2016-10-28 01:49:04]

Description was changed from

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo and moves
all banner code from chrome/renderer to Blink. It also substantially
improves the layout test coverage for the BeforeInstallPromptEvent,
fixing various renderer crash bugs that are covered by the tests.

The key change is that the browser-side AppBannerManager makes a Mojo
request to Blink, which is intercepted by the WebLocalFrame and rerouted
to the AppBannerController. The AppBannerManager passes a bound
InterfaceRequest/Ptr in the Mojo request, which are used to create a
bound BeforeInstallPromptEvent. This ensures that when a
BeforeInstallPromptEvent is created, it already has a browser connection,
and does not need another asynchronous (and possibly racey) call to
establish full two-way communication with the browser process.

Several files in Blink's modules/app_banner directories that were solely
required for the AppBannerClient layer in chrome/renderer are deleted.
The existing layout tests are simplified by removing request_ids and
eliminating the AppBannerClient from ContentRendererClients. This
requires a dependency on content/public/common from
components/test_runner to allow a shim mojom::AppBannerService to be
injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704,655877,655902
==========

to

==========
Convert app banners to use Mojo.

This CL converts the app banner system from Chrome IPC to Mojo and moves
all banner code from chrome/renderer to Blink. It also substantially
improves the layout test coverage for the BeforeInstallPromptEvent,
fixing various renderer crash bugs that are covered by the tests.

The key change is that the browser-side AppBannerManager makes a Mojo
request to Blink, which is intercepted by the WebLocalFrame and rerouted
to the AppBannerController. The AppBannerManager passes a bound
InterfaceRequest/Ptr in the Mojo request, which are used to create a
bound BeforeInstallPromptEvent. This ensures that when a
BeforeInstallPromptEvent is created, it already has a browser connection,
and does not need another asynchronous (and possibly racey) call to
establish full two-way communication with the browser process.

Several files in Blink's modules/app_banner directories that were solely
required for the AppBannerClient layer in chrome/renderer are deleted.
The existing layout tests are simplified by removing request_ids and
eliminating the AppBannerClient from ContentRendererClients. This
requires a dependency on content/public/common from
components/test_runner to allow a shim mojom::AppBannerService to be
injected.

dominickn@chromium.org is added to OWNERS for the Blink-side app
banner code.

BUG=499704,655877,655902

Committed: https://crrev.com/09f7b579849cc8725455d0468b03a0237a6b624d
Cr-Commit-Position: refs/heads/master@{#428238}
==========

[commit-bot: I haz the power, 2016-10-28 01:49:07]

Patchset 16 (id:??) landed as
https://crrev.com/09f7b579849cc8725455d0468b03a0237a6b624d
Cr-Commit-Position: refs/heads/master@{#428238}