Closed
Bug 1193572
Opened 8 years ago
Closed 8 years ago
More nsXULPrototypeElement::Deserialize() cleanups
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
9.25 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
2.47 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
These are all pretty simple. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe40a45f6e7
Attachment #8646654 -
Flags: review?(amarchesini)
Assignee | ||
Comment 2•8 years ago
|
||
I think this prevents a crash if we end up with a bogus cache value.
Attachment #8646655 -
Flags: review?(amarchesini)
Assignee | ||
Comment 3•8 years ago
|
||
These are already set in the constructor.
Attachment #8646656 -
Flags: review?(amarchesini)
Assignee | ||
Comment 4•8 years ago
|
||
Rather than keep around a raw pointer |script| and depend on the fact that |child| will keep alive the same object, make |script| strong, then pass the reference off to |child| when we're finished with it. I removed the comment about deleting |script| on failure because the existing stack smart pointers already are doing this. Also fix a tiny style nit.
Attachment #8646657 -
Flags: review?(amarchesini)
Comment 5•8 years ago
|
||
Comment on attachment 8646654 [details] [diff] [review] part 1 - Don't use return values of failing calls in XUL deserialize methods. Review of attachment 8646654 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xul/nsXULElement.cpp @@ +2277,5 @@ > > // Read Node Info > uint32_t number = 0; > nsresult rv = aStream->Read32(&number); > + if (NS_FAILED(rv)) return rv; if (NS_WARN_IF(NS_FAILED(rv))) return rv; @@ +2285,5 @@ > } > > // Read Attributes > + rv = aStream->Read32(&number); > + if (NS_FAILED(rv)) return rv; NS_WARN_IF @@ +2297,5 @@ > > nsAutoString attributeValue; > for (uint32_t i = 0; i < mNumAttributes; ++i) { > + rv = aStream->Read32(&number); > + if (NS_FAILED(rv)) return rv; ditto @@ +2306,5 @@ > > mAttributes[i].mName.SetTo(ni); > > + rv = aStream->ReadString(attributeValue); > + if (NS_FAILED(rv)) return rv; NS_WARN_IF @@ +2308,5 @@ > > + rv = aStream->ReadString(attributeValue); > + if (NS_FAILED(rv)) return rv; > + rv = SetAttrAt(i, attributeValue, aDocumentURI); > + if (NS_FAILED(rv)) return rv; NS_WARN_IF @@ +2313,5 @@ > } > } > > + rv = aStream->Read32(&number); > + if (NS_FAILED(rv)) return rv; NS_WARN_IF @@ +2321,5 @@ > mChildren.SetCapacity(numChildren); > > for (uint32_t i = 0; i < numChildren; i++) { > + rv = aStream->Read32(&number); > + if (NS_FAILED(rv)) return rv; NS_WARN_IF @@ +2333,5 @@ > child->mType = childType; > > + rv = child->Deserialize(aStream, aProtoDoc, aDocumentURI, > + aNodeInfos); > + if (NS_FAILED(rv)) return rv; ditto @@ +2341,5 @@ > child->mType = childType; > > + rv = child->Deserialize(aStream, aProtoDoc, aDocumentURI, > + aNodeInfos); > + if (NS_FAILED(rv)) return rv; s/NS_FAILED(rv)/NS_WARN_IF(NS_FAILED(rv))/ @@ +2839,5 @@ > nsXULPrototypeDocument* aProtoDoc, > nsIURI* aDocumentURI, > const nsTArray<nsRefPtr<mozilla::dom::NodeInfo>> *aNodeInfos) > { > + return aStream->ReadString(mValue); maybe here it's better to do: nsresult rv = aStream->ReadString(mValue); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } return NS_OK;
Attachment #8646654 -
Flags: review?(amarchesini) → review+
Updated•8 years ago
|
Attachment #8646655 -
Flags: review?(amarchesini) → review+
Comment 6•8 years ago
|
||
Comment on attachment 8646656 [details] [diff] [review] part 3 - Don't re-initialize mType for XUL proto elements. Review of attachment 8646656 [details] [diff] [review]: ----------------------------------------------------------------- what about if here http://mxr.mozilla.org/mozilla-central/source/dom/xul/nsXULElement.cpp#2409 you add: MOZ_ASSERT(child->mType == childType);
Attachment #8646656 -
Flags: review?(amarchesini) → review+
Updated•8 years ago
|
Attachment #8646657 -
Flags: review?(amarchesini) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/96f6f2f2ed32 https://hg.mozilla.org/integration/mozilla-inbound/rev/9b16a669ddc7 https://hg.mozilla.org/integration/mozilla-inbound/rev/35bb18f83595 https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd45ba4bf79
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #6) > MOZ_ASSERT(child->mType == childType); Added. I also added the WARN things all over the place.
Comment 9•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/96f6f2f2ed32 https://hg.mozilla.org/mozilla-central/rev/9b16a669ddc7 https://hg.mozilla.org/mozilla-central/rev/35bb18f83595 https://hg.mozilla.org/mozilla-central/rev/1fd45ba4bf79
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•