Closed
Bug 1492894
Opened 6 years ago
Closed 6 years ago
clean up some temporary already_AddRefed variables
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(4 files)
926.46 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
2.10 KB,
patch
|
dragana
:
review+
|
Details | Diff | Splinter Review |
12.82 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
2.57 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
We shouldn't have these laying around.
Assignee | ||
Comment 1•6 years 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•6 years 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•6 years 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•6 years ago
|
||
We need to disallow these to fix our static analysis, which should have already been disallowing them.
Attachment #9010733 -
Flags: review?(continuation)
Updated•6 years ago
|
Attachment #9010731 -
Flags: review?(dd.mozilla) → review+
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
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.
Comment 7•6 years ago
|
||
(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 8•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #9010733 -
Flags: review?(continuation) → review+
Updated•6 years ago
|
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
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a14c288aec85 https://hg.mozilla.org/mozilla-central/rev/bb0e08fb9530 https://hg.mozilla.org/mozilla-central/rev/0088198b811d https://hg.mozilla.org/mozilla-central/rev/b186c6699920
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•