1403516 - Port bug 1389650 to c-c [Remove nsIDOMHTMLAnchorElement]

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Richard Marti (:Paenglab)]

TB is actually busted by bug 1389650.

Comment 1

[Richard Marti (:Paenglab)]

Jörg, this is beyond my knowledge. Please can you look at it?

Comment 2

[Richard Marti (:Paenglab)]

Attachment: Bug1403516.patch - RM WIP.

This is what I tried to do. Jörg, maybe you can use this to start.

Comment 3

[Jorg K (CEST = GMT+2)]

Thanks, this looks about right. Does it compile? I've done a two of these already: Bug 1393088 and bug 1381011.

Comment 4

[Richard Marti (:Paenglab)]

No it fails with a missing header file. Can't say which one because I tried a build with the m-c bug backed out and haven't saved the error.

Comment 5

[Jorg K (CEST = GMT+2)]

I'm getting
10:42.60 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\mozilla/dom/HTMLAnchorElement.h(13): fatal error C1083: Cannot open include file: 'nsDOMTokenList.h': No such file or directory
and that shouldn't be. mozilla/dom/HTMLAnchorElement.h should include without errors, see bug 1389650 comment #29.

All I can do for now is take anchors out.

Comment 6

[Jorg K (CEST = GMT+2)]

Next is:
0:58.30 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\mozilla/dom/HTMLLinkElement.h(14): fatal error C1083: Cannot open include file: 'nsStyleLinkElement.h': No such file or directory

Comment 7

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #5)
> I'm getting
> 10:42.60
> c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\mozilla/
> dom/HTMLAnchorElement.h(13): fatal error C1083: Cannot open include file:
> 'nsDOMTokenList.h': No such file or directory
> and that shouldn't be. mozilla/dom/HTMLAnchorElement.h should include
> without errors, see bug 1389650 comment #29.
> 
> All I can do for now is take anchors out.

Yes, this is what I've got too. I stopped here because I didn't knew why this came.

Comment 8

[Jorg K (CEST = GMT+2)]

If mozilla/dom/HTMLLinkElement.h and mozilla/dom/HTMLAnchorElement.h don't include cleanly, there it nothing I can do other than to ask M-C to fix it.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: Bug1403516.patch

Richard's patch was 100% on the right track. Only that the includes don't work. Here I'm taking out what doesn't compile.

I'm landing this in my name since I don't want to attribute this mess to Richard.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Jorg, I think you should just patch m-c to export nsStyleLinkElement.h and nsDOMTokenList.h.  There's no good reason not to export them.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Temporay bustage fix removing what doesn't compile

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: M-C part: 1403516-export.patch

I decided not to do anything temporary here, so this is the full solution. M-C part.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: C-C part: 1403516-link-anchor.patch (v1)

Boris, could you be so kind and look at this.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8912699 [details] [diff] [review]
M-C part: 1403516-export.patch

r=me.  Thank you!

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: C-C part: 1403516-link-anchor.patch (v1b)

Removed superfluous NS_ENSURE_SUCCESS(rv, rv);

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2cbc6c1b82f
Export dom/base/nsDOMTokenList.h and dom/base/nsStyleLinkElement.h. r=bz

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8912708 [details] [diff] [review]
C-C part: 1403516-link-anchor.patch (v1b)

>+++ b/mailnews/compose/src/nsMsgCompose.cpp
>+  mozilla::dom::DOMString objURL;

Why not just nsAutoString like it used to be?  That would be much better.

>+  nsCOMPtr<Element> linkElement = do_QueryInterface(object);

Why?  You already have imageElement.  Just rename that to "element" and use it for all these FromContent bits.

>+    mozilla::dom::HTMLLinkElement::FromContent(linkElement);
>+    mozilla::dom::HTMLAnchorElement::FromContent(anchorElement);

Why FromContent here but FromContentOrNull in the HTMLImageElement case?

Also, you definitely don't need the "mozilla::" bit given the "using namespace mozilla" in this file.  Dunno about the "dom::" bit; Element shouldn't work without that here, but seems to...

>+  if (!objURL.AsAString().IsEmpty())

