Closed Bug 1492894 Opened 2 years ago Closed 2 years ago

clean up some temporary already_AddRefed variables

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(4 files)

We shouldn't have these laying around.
Various places in dom/ use the pattern:

  already_AddRefed<NodeInfo> ni = ...;

which is supposed to be disallowed by our static analysis code, but
isn't, for whatever reason.  To fix our static analysis code, we need to
eliminate instances of the above pattern.

Unfortunately, eliminating this pattern requires restructuring how Nodes
are created.  Most Node subclasses take `already_AddRefed<NodeInfo>&` in
their constructors, and a few accept `already_AddRefed<NodeInfo>&&`.  We
need to enforce the latter pattern consistently, which requires changing
dozens of source files.
Attachment #9010729 - Flags: review?(continuation)
We need to disallow these to fix our static analysis, which should have
already been disallowing them.
Attachment #9010731 - Flags: review?(dd.mozilla)
We need to disallow these to fix our static analysis, which should have
already been disallowing them.
Attachment #9010732 - Flags: review?(emilio)
We need to disallow these to fix our static analysis, which should have
already been disallowing them.
Attachment #9010733 - Flags: review?(continuation)
Attachment #9010731 - Flags: review?(dd.mozilla) → review+
Comment on attachment 9010732 [details] [diff] [review]
part 3 - eliminate already_AddRefed variables in layout/

Review of attachment 9010732 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I had a very similar patch in bug 1472897 for the frame constructor ones :)

::: layout/base/nsCSSFrameConstructor.cpp
@@ +2504,5 @@
>      // here... but would probably be able to get away with less code in this
>      // function in general.
>      // Use a null PendingBinding, since our binding is not in fact pending.
>      static const FrameConstructionData rootSVGData = FCDATA_DECL(0, nullptr);
> +    RefPtr<ComputedStyle> sc(computedStyle);

Can you pass do_AddRef(computedStyle) instead, and remove this variable?

@@ +2558,2 @@
>      AutoFrameConstructionItem item(this, &rootTableData, aDocElement,
> +                                   nullptr, sc.forget(), true);

ditto.
Attachment #9010732 - Flags: review?(emilio) → review+
Comment on attachment 9010729 [details] [diff] [review]
part 1 - make the node hierarchy consistently constructed with NodeInfo&&

Review of attachment 9010729 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp.orig
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this

You should remove this file, probably. Looks a leftover from conflict resolution.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)
> Comment on attachment 9010729 [details] [diff] [review]
> part 1 - make the node hierarchy consistently constructed with NodeInfo&&
> 
> Review of attachment 9010729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDocument.cpp.orig
> @@ +1,4 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> 
> You should remove this file, probably. Looks a leftover from conflict
> resolution.

There's also dom/html/HTMLMediaElement.cpp.orig.
Comment on attachment 9010729 [details] [diff] [review]
part 1 - make the node hierarchy consistently constructed with NodeInfo&&

Review of attachment 9010729 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp.orig
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */

As Emilio said, please don't add this file.

::: dom/base/nsTextNode.cpp
@@ +62,2 @@
>      RefPtr<nsAttributeTextNode> it =
> +      new nsAttributeTextNode(ni.forget(), mNameSpaceID, mAttrName);

So you don't have to do move here, too?

::: dom/html/HTMLMediaElement.cpp.orig
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Another extra file that was likely added in error.
Attachment #9010729 - Flags: review?(continuation) → review+
Attachment #9010733 - Flags: review?(continuation) → review+
Priority: -- → P2
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14c288aec85
part 1 - make the node hierarchy consistently constructed with NodeInfo&&; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb0e08fb9530
part 2 - eliminate already_AddRefed variables in netwerk/; r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/0088198b811d
part 3 - eliminate already_AddRefed variables in layout/; r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/b186c6699920
part 4 - eliminate already_AddRefed variables in dom/; r=mccr8
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.