Utilize PassRefPtr and C++ refs in more places in ShadowRoot code

Utilize PassRefPtr and C++ refs in more places in ShadowRoot code

R=tkent@chromium.org

[tkent, 2013-11-06 00:54:09]

lgtm

[commit-bot: I haz the power, 2013-11-06 00:54:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/59113007/1

[eseidel, 2013-11-06 01:11:16]

The names are all super confused in this code.  This is not happy times.

https://codereview.chromium.org/59113007/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/59113007/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:1737: return *shadowRoot;
Who's holding onto this root... Lets hope didAddUserAgentShadowRoot does...
otherwise this looks crashy.

https://codereview.chromium.org/59113007/diff/1/Source/core/dom/shadow/Elemen...
File Source/core/dom/shadow/ElementShadow.h (right):

https://codereview.chromium.org/59113007/diff/1/Source/core/dom/shadow/Elemen...
Source/core/dom/shadow/ElementShadow.h:54: PassRefPtr<ShadowRoot>
addShadowRoot(Element& shadowHost, ShadowRoot::ShadowRootType);
add methods generally take a PassRefPtr<>, not return one... Should this be
called create?

https://codereview.chromium.org/59113007/diff/1/Source/core/html/HTMLDetailsE...
File Source/core/html/HTMLDetailsElement.cpp (right):

https://codereview.chromium.org/59113007/diff/1/Source/core/html/HTMLDetailsE...
Source/core/html/HTMLDetailsElement.cpp:57: void
HTMLDetailsElement::didAddUserAgentShadowRoot(ShadowRoot& root)
Woh, so this is element-specific, and it's part of the contract that it
necessarily takes a ref?  If so, this really should be a PassRefPtr input?

[eseidel, 2013-11-06 01:11:55]

not lgtm.  I guess mostly because of larger naming problems, but this patch just
seems to make existing problems even more confusing in my eyes.

[commit-bot: I haz the power, 2013-11-06 01:50:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[adamk, 2013-11-06 02:01:38]

https://codereview.chromium.org/59113007/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/59113007/diff/1/Source/core/dom/Element.cpp#n...
Source/core/dom/Element.cpp:1737: return *shadowRoot;
On 2013/11/06 01:11:17, eseidel wrote:
> Who's holding onto this root... Lets hope didAddUserAgentShadowRoot does...
> otherwise this looks crashy.

addShadowRoot does (ShadowRoot is TreeShared). What interface would you prefer
for this method?

https://codereview.chromium.org/59113007/diff/1/Source/core/dom/shadow/Elemen...
File Source/core/dom/shadow/ElementShadow.h (right):

https://codereview.chromium.org/59113007/diff/1/Source/core/dom/shadow/Elemen...
Source/core/dom/shadow/ElementShadow.h:54: PassRefPtr<ShadowRoot>
addShadowRoot(Element& shadowHost, ShadowRoot::ShadowRootType);
On 2013/11/06 01:11:17, eseidel wrote:
> add methods generally take a PassRefPtr<>, not return one... Should this be
> called create?

It creates a shadow root and adds it to the shadowHost. I don't think
createShadowRoot is a better name for this method, based on its additive
behavior.

I changed it to return a PassRefPtr since its main caller is
Element::createShadowRoot (which itself returns a PassRefPtr). As noted above,
ShadowRoot is also kept alive by its connection to its host.

https://codereview.chromium.org/59113007/diff/1/Source/core/html/HTMLDetailsE...
File Source/core/html/HTMLDetailsElement.cpp (right):

https://codereview.chromium.org/59113007/diff/1/Source/core/html/HTMLDetailsE...
Source/core/html/HTMLDetailsElement.cpp:57: void
HTMLDetailsElement::didAddUserAgentShadowRoot(ShadowRoot& root)
On 2013/11/06 01:11:17, eseidel wrote:
> Woh, so this is element-specific, and it's part of the contract that it
> necessarily takes a ref?  If so, this really should be a PassRefPtr input?

How is this different than any other code that takes a raw pointer? I'm confused
what you're worried about.

[eseidel, 2013-11-06 02:03:32]

I'll be in SFO tomorrow morning.  If you're OK to wait that long we can just go
over this in person and you can sooth my confused little mind. :)

[adamk, 2013-11-06 02:04:27]

On 2013/11/06 02:03:32, eseidel wrote:
> I'll be in SFO tomorrow morning.  If you're OK to wait that long we can just
go
> over this in person and you can sooth my confused little mind. :)

