562698 - Pass Elements to AttributeChanged

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

AttributeChanged can only happen for Elements, so we should pass those to it.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch a: nsIMutationObserver::AttributeChanged v1

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: nsNodeUtils::AttributeChanged v1

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 462852 [details] [diff] [review]
Patch a: nsIMutationObserver::AttributeChanged v1

General comment:  I'd prefer that we rename the argument to aElement in consumers...  Calling it aContent when it's an Element is a little silly.

If some of these would be nontrivial patches, then followup patches is good instead of rolling it into this one, though.

>+++ b/content/base/public/nsIMutationObserver.h
>-#ifndef nsIMutationObserver_h___
>-#define nsIMutationObserver_h___

Why this change?

>-{ 0x85eea794, 0xed8e, 0x4e1b, \
>-  { 0xa1, 0x28, 0xd0, 0x93, 0x00, 0xae, 0x51, 0xaa } }
>+{ 0xe49a4a59, 0xb302, 0x4ca3, \
>+  { 0xac, 0x2c, 0xd7, 0xd0, 0xef, 0x6f, 0x25, 0x39 } }

No need for that; this is a binary-compatible change at the moment.

>+++ b/content/base/src/nsContentList.cpp
>+nsContentList::AttributeChanged(nsIDocument *aDocument, Element* aContent,

Again, aElement; this applies to all the consumers.

>   NS_PRECONDITION(aContent->IsElement(), "Should be an element");

This assertion can obviously go away.

Was there a reason to not also patch AttributeWillChange?  If no, please fix it too.

Comment 4

[Boris Zbarsky [:bzbarsky]]

And nsCSSFrameConstructor::AttributeChanged/WillChange need fixing too, no?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 462854 [details] [diff] [review]
Part b: nsNodeUtils::AttributeChanged v1

r=bzbarsky

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: nsIMutationObserver::AttributeChanged v2

(In reply to comment #3)
> Comment on attachment 462852 [details] [diff] [review]
> Patch a: nsIMutationObserver::AttributeChanged v1
> 
> General comment:  I'd prefer that we rename the argument to aElement in
> consumers...  Calling it aContent when it's an Element is a little silly.
> 
> If some of these would be nontrivial patches, then followup patches is good
> instead of rolling it into this one, though.

OK. I wasn't sure about taking blame for those lines, but you're right.

> >+++ b/content/base/public/nsIMutationObserver.h
> >-#ifndef nsIMutationObserver_h___
> >-#define nsIMutationObserver_h___
> 
> Why this change?

As David Baron said in bug 514661 comment 25, all names with two consecutive underscores are reserved to the implementation. (Though he suggests a single trailing underscore, I'm not sure why.) I'll leave it for another bug, if you prefer.

> >-{ 0x85eea794, 0xed8e, 0x4e1b, \
> >-  { 0xa1, 0x28, 0xd0, 0x93, 0x00, 0xae, 0x51, 0xaa } }
> >+{ 0xe49a4a59, 0xb302, 0x4ca3, \
> >+  { 0xac, 0x2c, 0xd7, 0xd0, 0xef, 0x6f, 0x25, 0x39 } }
> 
> No need for that; this is a binary-compatible change at the moment.

OK.

> >+++ b/content/base/src/nsContentList.cpp
> >+nsContentList::AttributeChanged(nsIDocument *aDocument, Element* aContent,
> 
> Again, aElement; this applies to all the consumers.
> 
> >   NS_PRECONDITION(aContent->IsElement(), "Should be an element");
> 
> This assertion can obviously go away.

Done.

> Was there a reason to not also patch AttributeWillChange?  If no, please fix it
> too.

No, I'll write a patch, probably tomorrow.

(In reply to comment #4)
> And nsCSSFrameConstructor::AttributeChanged/WillChange need fixing too, no?

Fixed.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 462890 [details] [diff] [review]
Part a: nsIMutationObserver::AttributeChanged v2

> +++ b/layout/base/nsCSSFrameConstructor.h
>+  void AttributeChanged(mozilla::dom::Element* aElement,

That can just be |Element*|, without the prefix (see the typedef in this class).

r=me with that.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: AttributeWillChange v1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 463479 [details] [diff] [review]
Part c: AttributeWillChange v1

r=bzbarsky.  Followup on the css declaration thing?

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: nsIMutationObserver::AttributeChanged and nsCSSFrameConstructor::AttributeChanged [for checkin]

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: nsNodeUtils::AttributeChanged [for checkin]

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: AttributeWillChange [for checkin]

Comment 13

[Dão Gottwald [:dao]]

part a doesn't seem to apply cleanly anymore.

patching file layout/xul/base/src/tree/src/nsTreeContentView.cpp
Hunk #1 FAILED at 41

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: nsIMutationObserver::AttributeChanged and nsCSSFrameConstructor::AttributeChanged [for checkin, take 2]

Let's try this again.

Comment 15

[Dão Gottwald [:dao]]

next failure in part b:

patching file parser/html/nsHtml5TreeOperation.cpp
Hunk #1 FAILED at 54
1 out of 2 hunks FAILED -- saving rejects to file parser/html/nsHtml5TreeOperation.cpp.rej

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: nsNodeUtils::AttributeChanged [for checkin, take 2]

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: AttributeWillChange [for checkin, take 2]

These really should apply.

Comment 18

[Dão Gottwald [:dao]]

landed and backed out, failed to compile on OS X

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: AttributeWillChange [for checkin, take 3]

This one includes the bustage fix.

Comment 20

[Dão Gottwald [:dao]]

patching file layout/svg/base/src/nsSVGEffects.cpp
Hunk #1 FAILED at 166
1 out of 1 hunks FAILED -- saving rejects to file layout/svg/base/src/nsSVGEffects.cpp.rej

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: nsIMutationObserver::AttributeChanged and nsCSSFrameConstructor::AttributeChanged [for checkin, take 3]

Parts b and c still apply fine. Thanks a lot for your patience with me, Dão.

Comment 22

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/9933c41be195
http://hg.mozilla.org/mozilla-central/rev/715fc9643c0c
http://hg.mozilla.org/mozilla-central/rev/f203095c85de