Make ScriptStreamer and dependents Oilpan friendly.

Make ScriptStreamer and dependents Oilpan friendly.

As ScriptStreamer has a reference to a GCed ScriptResource, turn
ScriptStreamer into a GCed object also. Transitively, ScriptSourceCode
and other objects that refer to those need to be moved to the
Oilpan heap also.

R=haraken
BUG=340515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188816

[sof, 2015-01-21 15:11:53]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-01-21 15:11:53]

Please take a look.

(Not very high priority, but moving towards fewer off-heap is worthwhile.)

[haraken, 2015-01-21 15:25:16]

Looks nice; let me take a detailed look tomorrow morning.

[haraken, 2015-01-22 02:11:53]

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

[haraken, 2015-01-22 02:11:54]

LGTM with a comment.

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/PageScriptDebugServer.h (right):

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/PageScriptDebugServer.h:72: Nullable<ScriptSourceCode>
preprocess(LocalFrame*, const ScriptSourceCode&) override;

Using Nullable<> looks a bit tricky. At the moment, Nullable<> is just intended
to be used to represent nullability of IDL values and not intended to be used to
represent nullable C++ pointers (In Blink, we use a raw pointer for a nullable
C++ pointer and use a reference for a non-nullable C++ pointer).

Shall we change m_preprocessorSourceCode to ScriptSourceCode* and add
GC_PLUGIN_IGNORE? Or a better approach would be to make ScriptSourceCode
GarbageCollected instead of ALLOW_ONLY_INLINE_ALLOCATION. Then we can simply use
Member<ScriptSourceCode>. Given that ScriptSourceCode is instantiated only per
script, the overhead of making it on-heap allocated would be negligible.

[marja, 2015-01-22 08:21:56]

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

[marja, 2015-01-22 08:21:57]

I don't understand much about the Oilpan special types but...

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:415:
RefPtrWillBeRawPtr<ScriptStreamer> protect(this);
... and why is WillBeRawPtr correct? As this is only meant to protect the
ScriptStreamer, an a raw pointer won't protect anything, doesn't sound correct.
What will protect ScriptStremer in the future? (And then I guess that this
should just be removed.)

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:134: RawPtrWillBeMember<ScriptResource>
m_resource;
Why is the WillBeMember part correct? Will this really have a ScriptResource as
a member? How will that work?

[haraken, 2015-01-22 08:24:50]

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:415:
RefPtrWillBeRawPtr<ScriptStreamer> protect(this);
On 2015/01/22 08:21:57, marja wrote:
> ... and why is WillBeRawPtr correct? As this is only meant to protect the
> ScriptStreamer, an a raw pointer won't protect anything, doesn't sound
correct.
> What will protect ScriptStremer in the future? (And then I guess that this
> should just be removed.)

In oilpan, you don't need to protect it because conservative scanning will find
on-stack pointers automatically.

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:134: RawPtrWillBeMember<ScriptResource>
m_resource;
On 2015/01/22 08:21:57, marja wrote:
> Why is the WillBeMember part correct? Will this really have a ScriptResource
as
> a member? How will that work?

When an on-heap object X has a reference to another on-heap object Y, X uses
Member<Y>. Then Y is traced when X is traced.

[sof, 2015-01-22 08:37:41]

On 2015/01/22 02:11:54, haraken wrote:
> LGTM with a comment.
> 
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> File Source/bindings/core/v8/PageScriptDebugServer.h (right):
> 
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> Source/bindings/core/v8/PageScriptDebugServer.h:72: Nullable<ScriptSourceCode>
> preprocess(LocalFrame*, const ScriptSourceCode&) override;
> 
> Using Nullable<> looks a bit tricky. At the moment, Nullable<> is just
intended
> to be used to represent nullability of IDL values and not intended to be used
to
> represent nullable C++ pointers (In Blink, we use a raw pointer for a nullable
> C++ pointer and use a reference for a non-nullable C++ pointer).
> 

I'm going beyond the type's original purpose, but the need to represent
not-there/maybe values over 'value objects' isn't uncommon. I didn't want to
encode that in ScriptSourceCode directly.

Given the restricted use of Nullable<> here, its use (and overhead) seemed
reasonable.

> Shall we change m_preprocessorSourceCode to ScriptSourceCode* and add
> GC_PLUGIN_IGNORE? Or a better approach would be to make ScriptSourceCode
> GarbageCollected instead of ALLOW_ONLY_INLINE_ALLOCATION. Then we can simply
use
> Member<ScriptSourceCode>. Given that ScriptSourceCode is instantiated only per
> script, the overhead of making it on-heap allocated would be negligible.

I've gone back&forth a bit on that representation, exploring the option of
having ScriptSourceCode derive from GarbageCollected instead, being on the "heap
proper". It is really quite ugly to deal with, as all stack allocation uses turn
into heap allocations and need code transformation.

[haraken, 2015-01-22 08:45:11]