This will fatally assert at runtime if this code is ever run, except for the fact that you're passing objURL to things that want an nsAString.  Again, please just don't use DOMString here.

>+++ b/mailnews/compose/src/nsMsgSend.cpp
>+  nsCOMPtr<Element> linkElement = do_QueryInterface(node);
>+  nsCOMPtr<Element> anchorElement = do_QueryInterface(node);

Again, just rename imageElement to "element" instead.

>+    mozilla::dom::HTMLLinkElement::FromContent(linkElement);
>+    mozilla::dom::HTMLAnchorElement::FromContent(anchorElement);

Again, why is the image case FromContentOrNull?  Which one is wrong?

Again, you can drop the "mozilla::" bit.

>+    mozilla::dom::DOMString tUrl;

Don't.  Leave it as nsString.

>+    if (tUrl.AsAString().IsEmpty())

Again, this is bogus.

>+    mozilla::dom::DOMString tUrl;

Don't.

>+    mozilla::dom::DOMString tName;

Add an nsAString overload of GetName on HTMLAnchorElement instead, please.  And then don't use DOMString here.

>+      nsCOMPtr<Element> linkElement = do_QueryInterface(domSaveArray[j].node);
>+      nsCOMPtr<Element> anchorElement = do_QueryInterface(domSaveArray[j].node);

This is silly.  Just have one "element".  Which you already have, as imageElement.

And again, the extra namespacing is pointless.

>         mozilla::dom::DOMString background;

Preexisting, but don't.  Just don't.  This code will do horrible things in opt builds (and assert in debug builds) if there is ever a background on the body and it gets hit!  I hope we're not shipping this to actual users so far, because this is a really bad idea.

Again, add an overload that takes nsAString& to HTMLBodyElement.

>+    nsCOMPtr<Element> linkElement = do_QueryInterface(domSaveArray[i].node);
>+    nsCOMPtr<Element> anchorElement = do_QueryInterface(domSaveArray[i].node);

You know the drill.

>+      mozilla::dom::HTMLLinkElement::FromContent(linkElement);
>+      mozilla::dom::HTMLAnchorElement::FromContent(anchorElement);

And here.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: C-C part: 1403516-link-anchor.patch (v2)

Thanks to the review, Boris. This addresses most of the issues. You asked why the DOMString. Well, the compiler told me so:

0:19.22 c:/mozilla-source/comm-central/mailnews/compose/src/nsMsgSend.cpp(1313): error C2664: 'void mozilla::dom::HTMLBodyElement::GetBackground(mozilla::dom::DOMString &)': cannot convert argument 1 from 'nsAutoString' to 'mozilla::dom::DOMString &'

Same goes for
0:15.85 c:/mozilla-source/comm-central/mailnews/compose/src/nsMsgSend.cpp(1406): error C2664: 'void mozilla::dom::HTMLAnchorElement::GetName(mozilla::dom::DOMString &)': cannot convert argument 1 from 'nsAutoString' to 'mozilla::dom::DOMString &'

What am I missing?

Comment 19

[Jorg K (CEST = GMT+2)]

And since you asked: Sending an e-mail with a background image works fine, the body stuff was done in bug 1393088, that's been shipping in Daily and will go to beta one day. I'm happy to fix it if you let me know how.

Comment 20

[Jorg K (CEST = GMT+2)]

Oops. I missed two lines from your previous comment #16:
> Add an nsAString overload of GetName on HTMLAnchorElement instead, please.  
> And then don't use DOMString here.
> Again, add an overload that takes nsAString& to HTMLBodyElement.

So another M-C patch to get this going? I should do this in a follow-up, but right now I'm worried about getting TB going again. In the follow-up I could also do the missing HTMLBodyElement::FromContentOrNull() and remove the hack we have. How does that sound?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Well, the compiler told me so:

Sure, for background and name.  Not for the other places you used it.

> Sending an e-mail with a background image works fine

Does it actually hit this code?  I cannot see how whatever hits this code can possibly "work fine" based on how DOMString works.

> So another M-C patch to get this going?

Yes.

> but right now I'm worried about getting TB going again.

