1519732 - Dragging a photo onto an address book card doesn't work any more in TB 65 and trunk

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Jorg K (CEST = GMT+2)]

STR: Open a contact to Photo tab. Click OK.

Result: Not working.

Working fine in TB 60 ESR.

Aceman, your baby broke here :-(

Alice, could you please find the regression.

Comment 1

[Alice0775 White]

STR: Open a contact to Photo tab. Click OK.

Can you please describe more detailed steps?

Comment 2

[Jorg K (CEST = GMT+2)]

OK. Open the address book. Double-click a contact. Open the Photo tab. Drag a (small) photo from the desktop to the photo area.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=6731dac68a6610d4e81447580c225ba8d63cbc97&tochange=6731dac68a6610d4e81447580c225ba8d63cbc97
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=987ea0d6a000b95cf93928b25a74a7fb1dfe37b2&tochange=cc3401e78e8bbae22e6dbc854e525ceae4923bcf

Comment 4

[Alice0775 White]

(In reply to Alice0775 White from comment #3)

Regression window:
https://hg.mozilla.org/comm-central/
pushloghtml?fromchange=6731dac68a6610d4e81447580c225ba8d63cbc97&tochange=6731
dac68a6610d4e81447580c225ba8d63cbc97
https://hg.mozilla.org/mozilla-central/
pushloghtml?fromchange=987ea0d6a000b95cf93928b25a74a7fb1dfe37b2&tochange=cc34
01e78e8bbae22e6dbc854e525ceae4923bcf

Via local build,
Last good: c-c 6731dac68a66, m-c 3d7f2fdc5bf7
First bad: c-c 6731dac68a66, m-c 9a2b02fe351b

Regressed by: 9a2b02fe351b Gijs Kruitbosch — Bug 1469916, r=ckerschb,jkt

Comment 5

[Jorg K (CEST = GMT+2)]

Thank you, Alice, very helpful!

Looks like https://hg.mozilla.org/mozilla-central/rev/9a2b02fe351b affected our ability to drag photos onto address book cards. I believe you're a TB user, so you might know this function. We implemented it in bug 892889:
https://hg.mozilla.org/comm-central/rev/7aa7b9e29d85

Looks like saveURI() takes a principal now:
https://hg.mozilla.org/mozilla-central/rev/9a2b02fe351b#l18.72

Comment 6

[Jorg K (CEST = GMT+2)]

Oops, I was going to address the previous comment to Gijs, so please ignore the "I believe you're a TB user, so you might know this function.".

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1519732-saveURI.patch - WIP

Hmm, I thought the attached patch would fix it, but it doesn't. It gets me a little further, but now the operation hangs with a progress bar "Saving the image" that never completes.

Hi Gijs,
I believe you're a TB user, so you might know this function.

What else do we need to do to restore the functionality, details in comment #5.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1519732-saveURI.patch - WIP, alternative

Following
https://hg.mozilla.org/mozilla-central/diff/9a2b02fe351b/toolkit/mozapps/extensions/LightweightThemeManager.jsm#l1.13
we should use Services.scriptSecurityManager.createCodebasePrincipal(source, {});
but that makes no difference.

Comment 9

[:Gijs (he/him)]

Comment on attachment 9036265 [details] [diff] [review]
1519732-saveURI.patch - WIP

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

I'd start by actually echo'ing whatever error you get here.

Comment 10

[:Gijs (he/him)]

I'm a TB user but I never bother with pictures for email/contacts, and I can't quickly find this functionality. Please provide detailed steps to reproduce.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1519732-saveURI.patch - WIP (v3)

Thanks for the comments. More detailed STR:
Open the address book. Double-click a contact. Open the Photo tab. Drag a (small) photo from the desktop to the photo area (see comment #2).

I looked a bit closer now and saw this error:
JavaScript error: chrome://messenger/content/addressbook/abCard.js, line 1137: TypeError: Value being assigned to HTMLProgressElement.value is not a finite floating-point value.

Removing the offending line brings the function back. We'll work out what's wrong with the progress meter there.

Gijs, last question: Using a codebase principal as per comment #8 is better than using the system principal, right?

Comment 12

[:Gijs (he/him)]

(In reply to Jorg K (GMT+1) from comment #11)

Gijs, last question: Using a codebase principal as per comment #8 is better than using the system principal, right?

We should avoid using system principal for helper functions, yes. If this is a drag/drop from the filesystem I expect it won't make much difference, you'll get a file: principal for the codebase principal, which is also fairly powerful... Does it not work with the codebase principal?

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1519732-saveURI.patch (v4)

This restores the function. Debugging showed that undefined was passed to onProgress() and it choked. That's most likely a side-effect of switching from XUL progress to HTML progress.

Yes, Gijs, the codebase principal works.

I think we're done here.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9036302 [details] [diff] [review]
1519732-saveURI.patch (v4)

Messing with some flags here, so it shows up on my list of potential uplifts.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1519732-saveURI.patch (v4b)

I doesn't need the !! - I got confused with all those falsy values :-(

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1519732-saveURI.patch (v4c)

Aceman pointed out that the percentage could be zero, so let' use !== undefined.

Comment 17

[:aceman]

Comment on attachment 9036455 [details] [diff] [review]
1519732-saveURI.patch (v4c)

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

Thank you, this works for me.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f9cedb1d7834
fix AB photo drag, nsIWebBrowserPersist.saveURI() takes a principal parameter after bug 1469916. r=aceman

Comment 19

[Jorg K (CEST = GMT+2)]

TB 65 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/8039ee8e0391270d9bfd6c45571ee6d22ebcb2d0