Presentation API: ignore page visibility changes if ExecutionContext is destoyed.

Presentation API: ignore page visibility changes if ExecutionContext is destoyed.

PresentationAvailability is listening to the page visibility but is using
the ExecutionContext to send messages to the controller. The ExecutionContext
might be destroyed before a page visibility change, for example, if the object
was created in a frame that has been removed from the frame tree.

BUG=526923, 526116
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201918

[mlamouri (slow - plz ping), 2015-09-07 15:44:56]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org, mfoltz@chromium.org

[whywhat, 2015-09-07 15:51:46]

lgtm with comments

https://codereview.chromium.org/1322493006/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailabilityTest.cpp (right):

https://codereview.chromium.org/1322493006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailabilityTest.cpp:45: const KURL url
= URLTestHelpers::toKURL("https://example.com");
nit: could be a const reference?

https://codereview.chromium.org/1322493006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailabilityTest.cpp:46:
ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState());
is it destroyed in take() when resolved?

[mlamouri (slow - plz ping), 2015-09-07 16:28:38]

Comments applied.

https://codereview.chromium.org/1322493006/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationAvailabilityTest.cpp (right):

https://codereview.chromium.org/1322493006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailabilityTest.cpp:45: const KURL url
= URLTestHelpers::toKURL("https://example.com");
On 2015/09/07 at 15:51:46, whywhat wrote:
> nit: could be a const reference?

I don't think so, toKURL() returns a new KURL object.

https://codereview.chromium.org/1322493006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationAvailabilityTest.cpp:46:
ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState());
On 2015/09/07 at 15:51:46, whywhat wrote:
> is it destroyed in take() when resolved?

I think it is GC'd. I've added Persistent<> to make the expected behaviour more
obvious.

[mlamouri (slow - plz ping), 2015-09-07 16:28:48]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-09-07 16:28:48]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1322493006/#ps20001
(title: "review comments")

[commit-bot: I haz the power, 2015-09-07 16:28:59]

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

[commit-bot: I haz the power, 2015-09-07 17:30:58]

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

[commit-bot: I haz the power, 2015-09-07 17:30:58]

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

[mlamouri (slow - plz ping), 2015-09-08 10:36:02]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-09-08 10:36:03]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1322493006/#ps40001
(title: "Add BLINK_PLATFORM_EXPORT to WebPresentationAvailabilityObserver")

[commit-bot: I haz the power, 2015-09-08 10:36:11]

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

[commit-bot: I haz the power, 2015-09-08 11:43:43]

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

[commit-bot: I haz the power, 2015-09-08 11:43:44]

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

[mlamouri (slow - plz ping), 2015-09-08 13:49:43]

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

[mlamouri (slow - plz ping), 2015-09-08 13:49:44]

Could you rubbestamp the Source/platform/ changes. I need that to fix the build
issue on Windows.

[Mike West, 2015-09-08 13:51:11]

On 2015/09/08 at 13:49:44, mlamouri wrote:
> Could you rubbestamp the Source/platform/ changes. I need that to fix the
build issue on Windows.

RS LGTM.

[mlamouri (slow - plz ping), 2015-09-08 13:54:23]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-09-08 13:54:23]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1322493006/#ps80001
(title: "forgot file")

[commit-bot: I haz the power, 2015-09-08 13:54:28]

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

[commit-bot: I haz the power, 2015-09-08 16:50:15]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201918

[mark a. foltz, 2015-09-08 20:01:12]

Belated LGTM

Thanks for writing the unit test to verify the fix.  I didn't quite follow the
comment in the cpp though.

https://codereview.chromium.org/1322493006/diff/80001/Source/platform/exporte...
File Source/platform/exported/WebPresentationAvailabilityObserver.cpp (right):

https://codereview.chromium.org/1322493006/diff/80001/Source/platform/exporte...
Source/platform/exported/WebPresentationAvailabilityObserver.cpp:12: //
unresolved symbol error when constructor/destructor's address is required.
I didn't quite follow this comment or the problem this is trying to solve.

I am not sure anything in Blink should be taking the address of the default
ctor/dtor, as these functions will be overridden by the embedder.  If you want
to get a handle to the real ctor/dtor, it seems like you need to implement a
wrapper that indirects through a pointer (and thus the vtable), similar to
struct DefaultDeleter in scoped_ptr.

At any rate, the module including .h should be able to take the address of the
default ctor/dtors declared there - as there is nothing defined in this cpp.

Maybe there is some linker magic that I'm missing :)

[mlamouri (slow - plz ping), 2015-09-08 21:54:00]

https://codereview.chromium.org/1322493006/diff/80001/Source/platform/exporte...
File Source/platform/exported/WebPresentationAvailabilityObserver.cpp (right):

https://codereview.chromium.org/1322493006/diff/80001/Source/platform/exporte...
Source/platform/exported/WebPresentationAvailabilityObserver.cpp:12: //
unresolved symbol error when constructor/destructor's address is required.
On 2015/09/08 at 20:01:12, mark a. foltz wrote:
> I didn't quite follow this comment or the problem this is trying to solve.
> 
> I am not sure anything in Blink should be taking the address of the default
ctor/dtor, as these functions will be overridden by the embedder.  If you want
to get a handle to the real ctor/dtor, it seems like you need to implement a
wrapper that indirects through a pointer (and thus the vtable), similar to
struct DefaultDeleter in scoped_ptr.
> 
> At any rate, the module including .h should be able to take the address of the
default ctor/dtors declared there - as there is nothing defined in this cpp.
> 
> Maybe there is some linker magic that I'm missing :)

This is coming from WebPresentationController.cpp which was created because of
recents changes coming from componetization. Without that file, some compilers
(ie. Windows) will not find the definition of ctor and dtor for
WebPresentationController.