Status

()

enhancement
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(10 attachments)

5.99 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
3.94 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
5.68 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
5.13 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
10.62 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
3.00 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
4.23 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
14.55 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
8.23 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
22.43 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
There's no good reason for this to exist.
Depends on: 1431965
MozReview-Commit-ID: 5VTrCvHmMWi
Attachment #8944273 - Flags: review?(continuation)
MozReview-Commit-ID: G4HfD7ni9hd
Attachment #8944274 - Flags: review?(continuation)
MozReview-Commit-ID: 8eXx6IdmZ4W
Attachment #8944275 - Flags: review?(continuation)
MozReview-Commit-ID: 7l8CcT1ntL7
Attachment #8944276 - Flags: review?(continuation)
MozReview-Commit-ID: GnrOUhx9nTQ
Attachment #8944277 - Flags: review?(continuation)
MozReview-Commit-ID: 9GJmiVSI0bs
Attachment #8944278 - Flags: review?(continuation)
MozReview-Commit-ID: H8gyGnbqYtf
Attachment #8944279 - Flags: review?(continuation)
MozReview-Commit-ID: C7z0hcjC0Tg
Attachment #8944280 - Flags: review?(continuation)
MozReview-Commit-ID: 59TspEgvNRF
Attachment #8944281 - Flags: review?(continuation)
MozReview-Commit-ID: xj4QeXBF9V
Attachment #8944282 - Flags: review?(continuation)
Attachment #8944273 - Flags: review?(continuation) → review+
Attachment #8944274 - Flags: review?(continuation) → review+
Attachment #8944275 - Flags: review?(continuation) → review+
Thanks for splitting this up into small patches.
Blocks: 1132934
Attachment #8944276 - Flags: review?(continuation) → review+
Comment on attachment 8944277 [details] [diff] [review]
part 5.  Remove nsIDOMMozNamedAttrMap::Item

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

::: editor/libeditor/HTMLURIRefObject.cpp
@@ +91,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Loop over attribute list:
>    if (!mAttributes) {
> +    nsCOMPtr<dom::Element> element (do_QueryInterface(mNode));

nit: while you are here, maybe get rid of the space between element and (.

::: layout/inspector/inDOMView.cpp
@@ +1245,5 @@
>  
>    return NS_OK;
>  }
>  
>  nsresult

You should change the return type of this method to void. (It only returns NS_OK, and it is only called by inDOMView::GetChildNodesFor, which does not check the return value.)

::: layout/xul/nsBox.cpp
@@ +66,5 @@
>  
>      nsIContent* content = GetContent();
>  
>      // add on all the set attributes
> +    if (content && content->IsElement()) {

I guess this would have crashed with a null deref in the old code if a non-element ended up in here? (on namedMap->GetLength())
Attachment #8944277 - Flags: review?(continuation) → review+
Attachment #8944278 - Flags: review?(continuation) → review+
Attachment #8944279 - Flags: review?(continuation) → review+
Attachment #8944280 - Flags: review?(continuation) → review+
Attachment #8944281 - Flags: review?(continuation) → review+
Attachment #8944282 - Flags: review?(continuation) → review+
> nit: while you are here, maybe get rid of the space between element and (.

Done.

> You should change the return type of this method to void.

Done.

> I guess this would have crashed with a null deref in the old code if a non-element ended up in here?

Well, the old code doesn't even compile, since there is no GetAttributes on nsIDOMNode.  All this stuff in nsBox.cpp is inside #ifdef DEBUG_LAYOUT which we apparently never really define or something...
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22a83c38a0d7
part 1.  Remove the XPCOM versions of GetAttributeNode(NS).  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/aac9cf730d20
part 2.  Remove the XPCOM versions of createAttribute(NS).  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8149c85354f0
part 3.  Remove nsIDOMMozNamedAttrMap::GetNamedItem.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f42ff3c796bf
part 4.  Remove nsIDOMMozNamedAttrMap::GetNamedItemNS.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d7a4d867c95
part 5.  Remove nsIDOMMozNamedAttrMap::Item.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8a113c0c9c2
part 6.  Remove nsIDOMMozNamedAttrMap::GetLength.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/be8aacd49efe
part 7.  Remove unused nsIDOMMozNamedAttrMap methods.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/49ae9160211d
part 8.  Remove nsIDOMMozNamedAttrMap.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c93509796edc
part 9.  Stop using nsIDOMAttr in JS.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbea2cff1cc2
part 10. Remove nsIDOMAttr. r=mccr8
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.