Rewrite extensions console to not leak function templates.

Rewrite extensions console to not leak function templates.

With this CL, an ObjectTemplate is properly cached in the PerIsolateData, and
the templates for each function are only constructed once. For simplicity, the
level is stored in the function data (rather than a function), which removes
the need to have those functions out-of-line and should reduce both code and
V8 heap size (since small integers don't need a heap allocation).

BUG=696748
Review-Url: https://codereview.chromium.org/2718133003
Cr-Commit-Position: refs/heads/master@{#453399}
Committed: https://chromium.googlesource.com/chromium/src/+/b1ad11a47382c78676e8bc71a6201d47309a6b23

[jbroman, 2017-02-27 21:19:42]

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

[commit-bot: I haz the power, 2017-02-27 21:20:18]

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

[jbroman, 2017-02-27 21:28:32]

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

[commit-bot: I haz the power, 2017-02-27 21:28:50]

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

[jbroman, 2017-02-27 21:36:51]

Description was changed from

==========
Rewrite extensions console to use v8::FunctionTemplate properly.

BUG=
==========

to

==========
Rewrite extensions console to use not leak function templates.

With this CL, an ObjectTemplate is properly cached in the PerIsolateData, and
the templates for each function are only constructed once. For simplicity, the
level is stored in the function data (rather than a function), which removes
the need to have those functions out-of-line and should reduce both code and
V8 heap size (since small integers don't need a heap allocation).

BUG=
==========

[jbroman, 2017-02-27 21:37:00]

Description was changed from

==========
Rewrite extensions console to use not leak function templates.

With this CL, an ObjectTemplate is properly cached in the PerIsolateData, and
the templates for each function are only constructed once. For simplicity, the
level is stored in the function data (rather than a function), which removes
the need to have those functions out-of-line and should reduce both code and
V8 heap size (since small integers don't need a heap allocation).

BUG=
==========

to

==========
Rewrite extensions console to not leak function templates.

With this CL, an ObjectTemplate is properly cached in the PerIsolateData, and
the templates for each function are only constructed once. For simplicity, the
level is stored in the function data (rather than a function), which removes
the need to have those functions out-of-line and should reduce both code and
V8 heap size (since small integers don't need a heap allocation).

BUG=
==========

[jbroman, 2017-02-27 22:06:21]

Description was changed from

==========
Rewrite extensions console to not leak function templates.

With this CL, an ObjectTemplate is properly cached in the PerIsolateData, and
the templates for each function are only constructed once. For simplicity, the
level is stored in the function data (rather than a function), which removes
the need to have those functions out-of-line and should reduce both code and
V8 heap size (since small integers don't need a heap allocation).

BUG=
==========

to

==========
Rewrite extensions console to not leak function templates.

With this CL, an ObjectTemplate is properly cached in the PerIsolateData, and
the templates for each function are only constructed once. For simplicity, the
level is stored in the function data (rather than a function), which removes
the need to have those functions out-of-line and should reduce both code and
V8 heap size (since small integers don't need a heap allocation).

BUG=696748
==========

[jbroman, 2017-02-27 22:06:51]

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

[commit-bot: I haz the power, 2017-02-27 22:07:09]

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

[jbroman, 2017-02-27 22:09:23]

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

[jbroman, 2017-02-27 22:09:24]

Noticed this leak after you mentioned that this file existed on another CL.

This version should be shorter, faster and safer; WDYT?

Another version might be to use gin::Wrappable
(https://codereview.chromium.org/2720733004), but for an object this simple I
figured I'd stick with this approach (and it sidesteps some of the
CallbackHolder lifetime questions that exist elsewhere, since we only need an
integer level, whose lifetime is easy to manage).

[Devlin, 2017-02-27 22:32:12]

lgtm; thanks!

https://codereview.chromium.org/2718133003/diff/40001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/2718133003/diff/40001/extensions/renderer/con...
extensions/renderer/console.cc:57: script_context ?
script_context->GetRenderFrame() : nullptr;
I think we *should* be able to (D)CHECK() script_context here, but we don't have
to do that in this CL (in case for some reason we can't).  Maybe add a TODO?

https://codereview.chromium.org/2718133003/diff/40001/extensions/renderer/con...
File extensions/renderer/console.h (right):

https://codereview.chromium.org/2718133003/diff/40001/extensions/renderer/con...
extensions/renderer/console.h:27: inline void Debug(content::RenderFrame*
render_frame,
Given we now bind these to the same function and just use v8::Data to determine
the level, I'd say we can remove the Debug(), Log(), Warn(), Error()
implementations.  Two don't seem to be used and the other two are only used once
or twice - I think callers can just use AddMessage() and pass in the level.

If you don't want to handle that in this CL, though, that's fine (it is  a
little orthogonal).

[jbroman, 2017-02-27 22:50:54]

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

[commit-bot: I haz the power, 2017-02-27 22:51:22]

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

[jbroman, 2017-02-27 22:52:49]

https://codereview.chromium.org/2718133003/diff/40001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/2718133003/diff/40001/extensions/renderer/con...
extensions/renderer/console.cc:57: script_context ?
script_context->GetRenderFrame() : nullptr;
On 2017/02/27 at 22:32:12, Devlin wrote:
> I think we *should* be able to (D)CHECK() script_context here, but we don't
have to do that in this CL (in case for some reason we can't).  Maybe add a
TODO?

Added a TODO(devlin). I'm not familiar enough with ScriptContextSet off the top
of my head.

https://codereview.chromium.org/2718133003/diff/40001/extensions/renderer/con...
File extensions/renderer/console.h (right):

https://codereview.chromium.org/2718133003/diff/40001/extensions/renderer/con...
extensions/renderer/console.h:27: inline void Debug(content::RenderFrame*
render_frame,
On 2017/02/27 at 22:32:12, Devlin wrote:
> Given we now bind these to the same function and just use v8::Data to
determine the level, I'd say we can remove the Debug(), Log(), Warn(), Error()
implementations.  Two don't seem to be used and the other two are only used once
or twice - I think callers can just use AddMessage() and pass in the level.
> 
> If you don't want to handle that in this CL, though, that's fine (it is  a
little orthogonal).

I'd made it compile-time-inlinable, and I did notice how few call sites there
were. I've now inlined them at the source level. :D

[jbroman, 2017-02-27 22:54:43]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2017-02-27 22:54:44]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2017-02-27 22:54:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2718133003/#ps60001
(title: "devlin")

[commit-bot: I haz the power, 2017-02-27 22:55:02]

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

[commit-bot: I haz the power, 2017-02-27 23:59:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488236084660660,
"parent_rev": "6c1dadffa7a8dcb5479c9b259f059de6d99af8b4", "commit_rev":
"b1ad11a47382c78676e8bc71a6201d47309a6b23"}

[commit-bot: I haz the power, 2017-02-28 00:00:06]

Description was changed from

==========
Rewrite extensions console to not leak function templates.

With this CL, an ObjectTemplate is properly cached in the PerIsolateData, and
the templates for each function are only constructed once. For simplicity, the
level is stored in the function data (rather than a function), which removes
the need to have those functions out-of-line and should reduce both code and
V8 heap size (since small integers don't need a heap allocation).

BUG=696748
==========

to

==========
Rewrite extensions console to not leak function templates.

With this CL, an ObjectTemplate is properly cached in the PerIsolateData, and
the templates for each function are only constructed once. For simplicity, the
level is stored in the function data (rather than a function), which removes
the need to have those functions out-of-line and should reduce both code and
V8 heap size (since small integers don't need a heap allocation).

BUG=696748

Review-Url: https://codereview.chromium.org/2718133003
Cr-Commit-Position: refs/heads/master@{#453399}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1ad11a47382c78676e8bc71a620...
==========

[commit-bot: I haz the power, 2017-02-28 00:00:07]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b1ad11a47382c78676e8bc71a620...