Closed Bug 945517 Opened 6 years ago Closed 6 years ago

consider making nsIFrame::GetPseudoElementContent return Element* instead of nsIContent*

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: heycam, Assigned: heycam)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

bz suggests so in bug 944246 comment 2.
Attached patch patch (obsolete) — Splinter Review
Is it worth it?
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8341383 - Flags: review?(bzbarsky)
Comment on attachment 8341383 [details] [diff] [review]
patch

I think it is.

I think we should also just change all the members involved to be nsCOMPtr<mozilla::dom::Element>.  And fix NS_NewHTMLElement to have an Element** as the first argument.

I can totally understand if you don't want to deal with all that, but seems to me like we'd be in a better place once it's all done.
I figured NS_NewHTMLElement's signature was the reason they're stored as nsCOMPtr<nsIContent>s.  Changing it sounds reasonable.
Actually, why don't we just make the form control frames call NS_NewHTMLDivElement etc. rather than NS_NewHTMLElement?
It requires including nsGenericHTMLElement.h, iirc, and slightly duplicating the info about what you're creating, in the nodeinfo and in the function name, but yes, we could do that.

We could even expose a convenience method on nsIDocument to create an HTML element with the given tagname and return the Element (much like CreateElementNS but without the annoying string namespace).
Attached patch patch v2Splinter Review
OK how about this?
Attachment #8341383 - Attachment is obsolete: true
Attachment #8341383 - Flags: review?(bzbarsky)
Attachment #8341431 - Flags: review?(bzbarsky)
Comment on attachment 8341431 [details] [diff] [review]
patch v2

>+++ b/layout/forms/nsProgressFrame.cpp
>+  // Create the div.

Maybe "Create the progress bar div"?

We could still make NS_NewHTMLElement take an Element** and get rid of the nasty casting bits, but a followup for that is just fine.

And then we could even make GetPlaceholderNode return Element* as well...

Anyway, r=me on this as-is.  This looks like it applies on top of the first patch you posted here, so I should review that as well, right?
Attachment #8341431 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #7)
> We could still make NS_NewHTMLElement take an Element** and get rid of the
> nasty casting bits, but a followup for that is just fine.

I didn't look into it, but I wonder whether this is more difficult than it seems -- I think having the signature of |NS_NewHTMLElement| and all the other |NS_NewHTMLFooElement|s be the same might be important.

> And then we could even make GetPlaceholderNode return Element* as well...
> 
> Anyway, r=me on this as-is.  This looks like it applies on top of the first
> patch you posted here, so I should review that as well, right?

No, I folded the changes all together.
(In reply to Boris Zbarsky [:bz] from comment #7)
> We could still make NS_NewHTMLElement take an Element** and get rid of the
> nasty casting bits, but a followup for that is just fine.

Bug 945572.

> And then we could even make GetPlaceholderNode return Element* as well...

Bug 945573.
> I think having the signature of |NS_NewHTMLElement| and all the other
> |NS_NewHTMLFooElement|s be the same might be important.

There are two different NS_NewHTMLElement functions, with two different signatures.  One is declared in nsGenericHTMLElement.h and has this signature:

 nsGenericHTMLElement*
 NS_NewHTMLElement(already_AddRefed<nsINodeInfo> aNodeInfo,
                   mozilla::dom::FromParser aFromParser = mozilla::dom::NOT_FROM_PARSER);

and the other is in nsContentCreatorFunctions.h and has this signature:

 nsresult
 NS_NewHTMLElement(nsIContent** aResult, already_AddRefed<nsINodeInfo> aNodeInfo,
                   mozilla::dom::FromParser aFromParser);

The NS_NewHTMLFooElement functions have the signature as the former, and you're right that those need to match (and need to match the contentCreatorCallback typedef in nsHTMLContentSinkc.cpp).  I'm talking about changing the latter.

> No, I folded the changes all together.

Then why does the v2 patch I reviewed have things like this:

>-  virtual nsIContent* GetPseudoElementContent(nsCSSPseudoElements::Type aType);
>+  virtual mozilla::dom::Element* GetPseudoElement(nsCSSPseudoElements::Type aType);

?
(In reply to Boris Zbarsky [:bz] from comment #10)
> The NS_NewHTMLFooElement functions have the signature as the former, and
> you're right that those need to match (and need to match the
> contentCreatorCallback typedef in nsHTMLContentSinkc.cpp).  I'm talking
> about changing the latter.

Oh right, yes.

> > No, I folded the changes all together.
> 
> Then why does the v2 patch I reviewed have things like this:
> 
> >-  virtual nsIContent* GetPseudoElementContent(nsCSSPseudoElements::Type aType);
> >+  virtual mozilla::dom::Element* GetPseudoElement(nsCSSPseudoElements::Type aType);

The removed line is from the patch in bug 944246 which already landed.
Oh, for some reason I thought this _was_ bug 944246.  Never mind that part, then!  Too many patches in flight all at once.  ;)

Do you want to take bug 945572, or should I?
(In reply to Boris Zbarsky [:bz] from comment #12)
> Oh, for some reason I thought this _was_ bug 944246.  Never mind that part,
> then!  Too many patches in flight all at once.  ;)

:)

> Do you want to take bug 945572, or should I?

I wasn't going to look at it right now, so feel free to if you wish.
https://hg.mozilla.org/mozilla-central/rev/ddd2dbbd9b2d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.