Closed Bug 957652 Opened 6 years ago Closed 6 years ago

Replace the Attr element getters by something that returns an Element

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- wontfix

People

(Reporter: Ms2ger, Assigned: ehsan)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

Replace Attr::GetContentInternal(), nsIAttribute::GetContent(), and Attr::GetElement() by a non-virtual Element* nsIAttribute::GetElement(), I think. (Maybe implemented out-of-line in Attr.cpp.)
Attachment #8357527 - Flags: review?(bzbarsky)
Comment on attachment 8357527 [details] [diff] [review]
Replace the Attr element getters by something that returns an Element; r=bzbarsky

r=me
Attachment #8357527 - Flags: review?(bzbarsky) → review+
Comment on attachment 8357527 [details] [diff] [review]
Replace the Attr element getters by something that returns an Element; r=bzbarsky

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

::: content/base/src/Attr.cpp
@@ +131,4 @@
>  Element*
>  Attr::GetElement() const
>  {
> +  nsIContent* content = mAttrMap ? mAttrMap->GetContent() : nullptr;

Could do an early return for !mAttrMap
Whiteboard: [qa-]
I'm going to back this out as part of backing out bug 957431.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
There doesn't seem to be any reason to back this out...
(In reply to comment #7)
> There doesn't seem to be any reason to back this out...

Hmm, perhaps I'm misremembering, but I think you asked me to  do this in a followup right?
Sure, but this is an improvement regardless of the existence of ownerElement.
Ah, hmm, you're right indeed!
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Actually, I will back this out, because this builds heavily on top of bug 957431 and I don't have time to disentangle the two right now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/528cdf00642b
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WONTFIX
We need this backout because the backout of bug 957431 is built on top of it.
Attachment #8394997 - Flags: approval-mozilla-beta?
Attachment #8394997 - Flags: approval-mozilla-aurora?
Still something we should fix, even if you don't want to do it right now.
Assignee: ehsan → nobody
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Target Milestone: mozilla29 → ---
Blocks: 986857
(In reply to :Ms2ger from comment #14)
> Still something we should fix, even if you don't want to do it right now.

Filed bug 986857.  We're going to use this bug for the backout on branches, etc.
Assignee: nobody → ehsan
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WONTFIX
Eshan, is it normal that the patch did not land in m-c yet? (or this bug has not been updated) Thanks
Flags: needinfo?(ehsan)
(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Eshan

s/sh/hs/  :-)

> is it normal that the patch did not land in m-c yet? (or this bug has
> not been updated) Thanks

Hmm, the patch has definitely landed in m-c <http://hg.mozilla.org/mozilla-central/rev/528cdf00642b>, not sure why the bug is not marked as such.
Flags: needinfo?(ehsan)
Comment on attachment 8394997 [details] [diff] [review]
Backout patch for branches

Sorry Ehsan for your name :/
Approving then. Sorry about the delay.
Attachment #8394997 - Flags: approval-mozilla-beta?
Attachment #8394997 - Flags: approval-mozilla-beta+
Attachment #8394997 - Flags: approval-mozilla-aurora?
Attachment #8394997 - Flags: approval-mozilla-aurora+
No worries, thanks for the approval!
You need to log in before you can comment on or make changes to this bug.