Refactor BeforeInstallPromptEvent to use ScriptPromiseProperty

Refactor BeforeInstallPromptEvent to use ScriptPromiseProperty

This stops the event holding a ScriptPromise as a member, which may 
lead to a memory leak. Instead, ScriptPromiseProperty members are held
as members in the event. A WebCallbacks subclass is used to store a
pointer to the necessary ScriptPromiseProperty in the associated event,
with the interface exposed to Chromium. When a banner event is resolved,
the WebCallbacks implementation in modules/app_banner looks up the 
ScriptPromiseProperty and resolves its promise. This also makes
resolving the prompt promise simpler as well, as it occurs entirely in the
event.

This CL also removes the app-banner-event-prompt.html test from 
LeakExpectations, and fixes some bugs in the test relating to promise
resolution.

BUG=504675
Committed: https://crrev.com/8a7809f1e7eef498eab9f99f15e53a876b0ae9be
git-svn-id: svn://svn.chromium.org/blink/trunk@199472 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[dominickn, 2015-07-23 04:11:29]

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

[dominickn, 2015-07-23 04:12:50]

mlamouri: PTAL. I'm not 100% sure that I'm handling memory correctly, since I'm
still a bit unfamiliar with blink's smart pointers. Thanks for all the help so
far on this.

[dominickn, 2015-07-23 06:29:54]

Patchset #4 (id:50001) has been deleted

[dominickn, 2015-07-23 06:30:04]

Patchset #4 (id:70001) has been deleted

[dominickn, 2015-07-23 06:30:09]

Patchset #4 (id:90001) has been deleted

[dominickn, 2015-07-23 06:30:16]

Patchset #4 (id:110001) has been deleted

[yhirano, 2015-07-23 12:25:01]

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

[yhirano, 2015-07-23 12:25:03]

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerCallbacks.h (right):

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:23: BeforeInstallPromptEvent*
m_event;
Persistent<BeforeInstallPromptEvent>?

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:23: BeforeInstallPromptEvent*
m_event;
[optional] It would be possible to have the ScriptPromiseProperty instead of
BeforeInstallPromptEvent.

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerController.cpp (right):

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/AppBannerController.cpp:33:
RefPtrWillBeRawPtr<BeforeInstallPromptEvent> event =
BeforeInstallPromptEvent::create(EventTypeNames::beforeinstallprompt,
scriptState, wtfPlatforms, requestId, client);
IIUC You don't need ScriptState. You only need ExecutionContext.

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
File Source/modules/app_banner/BeforeInstallPromptEvent.h (right):

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/BeforeInstallPromptEvent.h:47: 
+ trace

[dominickn, 2015-07-23 13:13:48]

Thanks for the feedback!

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerCallbacks.h (right):

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:23: BeforeInstallPromptEvent*
m_event;
On 2015/07/23 12:25:02, yhirano wrote:
> [optional] It would be possible to have the ScriptPromiseProperty instead of
> BeforeInstallPromptEvent.

Done.

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:23: BeforeInstallPromptEvent*
m_event;
On 2015/07/23 12:25:02, yhirano wrote:
> Persistent<BeforeInstallPromptEvent>?

Done.

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerController.cpp (right):

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/AppBannerController.cpp:33:
RefPtrWillBeRawPtr<BeforeInstallPromptEvent> event =
BeforeInstallPromptEvent::create(EventTypeNames::beforeinstallprompt,
scriptState, wtfPlatforms, requestId, client);
On 2015/07/23 12:25:02, yhirano wrote:
> IIUC You don't need ScriptState. You only need ExecutionContext.

Done.

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
File Source/modules/app_banner/BeforeInstallPromptEvent.h (right):

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/BeforeInstallPromptEvent.h:47: 
On 2015/07/23 12:25:02, yhirano wrote:
> + trace

Done.

[yhirano, 2015-07-24 03:41:17]

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerController.cpp (right):

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/AppBannerController.cpp:33:
RefPtrWillBeRawPtr<BeforeInstallPromptEvent> event =
BeforeInstallPromptEvent::create(EventTypeNames::beforeinstallprompt,
scriptState, wtfPlatforms, requestId, client);
On 2015/07/23 13:13:48, dominickn wrote:
> On 2015/07/23 12:25:02, yhirano wrote:
> > IIUC You don't need ScriptState. You only need ExecutionContext.
> 
> Done.

You can use frame->document() if it is the right ExecutionContext.