Sure, this CL was meant as a cleanup. Happy to explain things to you tomorrow.

[eseidel, 2013-11-06 02:06:09]

https://codereview.chromium.org/59113007/diff/210001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/59113007/diff/210001/Source/core/dom/Element....
Source/core/dom/Element.cpp:1735: RefPtr<ShadowRoot> shadowRoot =
ensureShadow().addShadowRoot(*this, ShadowRoot::UserAgentShadowRoot);
If the Shadow is holding onto this as long as it's alive, why would it return a
PassRefPtr from add?  Why not just a raw pointer or a reference?

Why do we hold on to ShadowRoot here if it can't be deallocated by anything we
call?  We definitely don't want to be the last ref to this ShadowRoot or when we
return it it will be dead (since our RefPtr goes out of scope).

[Yuta Kitamura, 2013-11-06 02:08:06]

Adding hayato@ who should have more knowledge on this area

[adamk, 2013-11-06 02:12:10]

On 2013/11/06 02:08:06, Yuta Kitamura wrote:
> Adding hayato@ who should have more knowledge on this area

Ok, but at this point I think I'd rather drop the CL than waste anyone else's
time. For the record, my thought process went like this:

1. Huh, Element::createShadowRoot returns a PassRefPtr, where does it get that
ref?

2. Oh, from ElementShadow::addShadowRoot. But addShadowRoot is written oddly: it
holds a RefPtr and returns the return value of .get()

3. So I can both make ElementShadow::addShadowRoot do something that looks less
scary and save a bit of refcount churn.

4. But that does make Element::ensureUserAgentShadowRoot() look as dangerous as
it already was. Oh well, at least it's now clear that this thing is non-null.

That was it.

[esprehn, 2013-11-06 02:16:24]

On 2013/11/06 02:06:09, eseidel wrote:
> ...
> ensureShadow().addShadowRoot(*this, ShadowRoot::UserAgentShadowRoot);
> If the Shadow is holding onto this as long as it's alive, why would it return
a
> PassRefPtr from add?  Why not just a raw pointer or a reference?
> 
> Why do we hold on to ShadowRoot here if it can't be deallocated by anything we
> call?  We definitely don't want to be the last ref to this ShadowRoot or when
we
> return it it will be dead (since our RefPtr goes out of scope).

As the author of this code I'd prefer a reference as well. :) I intentionally
didn't use a smart ptr since the caller doesn't need to retain ownership and
making all callers of addShadowRoot() put the result into a RefPtr is silly.

I think Element::createShadowRoot should probably return a raw pointer actually.

[adamk, 2013-11-06 02:20:47]

On 2013/11/06 02:16:24, esprehn wrote:
> On 2013/11/06 02:06:09, eseidel wrote:
> > ...
> > ensureShadow().addShadowRoot(*this, ShadowRoot::UserAgentShadowRoot);
> > If the Shadow is holding onto this as long as it's alive, why would it
return
> a
> > PassRefPtr from add?  Why not just a raw pointer or a reference?
> > 
> > Why do we hold on to ShadowRoot here if it can't be deallocated by anything
we
> > call?  We definitely don't want to be the last ref to this ShadowRoot or
when
> we
> > return it it will be dead (since our RefPtr goes out of scope).
> 
> As the author of this code I'd prefer a reference as well. :) I intentionally
> didn't use a smart ptr since the caller doesn't need to retain ownership and
> making all callers of addShadowRoot() put the result into a RefPtr is silly.
> 
> I think Element::createShadowRoot should probably return a raw pointer
actually.

Actually, that was step 0 of my process, but the only caller is the bindings
which want a RefPtr anyway (and pass the reference on to toV8())