957652 - Replace the Attr element getters by something that returns an Element

Status: RESOLVED WONTFIX

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

Description

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

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.)

Comment 1

[(no longer active)]

Attachment: Replace the Attr element getters by something that returns an Element; r=bzbarsky

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8357527 [details] [diff] [review]
Replace the Attr element getters by something that returns an Element; r=bzbarsky

r=me

Comment 3

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

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

Comment 4

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d405c64a7bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/92efd61cfca5

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9d405c64a7bf
https://hg.mozilla.org/mozilla-central/rev/92efd61cfca5

Comment 6

[(no longer active)]

I'm going to back this out as part of backing out bug 957431.

Comment 7

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

There doesn't seem to be any reason to back this out...

Comment 8

[(no longer active)]

(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?

Comment 9

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

Sure, but this is an improvement regardless of the existence of ownerElement.

Comment 10

[(no longer active)]

Ah, hmm, you're right indeed!

Comment 11

[(no longer active)]

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.

Comment 12

[(no longer active)]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/528cdf00642b

Comment 13

[(no longer active)]

Attachment: Backout patch for branches

We need this backout because the backout of bug 957431 is built on top of it.

Comment 14

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

Still something we should fix, even if you don't want to do it right now.

Comment 15

[(no longer active)]

(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.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

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

Comment 17

[(no longer active)]

(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.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8394997 [details] [diff] [review]
Backout patch for branches

Sorry Ehsan for your name :/
Approving then. Sorry about the delay.

Comment 19

[(no longer active)]

No worries, thanks for the approval!

Comment 20

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8e00ee13c02
https://hg.mozilla.org/releases/mozilla-beta/rev/f076b01277e5