clean up some temporary already_AddRefed variables

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
9 months ago
3 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

9 months ago
We shouldn't have these laying around.
Assignee

Comment 1

9 months ago
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)
Assignee

Comment 2

9 months ago
We need to disallow these to fix our static analysis, which should have
already been disallowing them.
Attachment #9010731 - Flags: review?(dd.mozilla)
Assignee

Comment 3

9 months ago
We need to disallow these to fix our static analysis, which should have
already been disallowing them.
Attachment #9010732 - Flags: review?(emilio)
Assignee

Comment 4

9 months ago
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

Comment 9

9 months ago
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.