[Chromecast] Clarify lifetime of CastGinRunner.

[Chromecast] Clarify lifetime of CastGinRunner.

CastGinRunner is now owned by the gin::PerContextData associated
with a content::RenderFrame. Do not rely on RenderFrameObserver
notifications to manager it's lifetime. Instead, this class will
be deleted when gin::PerContextData is destroyed.

BUG=internal b/32916396

R=slan@chromium.org,alokp@chromium.org

Committed: https://crrev.com/693c2b9347ccbd85043b9dff0cbc18824a14e7b5
Cr-Commit-Position: refs/heads/master@{#435640}

[derekjchow1, 2016-11-30 19:04:16]

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

[commit-bot: I haz the power, 2016-11-30 19:04:58]

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

[derekjchow1, 2016-11-30 19:06:57]

The following documentation might help clarify ownership questions you might
have.

https://cs.chromium.org/chromium/src/base/supports_user_data.h?l=34

[commit-bot: I haz the power, 2016-11-30 19:23:31]

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

[commit-bot: I haz the power, 2016-11-30 19:23:35]

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

[slan, 2016-11-30 19:26:09]

https://codereview.chromium.org/2535313004/diff/1/chromecast/renderer/cast_gi...
File chromecast/renderer/cast_gin_runner.cc (right):

https://codereview.chromium.org/2535313004/diff/1/chromecast/renderer/cast_gi...
chromecast/renderer/cast_gin_runner.cc:18: const void* kCastGinRunnerKey =
static_cast<const void*>(kCastGinRunnerKey);
kCastContextData? (with declaration above?)

https://codereview.chromium.org/2535313004/diff/1/chromecast/renderer/cast_gi...
chromecast/renderer/cast_gin_runner.cc:23: blink::WebFrame* frame =
render_frame->GetWebFrame();
nit: DCHECK(render_frame)

https://codereview.chromium.org/2535313004/diff/1/chromecast/renderer/cast_gi...
chromecast/renderer/cast_gin_runner.cc:38:
context_data->SetUserData(kCastGinRunnerKey, this);
Can you add a comment here that |context_data| takes ownership of |this|?

[derekjchow1, 2016-11-30 20:56:51]

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

[commit-bot: I haz the power, 2016-11-30 20:57:15]

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-11-30 21:20:38]

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

[commit-bot: I haz the power, 2016-11-30 21:20:39]

Dry run: This issue passed the CQ dry run.

[derekjchow1, 2016-11-30 22:32:34]

https://codereview.chromium.org/2535313004/diff/1/chromecast/renderer/cast_gi...
File chromecast/renderer/cast_gin_runner.cc (right):

https://codereview.chromium.org/2535313004/diff/1/chromecast/renderer/cast_gi...
chromecast/renderer/cast_gin_runner.cc:18: const void* kCastGinRunnerKey =
static_cast<const void*>(kCastGinRunnerKey);
On 2016/11/30 19:26:09, slan wrote:
> kCastContextData? (with declaration above?)

Done.

https://codereview.chromium.org/2535313004/diff/1/chromecast/renderer/cast_gi...
chromecast/renderer/cast_gin_runner.cc:23: blink::WebFrame* frame =
render_frame->GetWebFrame();
On 2016/11/30 19:26:09, slan wrote:
> nit: DCHECK(render_frame)

Done.

https://codereview.chromium.org/2535313004/diff/1/chromecast/renderer/cast_gi...
chromecast/renderer/cast_gin_runner.cc:38:
context_data->SetUserData(kCastGinRunnerKey, this);
On 2016/11/30 19:26:09, slan wrote:
> Can you add a comment here that |context_data| takes ownership of |this|?

Done.

[slan, 2016-12-01 01:33:22]

lgtm

https://codereview.chromium.org/2535313004/diff/20001/chromecast/renderer/cas...
File chromecast/renderer/cast_gin_runner.cc (right):

https://codereview.chromium.org/2535313004/diff/20001/chromecast/renderer/cas...
chromecast/renderer/cast_gin_runner.cc:19: const void* kCastGinRunnerKey =
static_cast<const void*>(&kCastContextData);
nit: Could you add a comment explaining that the key value must be a memory
address that is unique outside this file, but shared among all created instances
of CastGinRunner?

[slan, 2016-12-01 01:37:51]

https://codereview.chromium.org/2535313004/diff/20001/chromecast/renderer/cas...
File chromecast/renderer/cast_gin_runner.cc (right):

https://codereview.chromium.org/2535313004/diff/20001/chromecast/renderer/cas...
chromecast/renderer/cast_gin_runner.cc:19: const void* kCastGinRunnerKey =
static_cast<const void*>(&kCastContextData);
On 2016/12/01 01:33:22, slan wrote:
> nit: Could you add a comment explaining that the key value must be a memory
> address that is unique outside this file, but shared among all created
instances
> of CastGinRunner?
> 

Alternatively, you could also key on |render_frame| or the associated WebFrame*.
This would be an allocated address (and therefore unique) for the lifetime of
the CastGinRunner. And it would eliminate the need for this trick.

[alokp, 2016-12-01 05:15:31]

lgtm

[derekjchow1, 2016-12-01 17:34:08]

The CQ bit was checked by derekjchow@chromium.org

[commit-bot: I haz the power, 2016-12-01 17:34:34]

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-12-01 17:38:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480613648440510,
"parent_rev": "fe6adca87130fe258886b904395b5f925c74f6b3", "commit_rev":
"5aaf469fb3f35401a261fef617804c62f93f1dd0"}

[commit-bot: I haz the power, 2016-12-01 17:38:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-01 17:40:59]

Description was changed from

==========
[Chromecast] Clarify lifetime of CastGinRunner.

CastGinRunner is now owned by the gin::PerContextData associated
with a content::RenderFrame. Do not rely on RenderFrameObserver
notifications to manager it's lifetime. Instead, this class will
be deleted when gin::PerContextData is destroyed.

BUG=internal b/32916396

R=slan@chromium.org,alokp@chromium.org
==========

to

==========
[Chromecast] Clarify lifetime of CastGinRunner.

CastGinRunner is now owned by the gin::PerContextData associated
with a content::RenderFrame. Do not rely on RenderFrameObserver
notifications to manager it's lifetime. Instead, this class will
be deleted when gin::PerContextData is destroyed.

BUG=internal b/32916396

R=slan@chromium.org,alokp@chromium.org

Committed: https://crrev.com/693c2b9347ccbd85043b9dff0cbc18824a14e7b5
Cr-Commit-Position: refs/heads/master@{#435640}
==========

[commit-bot: I haz the power, 2016-12-01 17:41:00]

Patchset 2 (id:??) landed as
https://crrev.com/693c2b9347ccbd85043b9dff0cbc18824a14e7b5
Cr-Commit-Position: refs/heads/master@{#435640}