https://codereview.chromium.org/1247283004/diff/190001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerCallbacks.h (right):

https://codereview.chromium.org/1247283004/diff/190001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:15:
AppBannerCallbacks(BeforeInstallPromptEvent::UserChoiceProperty*);
+explicit

https://codereview.chromium.org/1247283004/diff/190001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:16: virtual ~AppBannerCallbacks()
{ }
"~AppBannerCallbacks() override" is more preferable.

[dominickn, 2015-07-24 05:02:09]

Thanks again!

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerController.cpp (right):

https://codereview.chromium.org/1247283004/diff/110002/Source/modules/app_ban...
Source/modules/app_banner/AppBannerController.cpp:33:
RefPtrWillBeRawPtr<BeforeInstallPromptEvent> event =
BeforeInstallPromptEvent::create(EventTypeNames::beforeinstallprompt,
scriptState, wtfPlatforms, requestId, client);
On 2015/07/24 03:41:16, yhirano wrote:
> On 2015/07/23 13:13:48, dominickn wrote:
> > On 2015/07/23 12:25:02, yhirano wrote:
> > > IIUC You don't need ScriptState. You only need ExecutionContext.
> > 
> > Done.
> 
> You can use frame->document() if it is the right ExecutionContext.

Done.

https://codereview.chromium.org/1247283004/diff/190001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerCallbacks.h (right):

https://codereview.chromium.org/1247283004/diff/190001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:15:
AppBannerCallbacks(BeforeInstallPromptEvent::UserChoiceProperty*);
On 2015/07/24 03:41:16, yhirano wrote:
> +explicit

Done.

https://codereview.chromium.org/1247283004/diff/190001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:16: virtual ~AppBannerCallbacks()
{ }
On 2015/07/24 03:41:16, yhirano wrote:
> "~AppBannerCallbacks() override" is more preferable.

Done.

[yhirano, 2015-07-24 05:48:15]

lgtm

[mlamouri (slow - plz ping), 2015-07-24 15:08:25]

lgtm with comments applied.

