Closed Bug 1403516 Opened 2 years ago Closed 2 years ago

Port bug 1389650 to c-c [Remove nsIDOMHTMLAnchorElement]

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set

Tracking

(thunderbird57 fixed, thunderbird58 fixed)

RESOLVED FIXED
Thunderbird 58.0
Tracking Status
thunderbird57 --- fixed
thunderbird58 --- fixed

People

(Reporter: Paenglab, Assigned: jorgk-bmo)

References

Details

Attachments

(3 files, 7 obsolete files)

1.27 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.04 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.13 KB, patch
Details | Diff | Splinter Review
TB is actually busted by bug 1389650.
Jörg, this is beyond my knowledge. Please can you look at it?
Attached patch Bug1403516.patch - RM WIP. (obsolete) — Splinter Review
This is what I tried to do. Jörg, maybe you can use this to start.
Thanks, this looks about right. Does it compile? I've done a two of these already: Bug 1393088 and bug 1381011.
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.
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.
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
(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.
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.
Attached patch Bug1403516.patch (obsolete) — Splinter Review
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.
Assignee: nobody → jorgk
Attachment #8912641 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
Attachment #8912641 - Attachment description: Bug1403516.patch → Bug1403516.patch - RM WIP.
Attachment #8912641 - Attachment is obsolete: false
I decided not to do anything temporary here, so this is the full solution. M-C part.
Attachment #8912641 - Attachment is obsolete: true
Attachment #8912655 - Attachment is obsolete: true
Attachment #8912680 - Attachment is obsolete: true
Attachment #8912699 - Flags: review?(bzbarsky)
Boris, could you be so kind and look at this.
Attachment #8912705 - Flags: review?(bzbarsky)
Comment on attachment 8912699 [details] [diff] [review]
M-C part: 1403516-export.patch

r=me.  Thank you!
Attachment #8912699 - Flags: review?(bzbarsky) → review+
Removed superfluous NS_ENSURE_SUCCESS(rv, rv);
Attachment #8912705 - Attachment is obsolete: true
Attachment #8912705 - Flags: review?(bzbarsky)
Attachment #8912708 - Flags: review?(bzbarsky)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2cbc6c1b82f
Export dom/base/nsDOMTokenList.h and dom/base/nsStyleLinkElement.h. r=bz
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.
Attachment #8912708 - Flags: review?(bzbarsky) → review-
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?
Attachment #8912708 - Attachment is obsolete: true
Attachment #8912771 - Flags: review?(bzbarsky)
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.
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?
> 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.
Attachment #8912771 - Flags: review?(bzbarsky) → review-
Attached patch 1403516-link-anchor.patch (v3) (obsolete) — Splinter Review
Thanks for the support, Boris. This addresses the DOMString issues.

I have filed bug 1403658 as a follow-up.
Attachment #8912771 - Attachment is obsolete: true
Attachment #8912821 - Flags: review?(bzbarsky)
Went back to just nsString to avoid unnecessary changes. Sorry about the noise.
Attachment #8912821 - Attachment is obsolete: true
Attachment #8912821 - Flags: review?(bzbarsky)
Attachment #8912826 - Flags: review?(bzbarsky)
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.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/f2cbc6c1b82f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fe2507d2cf40
Port bug 1389650 to mailnews: Replace use of nsIDOMHTMLAnchorElement and nsIDOMHTMLLinkElement. r=bz
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!
Attachment #8912826 - Flags: review?(bzbarsky) → review+
(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.
Port the DOMString fix back to TB 57 where it belongs.
Attachment #8914043 - Flags: approval-comm-beta+
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
Flags: needinfo?(bzbarsky)
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?
> 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.
Flags: needinfo?(bzbarsky)
Aceman, can you take care of this, this is SM only, right? FRG, see comment #31.
Flags: needinfo?(frgrahl)
Flags: needinfo?(acelists)
That sample was from editor/ui/composer/content/editor.js . SM has its own share of these older objects.
Flags: needinfo?(acelists)
Duplicate of this bug: 1403067
Jorg thanks for the heads up.
Flags: needinfo?(frgrahl)
You need to log in before you can comment on or make changes to this bug.