> > Shall we change m_preprocessorSourceCode to ScriptSourceCode* and add
> > GC_PLUGIN_IGNORE? Or a better approach would be to make ScriptSourceCode
> > GarbageCollected instead of ALLOW_ONLY_INLINE_ALLOCATION. Then we can simply
> use
> > Member<ScriptSourceCode>. Given that ScriptSourceCode is instantiated only
per
> > script, the overhead of making it on-heap allocated would be negligible.
> 
> I've gone back&forth a bit on that representation, exploring the option of
> having ScriptSourceCode derive from GarbageCollected instead, being on the
"heap
> proper". It is really quite ugly to deal with, as all stack allocation uses
turn
> into heap allocations and need code transformation.

Ah, makes sense...

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/PageScriptDebugServer.cpp:116:
visitor->trace(m_preprocessorSourceCode);

Just want to confirm, but does visitor->trace(m_preprocessorSourceCode) work as
expected? I mean, the ScriptSourceCode does not have a valid header, but won't
it cause an issue when Nullable<ScriptSourceCode> traces the ScriptSourceCode?

[sof, 2015-01-22 09:16:46]

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):

https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/PageScriptDebugServer.cpp:116:
visitor->trace(m_preprocessorSourceCode);
On 2015/01/22 08:45:11, haraken wrote:
> 
> Just want to confirm, but does visitor->trace(m_preprocessorSourceCode) work
as
> expected? I mean, the ScriptSourceCode does not have a valid header, but won't
> it cause an issue when Nullable<ScriptSourceCode> traces the ScriptSourceCode?

I think it will statically resolve to the trace() method used for part objects
(as ScriptSourceCode is a part object of Nullable<ScriptSourceCode>), but let me
verify.

[haraken, 2015-01-22 09:36:25]

On 2015/01/22 09:16:46, sof wrote:
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):
> 
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> Source/bindings/core/v8/PageScriptDebugServer.cpp:116:
> visitor->trace(m_preprocessorSourceCode);
> On 2015/01/22 08:45:11, haraken wrote:
> > 
> > Just want to confirm, but does visitor->trace(m_preprocessorSourceCode) work
> as
> > expected? I mean, the ScriptSourceCode does not have a valid header, but
won't
> > it cause an issue when Nullable<ScriptSourceCode> traces the
ScriptSourceCode?
> 
> I think it will statically resolve to the trace() method used for part objects
> (as ScriptSourceCode is a part object of Nullable<ScriptSourceCode>), but let
me
> verify.

Thanks. In the particular case of ScriptSourceCode, we'll be able to use
ScriptSourceCode::isEmpty to represent a null ScriptSourceCode. (That said, if
there are other cases that need similar nullability, I'm fine with starting
using Nullable for those cases.)

[sof, 2015-01-22 09:38:23]

On 2015/01/22 09:36:25, haraken wrote:
> On 2015/01/22 09:16:46, sof wrote:
> >
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> > File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> > Source/bindings/core/v8/PageScriptDebugServer.cpp:116:
> > visitor->trace(m_preprocessorSourceCode);
> > On 2015/01/22 08:45:11, haraken wrote:
> > > 
> > > Just want to confirm, but does visitor->trace(m_preprocessorSourceCode)
work
> > as
> > > expected? I mean, the ScriptSourceCode does not have a valid header, but
> won't
> > > it cause an issue when Nullable<ScriptSourceCode> traces the
> ScriptSourceCode?
> > 
> > I think it will statically resolve to the trace() method used for part
objects
> > (as ScriptSourceCode is a part object of Nullable<ScriptSourceCode>), but
let
> me
> > verify.
> 
> Thanks. In the particular case of ScriptSourceCode, we'll be able to use
> ScriptSourceCode::isEmpty to represent a null ScriptSourceCode. (That said, if
> there are other cases that need similar nullability, I'm fine with starting
> using Nullable for those cases.)

Is it clear that no-script-source === empty script ?

[haraken, 2015-01-22 09:41:27]

On 2015/01/22 09:38:23, sof wrote:
> On 2015/01/22 09:36:25, haraken wrote:
> > On 2015/01/22 09:16:46, sof wrote:
> > >
> >
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> > > File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> > > Source/bindings/core/v8/PageScriptDebugServer.cpp:116:
> > > visitor->trace(m_preprocessorSourceCode);
> > > On 2015/01/22 08:45:11, haraken wrote:
> > > > 
> > > > Just want to confirm, but does visitor->trace(m_preprocessorSourceCode)
> work
> > > as
> > > > expected? I mean, the ScriptSourceCode does not have a valid header, but
> > won't
> > > > it cause an issue when Nullable<ScriptSourceCode> traces the
> > ScriptSourceCode?
> > > 
> > > I think it will statically resolve to the trace() method used for part
> objects
> > > (as ScriptSourceCode is a part object of Nullable<ScriptSourceCode>), but
> let
> > me
> > > verify.
> > 
> > Thanks. In the particular case of ScriptSourceCode, we'll be able to use
> > ScriptSourceCode::isEmpty to represent a null ScriptSourceCode. (That said,
if
> > there are other cases that need similar nullability, I'm fine with starting
> > using Nullable for those cases.)
> 
> Is it clear that no-script-source === empty script ?

