Pass Elements to AttributeChanged

RESOLVED FIXED in mozilla2.0b5

Status

()

P3
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bzbarsky, Assigned: Ms2ger)

Tracking

Trunk
mozilla2.0b5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

6.48 KB, patch
Details | Diff | Splinter Review
17.75 KB, patch
Details | Diff | Splinter Review
60.74 KB, patch
Details | Diff | Splinter Review
AttributeChanged can only happen for Elements, so we should pass those to it.
Priority: -- → P2
(Assignee)

Comment 1

8 years ago
Created attachment 462852 [details] [diff] [review]
Patch a: nsIMutationObserver::AttributeChanged v1
Assignee: bzbarsky → Ms2ger
Status: NEW → ASSIGNED
Attachment #462852 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

8 years ago
Created attachment 462854 [details] [diff] [review]
Part b: nsNodeUtils::AttributeChanged v1
Attachment #462854 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
OS: Mac OS X → All
Priority: P2 → P3
Hardware: x86 → All
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.
Attachment #462852 - Flags: review?(bzbarsky) → review-
And nsCSSFrameConstructor::AttributeChanged/WillChange need fixing too, no?
Comment on attachment 462854 [details] [diff] [review]
Part b: nsNodeUtils::AttributeChanged v1

r=bzbarsky
Attachment #462854 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 6

8 years ago
Created attachment 462890 [details] [diff] [review]
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.
Attachment #462852 - Attachment is obsolete: true
Attachment #462890 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #462854 - Attachment description: Part b: nsNodeUtils::AttributeChanged v2 → Part b: nsNodeUtils::AttributeChanged v1
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.
Attachment #462890 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

8 years ago
Created attachment 463479 [details] [diff] [review]
Part c: AttributeWillChange v1
Attachment #463479 - Flags: review?(bzbarsky)
Comment on attachment 463479 [details] [diff] [review]
Part c: AttributeWillChange v1

r=bzbarsky.  Followup on the css declaration thing?
Attachment #463479 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

8 years ago
Depends on: 585014
(Assignee)

Updated

8 years ago
Attachment #462854 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #462890 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #463479 - Flags: approval2.0?
Attachment #462854 - Flags: approval2.0? → approval2.0+
Attachment #462890 - Flags: approval2.0? → approval2.0+
Attachment #463479 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 10

8 years ago
Created attachment 463608 [details] [diff] [review]
Part a: nsIMutationObserver::AttributeChanged and nsCSSFrameConstructor::AttributeChanged [for checkin]
Attachment #462890 - Attachment is obsolete: true
(Assignee)

Comment 11

8 years ago
Created attachment 463610 [details] [diff] [review]
Part b: nsNodeUtils::AttributeChanged [for checkin]
Attachment #462854 - Attachment is obsolete: true
(Assignee)

Comment 12

8 years ago
Created attachment 463611 [details] [diff] [review]
Part c: AttributeWillChange [for checkin]
Attachment #463479 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
part a doesn't seem to apply cleanly anymore.

patching file layout/xul/base/src/tree/src/nsTreeContentView.cpp
Hunk #1 FAILED at 41
Keywords: checkin-needed
(Assignee)

Comment 14

8 years ago
Created attachment 463781 [details] [diff] [review]
Part a: nsIMutationObserver::AttributeChanged and nsCSSFrameConstructor::AttributeChanged [for checkin, take 2]

Let's try this again.
Attachment #463608 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
(Assignee)

Comment 16

8 years ago
Created attachment 464342 [details] [diff] [review]
Part b: nsNodeUtils::AttributeChanged [for checkin, take 2]
Attachment #463610 - Attachment is obsolete: true
(Assignee)

Comment 17

8 years ago
Created attachment 464343 [details] [diff] [review]
Part c: AttributeWillChange [for checkin, take 2]

These really should apply.
Attachment #463611 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
landed and backed out, failed to compile on OS X
Keywords: checkin-needed
(Assignee)

Comment 19

8 years ago
Created attachment 464393 [details] [diff] [review]
Part c: AttributeWillChange [for checkin, take 3]

This one includes the bustage fix.
Attachment #464343 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
(Assignee)

Comment 21

8 years ago
Created attachment 468311 [details] [diff] [review]
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.
Attachment #463781 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9933c41be195
http://hg.mozilla.org/mozilla-central/rev/715fc9643c0c
http://hg.mozilla.org/mozilla-central/rev/f203095c85de
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5

Updated

8 years ago
Blocks: 585437

Updated

8 years ago
No longer blocks: 585437
You need to log in before you can comment on or make changes to this bug.