1407229 - Port bug 1406278 to C-C: Changed parameters of HTMLImageElement::SetSrc/GetSrc()

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

c:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/compose/src/nsMsgCompose.cpp(274): error C2660: 'mozilla::dom::HTMLImageElement::GetSrc': function does not take 1 arguments [log…]
c:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/compose/src/nsMsgCompose.cpp(445): error C2660: 'mozilla::dom::HTMLImageElement::SetSrc': function does not take 2 arguments [log…]

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1407229-setsrc.patch - WIP - does not compile

Boris, what's the magic we need to apply here?

This patch gives two compile errors:
'nsIPrincipal': cannot instantiate abstract class
'void mozilla::dom::HTMLImageElement::SetSrc(const nsAString &,nsIPrincipal &,nsINode::ErrorResult &)': cannot convert argument 2 from 'nullptr' to 'nsIPrincipal &'

Oh, I see, Get/SetHref have changed, too.

Comment 2

[Jorg K (CEST = GMT+2)]

I forgot to say, I looked at
https://hg.mozilla.org/mozilla-central/rev/bf42aab38182
https://hg.mozilla.org/mozilla-central/rev/a679728a98e9
https://hg.mozilla.org/mozilla-central/rev/9fca7bef9dfe
https://hg.mozilla.org/mozilla-central/rev/cf763570260a
https://hg.mozilla.org/mozilla-central/rev/cf383eff981f
https://hg.mozilla.org/mozilla-central/rev/8e7305ab6283
https://hg.mozilla.org/mozilla-central/rev/e894f7b9168b
https://hg.mozilla.org/mozilla-central/rev/ac4afd1e97b7
https://hg.mozilla.org/mozilla-central/rev/19b365ebc32c
https://hg.mozilla.org/mozilla-central/rev/1784e206cc10
https://hg.mozilla.org/mozilla-central/rev/e4308fbed9c5
https://hg.mozilla.org/mozilla-central/rev/ec0c2767cf57
and didn't see a single replacement example. Of course under bustage pressure one misses things :-(

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1407229-setsrc.patch - WIP - compiles but isn't right

OK, by digging around in the (generated) HTMLImageElementBinding.cpp I found something that compiles.

Boris, can you please advise further.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8917011 [details] [diff] [review]
1407229-setsrc.patch - WIP - compiles but isn't right

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

::: mailnews/compose/src/nsMsgSend.cpp
@@ +1890,2 @@
>      } else if (image) {
> +      image->SetSrc(NS_ConvertASCIItoUTF16(domSaveArray[i].url), subjectPrincipal, rv2);

Looks like everywhere else in the patch we do a Get call populating 'subjectPrincipal' before doing a Set call. This is the only place without the Get call. Should I do a Get call just to retrieve the principal?

Comment 5

[Boris Zbarsky [:bzbarsky]]

We should add overloads of the relevant functions without the principal argument on the Gecko side.  The fact that Get* was affected at all is just because bindings are a little silly.

For SetSrc on image we should also add a no-principal overload, that looks like this:

  void SetSrc(const nsAString& aSrc, ErrorResult& aError)
  {
    SetHTMLAttr(nsGkAtoms::src, aSrc, aError);
  }

Similar for SetHref on <link>.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8917011 [details] [diff] [review]
1407229-setsrc.patch - WIP - compiles but isn't right

Passing through uninitialized NonNull is not ok.  It will pass a random value, and probably end up crashing.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1407229-setsrc.patch (v2)

Temporary fix.

As discussed with Boris and Kris (John-Galt?) on IRC, I can use the     NonNull<nsIPrincipal> subjectPrincipal; for the get calls and a system principal for the set calls.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1407229-setsrc.patch (v3)

The NonNull<nsIPrincipal> subjectPrincipal; actually asserts, so that's not an option. So I came up with another hack, since I didn't want to use *(nsIPrincipal*)nullptr.

OK, I'll land this now to unbust TB. How's going to add the overloads, as per IRC:
18:28:14 - bz: Anyway, it's clear how to fix this: add some non-principal-taking overloads

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2ca8d8d56941
Temporary fix for changes in bug 1406278: Changed parameters of HTMLImageElement::SetSrc/GetSrc() and HTMLLinkElement::SetHref(). rs=bustage-fix

Comment 10

[Kris Maglione [:kmag]]

(In reply to Jorg K (GMT+2) from comment #8)
> The NonNull<nsIPrincipal> subjectPrincipal; actually asserts, so that's not
> an option.

That's good to hear. I was going to file a bug if it didn't.

>  So I came up with another hack, since I didn't want to use
> *(nsIPrincipal*)nullptr.

Is there a particular reason not to just pass the system principal, as bz
suggested?

> OK, I'll land this now to unbust TB. How's going to add the overloads, as
> per IRC:
> 18:28:14 - bz: Anyway, it's clear how to fix this: add some
> non-principal-taking overloads

I'll have patches for that sometime this morning.

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #10)
> Is there a particular reason not to just pass the system principal, as bz
> suggested?
Apparently a misunderstanding. I wanted to avoid passing the real value of the system principal as a reference to a function that could possibly overwrite it. Well, we know that the callee wouldn't have looked at it.

This will bug be backed out 100% once bug 1407334 lands.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dd2c759074f4
Backed out changeset 2ca8d8d56941: Backout of temporary fix. a=jorgk

Comment 13

[Jorg K (CEST = GMT+2)]

We're done here since bug 1407334 landed on M-C minutes before the backout in comment #12. So the hacky stuff never got shipped in a Daily, but it helped to keep the tree open and do other stuff in the meantime.

Boris and Kris, thanks again for your help.