Thanks for fixing this! :)

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerCallbacks.cpp (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.cpp:18:
m_userChoice->resolve(AppBannerPromptResult::take(ownPtr.release()));
There is no way to do that in one line? (ie. not have to create an OwnPtr<> on
the stack in order to pass a PassOwnPtr<>?

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerCallbacks.h (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:9: #include
"public/platform/modules/app_banner/WebAppBannerClient.h"
I don't think you need to include WebAppBannerClient.h

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:16: ~AppBannerCallbacks()
override { }
= default;

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerPromptResult.cpp (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerPromptResult.cpp:19: AppBannerPromptResult*
AppBannerPromptResult::take(PassOwnPtr<WebAppBannerPromptResult> webInstance)
ditto, shouldn't you remove the other take method?

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerPromptResult.h (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerPromptResult.h:26: static
AppBannerPromptResult* take(PassOwnPtr<WebAppBannerPromptResult> webInstance);
Do you need to keep the other take method? Same for the typedef.

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/BeforeInstallPromptEvent.cpp:77: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(InvalidStateError, "The prompt() method cannot be called on
this event."));
Linus Torvalds once said "If you need more than 3 levels of indentation, you're
screwed". You have 4 ;)

Maybe you can do:
if (!m_prompt)
    return ScriptPromise::reject....

if (m_prompt->state() != PromptPropertyPending)
    return m_prompt->promise(scriptState->world();

if (!m_client || !defaultPrevented() || m_redispatched) {
    m_prompt->reject(DOMException::create(InvalidStateError, ...
    return m_prompt->promise(scriptState->world();
}

// the rest of the code.

[dominickn, 2015-07-24 23:53:36]

Thanks for all the help. I actually understand the promise implementation much
better now. :)

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerCallbacks.cpp (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.cpp:18:
m_userChoice->resolve(AppBannerPromptResult::take(ownPtr.release()));
On 2015/07/24 15:08:25, Mounir Lamouri wrote:
> There is no way to do that in one line? (ie. not have to create an OwnPtr<> on
> the stack in order to pass a PassOwnPtr<>?

Done.

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerCallbacks.h (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:9: #include
"public/platform/modules/app_banner/WebAppBannerClient.h"
On 2015/07/24 15:08:25, Mounir Lamouri wrote:
> I don't think you need to include WebAppBannerClient.h

It's needed for WebAppBannerCallbacks

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerCallbacks.h:16: ~AppBannerCallbacks()
override { }
On 2015/07/24 15:08:25, Mounir Lamouri wrote:
> = default;

Done.

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerPromptResult.cpp (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerPromptResult.cpp:19: AppBannerPromptResult*
AppBannerPromptResult::take(PassOwnPtr<WebAppBannerPromptResult> webInstance)
On 2015/07/24 15:08:25, Mounir Lamouri wrote:
> ditto, shouldn't you remove the other take method?

Done.

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/AppBannerPromptResult.h (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/AppBannerPromptResult.h:26: static
AppBannerPromptResult* take(PassOwnPtr<WebAppBannerPromptResult> webInstance);
On 2015/07/24 15:08:25, Mounir Lamouri wrote:
> Do you need to keep the other take method? Same for the typedef.

Done.

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right):

https://codereview.chromium.org/1247283004/diff/230001/Source/modules/app_ban...
Source/modules/app_banner/BeforeInstallPromptEvent.cpp:77: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(InvalidStateError, "The prompt() method cannot be called on
this event."));
On 2015/07/24 15:08:25, Mounir Lamouri wrote:
> Linus Torvalds once said "If you need more than 3 levels of indentation,
you're
> screwed". You have 4 ;)
> 
> Maybe you can do:
> if (!m_prompt)
>     return ScriptPromise::reject....
> 
> if (m_prompt->state() != PromptPropertyPending)
>     return m_prompt->promise(scriptState->world();
> 
> if (!m_client || !defaultPrevented() || m_redispatched) {
>     m_prompt->reject(DOMException::create(InvalidStateError, ...
>     return m_prompt->promise(scriptState->world();
> }
> 
> // the rest of the code.

Done, and I even removed m_redispatched because the promise check should be all
that's required here.

[dominickn, 2015-07-24 23:55:11]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2015-07-24 23:55:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1247283004/#ps250001
(title: "Addressing reviewer comments")

[commit-bot: I haz the power, 2015-07-24 23:55:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1247283004/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1247283004/250001

[commit-bot: I haz the power, 2015-07-25 00:55:50]

Committed patchset #11 (id:250001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199472

[sof, 2015-07-29 15:23:48]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-07-29 15:23:49]

This CL has flaky trybot crashes against its own
app_banner/app-banner-event-prompt.html test which can also be seen on trunk
after it landed,


http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=app...

[sof, 2015-07-29 15:47:58]

A revert of this CL (patchset #11 id:250001) has been created in
https://codereview.chromium.org/1257823003/ by sigbjornf@opera.com.

The reason for reverting is: Causes app_banner/app-banner-event-prompt.html
failure,

 
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=app....

[yhirano, 2015-07-30 06:37:45]

Reopen, as this CL was reverted.

The test crashes because of an assertion failure at [1] called from [2]. A
callback is registered with (id = index = 0) in app-banner-event-dispatch.html
but the callback is not resolved nor cleared in the test because the event is
cancelled. After that app-banner-event-prompt.html runs in the same shell and it
tries to register a callback with (id = index = 0) and that causes the assertion
failure.
The behavior change was introduced in BeforeInstallpromptEvent.cpp and
AppBannerController.cpp: Before this CL, the callback was registered after
|userChoice| was accessed. After this CL, the callback is registered when
willShowInstallBannerPrompt is called.

I read [3]. I'm not 100% sure, but It looks the callbacks should be registered
before showing a prompt window, i.e. 
 - BeforeInstallPromptEvent was dispatched and the event was not default
prevented, or
 - prompt() was called correctly (i.e. the event was default prevented, prompt()
was called for the first time)
, is that correct?

1: https://code.google.com/p/chromium/codesearch#chromium/src/base/id_map.h&l=78

2:
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...
3:
https://github.com/slightlyoff/AppInstallImprovements/blob/master/explainer.md

[yhirano, 2015-07-30 06:43:32]

https://codereview.chromium.org/1247283004/diff/250001/Source/modules/app_ban...
File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right):

https://codereview.chromium.org/1247283004/diff/250001/Source/modules/app_ban...
Source/modules/app_banner/BeforeInstallPromptEvent.cpp:60: ScriptPromise
BeforeInstallPromptEvent::prompt(ScriptState* scriptState)
I think we don't need ScriptPromiseProperty for this function, given that this
implements an IDL function, not an IDL attribute (sorry I overlooked that).

[dominickn, 2015-08-03 00:44:05]

On 2015/07/30 06:37:45, yhirano wrote:
> Reopen, as this CL was reverted.
> 
> The test crashes because of an assertion failure at [1] called from [2]. A
> callback is registered with (id = index = 0) in app-banner-event-dispatch.html
> but the callback is not resolved nor cleared in the test because the event is
> cancelled. After that app-banner-event-prompt.html runs in the same shell and
it
> tries to register a callback with (id = index = 0) and that causes the
assertion
> failure.
> The behavior change was introduced in BeforeInstallpromptEvent.cpp and
> AppBannerController.cpp: Before this CL, the callback was registered after
> |userChoice| was accessed. After this CL, the callback is registered when
> willShowInstallBannerPrompt is called.
> 
> I read [3]. I'm not 100% sure, but It looks the callbacks should be registered
> before showing a prompt window, i.e. 
>  - BeforeInstallPromptEvent was dispatched and the event was not default
> prevented, or
>  - prompt() was called correctly (i.e. the event was default prevented,
prompt()
> was called for the first time)
> , is that correct?
> 
> 1:
https://code.google.com/p/chromium/codesearch#chromium/src/base/id_map.h&l=78
> 
> 2:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...
> 3:
> https://github.com/slightlyoff/AppInstallImprovements/blob/master/explainer.md

The main reason why I changed the order in which the callback was registered was
that
benwells would like to convert the banner architecture to use Mojo rather than
IPCs. This
requires the banner callbacks to be created synchronously. For now, I'm happy to
unroll
the synchronous callback registration and work out a better way of handling that
later.


> I think we don't need ScriptPromiseProperty for this function, given that this
> implements an IDL function, not an IDL attribute (sorry I overlooked that).

Do you mean that persisting the state of this promise isn't necessary, and it
can just return a new promise each time?

[yhirano, 2015-08-03 02:30:56]

On 2015/08/03 00:44:05, dominickn wrote:
> On 2015/07/30 06:37:45, yhirano wrote:

> > I think we don't need ScriptPromiseProperty for this function, given that
this
> > implements an IDL function, not an IDL attribute (sorry I overlooked that).
> 
> Do you mean that persisting the state of this promise isn't necessary, and it
> can just return a new promise each time?

Yes, if your API says nothing about it.
In general, a function returning a promise returns a new promise on every call.
If your API specifically says it returns the same promise the principle is
overwritten.

[dominickn, 2015-08-03 02:45:54]

On 2015/08/03 02:30:56, yhirano wrote:
> On 2015/08/03 00:44:05, dominickn wrote:
> > On 2015/07/30 06:37:45, yhirano wrote:
> 
> > > I think we don't need ScriptPromiseProperty for this function, given that
> this
> > > implements an IDL function, not an IDL attribute (sorry I overlooked
that).
> > 
> > Do you mean that persisting the state of this promise isn't necessary, and
it
> > can just return a new promise each time?
> 
> Yes, if your API says nothing about it.
> In general, a function returning a promise returns a new promise on every
call.
> If your API specifically says it returns the same promise the principle is
> overwritten.

Mounir suggested that a promise be returned when I first implemented prompt();
the API doesn't actually require a promise. Given this, I'll restore the
original
behaviour here as well.

[dominickn, 2015-08-03 04:15:03]

PTAL, thanks.

https://codereview.chromium.org/1247283004/diff/250001/Source/modules/app_ban...
File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right):

https://codereview.chromium.org/1247283004/diff/250001/Source/modules/app_ban...
Source/modules/app_banner/BeforeInstallPromptEvent.cpp:60: ScriptPromise
BeforeInstallPromptEvent::prompt(ScriptState* scriptState)
On 2015/07/30 06:43:31, yhirano wrote:
> I think we don't need ScriptPromiseProperty for this function, given that this
> implements an IDL function, not an IDL attribute (sorry I overlooked that).

Done.

[yhirano, 2015-08-03 04:32:12]

lgtm, thanks.

[dominickn, 2015-08-04 00:20:37]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2015-08-04 00:20:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1247283004/#ps270001
(title: "Make prompt() return a new promise each call, and restore async
callback registration.")

[commit-bot: I haz the power, 2015-08-04 00:20:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1247283004/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1247283004/270001

[commit-bot: I haz the power, 2015-08-04 00:24:15]

Committed patchset #12 (id:270001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199926

[commit-bot: I haz the power, 2015-09-23 11:47:16]

Patchset 12 (id:??) landed as
https://crrev.com/8a7809f1e7eef498eab9f99f15e53a876b0ae9be