I'm worried about you introducing security bugs the way the patches are written right now.  I don't know enough about the TB code to tell you whether you are; I can just tell you that you're going to end up poking at fairly random DOMString memory in various ways.  If needed, I can probably figure out exactly what would go wrong, but it's faster to just fix this.

So I'm against this landing with the DOMString stuff as is.  If you absolutely must land this before more m-c changes, the right thing to do is something like this:

  DOMString foo;
  elem->SomeGetter(foo);
  nsString bar; // or nsAutoString
  foo.ToString(bar);
  // Never touch "foo" again, just work with "bar".

> In the follow-up I could also do the missing HTMLBodyElement::FromContentOrNull()

Sounds fine.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1403516-link-anchor.patch (v3)

Thanks for the support, Boris. This addresses the DOMString issues.

I have filed bug 1403658 as a follow-up.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: C-C part: 1403516-link-anchor.patch (v3b)

Went back to just nsString to avoid unnecessary changes. Sorry about the noise.

Comment 24

[Jorg K (CEST = GMT+2)]

Boris, I'll have to land this without your final review, I hope you understand. If there are any remaining issues, I'll do a follow-up. I'm undecided whether to land it as a bustage-fix or with r=bz despite not having your final seal of approval. Since I addressed your concerns, I'll go with the latter. 

As you know, TB is in a quite uncomfortable situation: We don't have continuous integration like Mozilla's autoland or inbound. We are faced with up to 300 changesets landing on M-C up to three times daily. That makes is very hard to find regressions. TB has been busted for 24 hours now, starting with bug 1403393 about 24 hours ago. I don't even know whether the fix to that bug has worked since this bug here got in the way and try pushes have been hampered by slowness and retries today.

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/f2cbc6c1b82f

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fe2507d2cf40
Port bug 1389650 to mailnews: Replace use of nsIDOMHTMLAnchorElement and nsIDOMHTMLLinkElement. r=bz

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8912826 [details] [diff] [review]
C-C part: 1403516-link-anchor.patch (v3b)

r=me (I actually reviewed the changeset that landed, fwiw, since that seemed more relevant.)

Thank you for dealing with all this!

Comment 28

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #27)
> Thank you for dealing with all this!
Boris, thanks for your continued support and your understanding for our situation. We'll meet again soon in bug 1403658.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1403516-domstring-beta.patch

Port the DOMString fix back to TB 57 where it belongs.

Comment 30

[Jorg K (CEST = GMT+2)]

Beta (TB 57):
https://hg.mozilla.org/releases/comm-beta/rev/bf3078cf0a720532f9db32dd99de78d9d6d89c41

Comment 31

[:aceman]

Is seems we have some remaining references in editor.js:

    if (node instanceof Components.interfaces.nsIDOMHTMLImageElement ||
          node instanceof Components.interfaces.nsIDOMHTMLInputElement)
        tooltipText = node.getAttribute("src");
      else if (node instanceof Components.interfaces.nsIDOMHTMLAnchorElement)

Can we just replace this with 'node instanceof HTMLAnchorElement' ? I have seen this is not defined in every JS file, so how can we access it? Or is it not the same as nsIDOMHTMLAnchorElement

Comment 32

[Jorg K (CEST = GMT+2)]

Just node.localName == "a" or "img" or "input", I'd say.

nsIDOMHTMLImageElement.idl is still there, as is nsIDOMHTMLInputElement, however nsIDOMHTMLAnchorElement is gone. Is that code run in TB?

Comment 33

[Boris Zbarsky [:bzbarsky]]

> Can we just replace this with 'node instanceof HTMLAnchorElement' ? 

The safest thing to replace with is:

  ChromeUtils.getClassName(node) == "HTMLAnchorElement"

I suggest proactively doing that with nsIDOMHTMLImageElement/nsIDOMHTMLInputElement too, because those will go away as well.

Comment 34

[Jorg K (CEST = GMT+2)]

Aceman, can you take care of this, this is SM only, right? FRG, see comment #31.

Comment 35

[:aceman]

That sample was from editor/ui/composer/content/editor.js . SM has its own share of these older objects.

Comment 37

[Frank-Rainer Grahl (:frg)]

Jorg thanks for the heads up.