We can probably differentiate them by String::isEmpty and String::isNull (i.e.,
let ScriptSourceCode::m_source represent no-script-source/empty script/non-empty
script).

[sof, 2015-01-22 10:10:53]

On 2015/01/22 09:41:27, haraken wrote:
> On 2015/01/22 09:38:23, sof wrote:
> > On 2015/01/22 09:36:25, haraken wrote:
> > > On 2015/01/22 09:16:46, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> > > > File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> > > > Source/bindings/core/v8/PageScriptDebugServer.cpp:116:
> > > > visitor->trace(m_preprocessorSourceCode);
> > > > On 2015/01/22 08:45:11, haraken wrote:
> > > > > 
> > > > > Just want to confirm, but does
visitor->trace(m_preprocessorSourceCode)
> > work
> > > > as
> > > > > expected? I mean, the ScriptSourceCode does not have a valid header,
but
> > > won't
> > > > > it cause an issue when Nullable<ScriptSourceCode> traces the
> > > ScriptSourceCode?
> > > > 
> > > > I think it will statically resolve to the trace() method used for part
> > objects
> > > > (as ScriptSourceCode is a part object of Nullable<ScriptSourceCode>),
but
> > let
> > > me
> > > > verify.
> > > 
> > > Thanks. In the particular case of ScriptSourceCode, we'll be able to use
> > > ScriptSourceCode::isEmpty to represent a null ScriptSourceCode. (That
said,
> if
> > > there are other cases that need similar nullability, I'm fine with
starting
> > > using Nullable for those cases.)
> > 
> > Is it clear that no-script-source === empty script ?
> 
> We can probably differentiate them by String::isEmpty and String::isNull
(i.e.,
> let ScriptSourceCode::m_source represent no-script-source/empty
script/non-empty
> script).

Probably; to be safe, I think ScriptSourceCode(const String&) needs to map a
null string argument to an empty one internally.

[sof, 2015-01-22 12:02:48]

On 2015/01/22 10:10:53, sof wrote:
> On 2015/01/22 09:41:27, haraken wrote:
> > On 2015/01/22 09:38:23, sof wrote:
> > > On 2015/01/22 09:36:25, haraken wrote:
> > > > On 2015/01/22 09:16:46, sof wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> > > > > File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/847803002/diff/100001/Source/bindings/core/v8...
> > > > > Source/bindings/core/v8/PageScriptDebugServer.cpp:116:
> > > > > visitor->trace(m_preprocessorSourceCode);
> > > > > On 2015/01/22 08:45:11, haraken wrote:
> > > > > > 
> > > > > > Just want to confirm, but does
> visitor->trace(m_preprocessorSourceCode)
> > > work
> > > > > as
> > > > > > expected? I mean, the ScriptSourceCode does not have a valid header,
> but
> > > > won't
> > > > > > it cause an issue when Nullable<ScriptSourceCode> traces the
> > > > ScriptSourceCode?
> > > > > 
> > > > > I think it will statically resolve to the trace() method used for part
> > > objects
> > > > > (as ScriptSourceCode is a part object of Nullable<ScriptSourceCode>),
> but
> > > let
> > > > me
> > > > > verify.
> > > > 
> > > > Thanks. In the particular case of ScriptSourceCode, we'll be able to use
> > > > ScriptSourceCode::isEmpty to represent a null ScriptSourceCode. (That
> said,
> > if
> > > > there are other cases that need similar nullability, I'm fine with
> starting
> > > > using Nullable for those cases.)
> > > 
> > > Is it clear that no-script-source === empty script ?
> > 
> > We can probably differentiate them by String::isEmpty and String::isNull
> (i.e.,
> > let ScriptSourceCode::m_source represent no-script-source/empty
> script/non-empty
> > script).
> 
> Probably; to be safe, I think ScriptSourceCode(const String&) needs to map a
> null string argument to an empty one internally.

Now switched to that representation of null ScriptSourceCode values.

[haraken, 2015-01-22 13:38:36]

LGTM

https://codereview.chromium.org/847803002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/847803002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptSourceCode.h:61: bool isNull() const { return
m_source.isNull(); }

Shall we add a comment about the difference?

[sof, 2015-01-22 14:04:12]

Thanks for the review.

https://codereview.chromium.org/847803002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/847803002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptSourceCode.h:61: bool isNull() const { return
m_source.isNull(); }
On 2015/01/22 13:38:36, haraken wrote:
> 
> Shall we add a comment about the difference?

Added.

[sof, 2015-01-22 14:04:18]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-01-22 14:05:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/847803002/160001

[commit-bot: I haz the power, 2015-01-22 15:30:07]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188816