1552866 - Prefill user name in account creation again

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Comment 1

[[e-mail]]

Attachment: 1552866.diff

Comment 2

[:aceman]

Why?

Comment 3

[Ben Bucksch (:BenB)]

Because we should never ask the user to enter information that we already have. With this, user user only needs to fill out 2 fields instead of 3.

The reasoning for Firefox mentioned in bug 1433350 is not valid here in this case. That was about anonymous users in a browser. We are a mail client, and that doesn't apply. Still, if Tor wants to avoid that, they can remove the interface in their build. This is not a reason to make things deliberately difficult for the normal users.

Comment 4

[Ben Bucksch (:BenB)]

This restores functionality we had in TB 60.

Comment 5

[Ben Bucksch (:BenB)]

Note that the original Tor bug was: "At startup, browser gleans user full name from OS". With this patch, we don't do that. We only query the full name when the user sets up an account, when we need it. And we're a mail client, not a browser.

Comment 6

[[e-mail]]

Comment on attachment 9066063 [details] [diff] [review]
1552866.diff

Sorry, I forgot to describe the patch when I first attached it.

The C++ bits are basically copied from ESR 60, but with the code style updated. The only real difference is the removal of a configure flag that had been used to detect Android.

The JS bits are reverts of the commits that removed nsIUserInfo, but again with the code style updated.

Comment 7

[[e-mail]]

Oh, I guess nsCommonModule.cpp has some real changes in it too.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9066063 [details] [diff] [review]
1552866.diff

I'm generally in favour of this. Looks like you ran the formatter on the C++ code (thanks, or it was already formatted before it got removed from M-C).

The JS part looks like a backout of bug 1433350 (two changesets), but it doesn't pass linting now:
c:\mozilla-source\comm-central\comm\mailnews\base\prefs\content\aw-identity.js
  124:19  error  Operator '==' must be spaced.                                                  space-infix-ops (eslint)
  128:9   error  Closing curly brace does not appear on the same line as the subsequent block.  brace-style (eslint)
  145:9   error  Closing curly brace does not appear on the same line as the subsequent block.  brace-style (eslint)

While you're there, please make the indentation two spaces and lose the parenthesis in the ternary operator (x) ? y : x;

Comment 9

[[e-mail]]

Attachment: With tweak

Checks for a space in the name to ensure it's a full name.

Comment 10

[[e-mail]]

(In reply to Jorg K (GMT+2) from comment #8)

124:19 error Operator '==' must be spaced.

Bah, I missed one...

Comment 11

[[e-mail]]

Attachment: With exceptions

Comment 12

[[e-mail]]

Attachment: Original versions of files for comparison purposes

Comment 13

[Ben Bucksch (:BenB)]

Thanks, Neil!

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9066094 [details] [diff] [review]
With exceptions

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

If we're adding it back, let's make the interface reasonable. I'd really suggest not throwing if one of the properties isn't available... :/

::: common/public/nsIUserInfo.idl
@@ +7,5 @@
> +
> +[scriptable, uuid(6c1034f0-1dd2-11b2-aa14-e6657ed7bb0b)]
> +interface nsIUserInfo : nsISupports
> +{
> +    /* these are things the system may know about the current user */

Please move the document up to top level and use proper documentation style /** */

@@ +13,5 @@
> +    readonly attribute wstring fullname;
> +
> +    readonly attribute string emailAddress;
> +
> +    /* should this be a wstring? */

I think, all of these strings should be AUTF8String.

::: mail/components/accountcreation/content/emailWizard.js
@@ +173,5 @@
> +        userFullname = userInfo.fullname;
> +      }
> +    } catch (e) {
> +      // nsIUserInfo may not be implemented on all platforms, and name might
> +      // not be available even if it is.

should really check if ("@mozilla.org/userinfo;1"]" in Cc) instead of try/catch

Comment 15

[[e-mail]]

(In reply to Magnus Melin from comment #14)

I'd really suggest not throwing if one of the properties isn't available... :/

So what would you do in that case?

Comment 16

[Magnus Melin [:mkmelin]]

Return NS_OK with an empty string.

Comment 17

[Ben Bucksch (:BenB)]

should really check if ("@mozilla.org/userinfo;1"]" in Cc) instead of try/catch

Good idea with the |if|, please keep the try/catch, because the call might fail for other reasons, and we don't want to abort the dialog because of this.

Comment 18

[Ben Bucksch (:BenB)]

Clarification:
Good idea with the |if|, please add that, but please keep the try/catch as well...

Comment 19

[[e-mail]]

Attachment: Addressed review comments

Changed IDL as requested. Works on Linux, spotted typo in Mac code after pushing to try, not tested on Windows.

Comment 20

[Ben Bucksch (:BenB)]

Return NS_OK with an empty string.

My 2 cents: I think returning an error when an actual error happened is the correct API for the call. It's always up to the caller how to deal with it.

Comment 21

[Ben Bucksch (:BenB)]

@Magnus: Ping

@Jörg: We need this merged to TB 68, please.

Comment 22

[Ben Bucksch (:BenB)]

FWIW, Neil did remove the error handling in the C++ code. I think that's a mistake, but it does follow Magnus' review request, so this should be ready to land.

Comment 23

[[e-mail]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=83499ac3c2f57e6b6014c42edf51ddd4deeeb514

Comment 24

[Jorg K (CEST = GMT+2)]

Ben, once you have your review, please request landing and beta uplift. I see that you and Neil are working on various AM bugs, for example bug 1553087, please don't forget to set checkin-needed when they are ready to go. In general, everything will be taken care of if you set the right flags ;-)

Comment 25

[Ben Bucksch (:BenB)]

Re try: The 4 oranges seem to be unrelated intermittent failures.

@Jörg: Thanks :-). I've set the flags on the other bug, indeed I forgot them. Thanks for the reminder.

@Magnus: Neil addressed your review comments. Can we please have r+?

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9066427 [details] [diff] [review]
Addressed review comments

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

Please set a proper hg commit message.

r=mkmelin with the below addressed

::: mail/components/accountcreation/content/emailWizard.js
@@ +173,5 @@
> +        if (userInfo.fullname.includes(" ")) {
> +          userFullname = userInfo.fullname;
> +        }
> +      } catch (e) {
> +        // nsIUserInfo might fail on some platforms.

I don't see that happening. The only error we'd be catching is syntax errors inside the try-catch.

@@ +181,3 @@
>      this._domain = "";
>      this._email = "";
> +    this._realname = userFullname ? userFullname : "";

userFullname || ""
But I think you don't need the tmp. You can just init this._realname = "", and set it to something else inside the userinfo block

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +291,5 @@
> +        let userInfo = Cc["@mozilla.org/userinfo;1"].getService(Ci.nsIUserInfo);
> +        name = userInfo.fullname;
> +      } catch (e) {
> +        // nsIUserInfo may not be implemented on all platforms, and name might
> +        // not be available even if it is.

please remove this, try-catch as name is simply empty now if it's not available

::: mailnews/base/prefs/content/aw-identity.js
@@ +120,5 @@
>  }
>  
>  function checkForFullName() {
> +  var name = document.getElementById("fullName");
> +  if (name.value == "" && "@mozilla.org/userinfo;1" in Cc) {

if (!name.value && ....

or maybe early return?

@@ +125,5 @@
> +    try {
> +      var userInfo = Cc["@mozilla.org/userinfo;1"].getService(Ci.nsIUserInfo);
> +      name.value = userInfo.fullname;
> +    } catch (ex) {
> +      // dump ("checkForFullName failed: " + ex + "\n");

and this try-catch too

Comment 27

[Ben Bucksch (:BenB)]

I don't see that happening. The only error we'd be catching is syntax errors inside the try-catch.

There could also be XPCOM errors. Removing the try/catch would be wrong. This is optional code, and if that breaks or fails - for whatever reason, that doesn't matter - , the dialog should not break. Wrapping helpful, but strictly optional code in a try/catch is the correct coding pattern. Removing the try/catch would be wrong.

The very nature of bugs and errors is that you cannot predict them.

BTW: Removing errors and just returning OK and an empty string in case of errors, as you suggested, is a bad API, but we accepted that. Removing the try/catch, however, is absolutely and clearly bad coding. The try/catch doesn't cost anything, and can help in case of unforseen errors, so removing that is not a reasonable course.

Comment 28

[Ben Bucksch (:BenB)]

Given that I consider the try/catch to be important for safe coding in face of unforeseen problems, and that all the code was there before already, and we merely restore it, could you please approve this as-is?
Please note that this is already a compromise, because we already removed the error handling inside the component due to your request.

But in face of the drastic earth-shakeing changes that keep coming in from Mozilla, and the fact that Thunderbird developers often do not catch the bugs themselves, I'd rather code safely. Can you follow that reasoning, please?

Comment 29

[Magnus Melin [:mkmelin]]

(In reply to Ben Bucksch (:BenB) from comment #27)

There could also be XPCOM errors.

Such as? If creating the userInfo fails, clearly you'd have much bigger problems at hand - and the methods do not throw errors now.

The whole point of the try-catch earlier was to in case the interface isn't implemented for some platform.

Clearly hiding syntax errors in js is bad and does have a real cost.

Comment 30

[[e-mail]]

(In reply to Magnus Melin from comment #26)

Please set a proper hg commit message.

Sorry about that, that was my Try push; my original patch did have a proper message.

Comment 31

[[e-mail]]

Attachment: Without exceptions

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 9067275 [details] [diff] [review]
Without exceptions

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

LGTM, thx! r=mkmelin

Comment 33

[Ben Bucksch (:BenB)]

Comment on attachment 9067275 [details] [diff] [review]
Without exceptions

[Approval Request Comment]
Regression caused by (bug #): 1433350
User impact if declined: User has to fill in 3 fields. With this fixed, he has to fill in only 2 fields.

Comment 34

[Jorg K (CEST = GMT+2)]

Comment on attachment 9067275 [details] [diff] [review]
Without exceptions

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

Some unrelated changes slipped in here.

::: mail/components/accountcreation/content/emailWizard.js
@@ +170,5 @@
>      this._realname = "";
> +    if ("@mozilla.org/userinfo;1" in Cc) {
> +      let userInfo = Cc["@mozilla.org/userinfo;1"].getService(Ci.nsIUserInfo);
> +      // Assume that it's a genuine full name if it includes a space.
> +      if (userInfo.fullname.includes(" ")) {

Why is this a valid assumption? What about CJK languages? Or someone who configured their machine with "full name" 'Froggy' and wants to use this? Was this in the original code?

::: mailnews/mime/emitters/nsMimeBaseEmitter.cpp
@@ +422,5 @@
>      uint64_t avail;
>      rv = mInputStream->Available(&avail);
>      if (NS_SUCCEEDED(rv) && avail) {
> +      nsCOMPtr<nsIRequest> request(do_QueryInterface((nsISupports*)mChannel));
> +      mOutListener->OnDataAvailable(request, mInputStream, 0,

What is this?

@@ +935,4 @@
>      mozilla::DebugOnly<nsresult> rv2 = mInputStream->Available(&bytesInStream);
>      NS_ASSERTION(NS_SUCCEEDED(rv2), "Available failed");
>      if (bytesInStream) {
> +      nsCOMPtr<nsIRequest> request(do_QueryInterface((nsISupports*)mChannel));

And this?

Comment 35

[Ben Bucksch (:BenB)]

someone who configured their machine with "full name" 'Froggy'

That's precisely what we do not want and explicitly want to prevent. It's fine to use a nick name on your personal computer. But it's not for the rest of the world.

This should be the real name of the person. If he is really determined to use that nick name, and use it in front of the world, he should write it himself. We do not want to prefill nick names like "Schnucki".

nsMimeBaseEmitter.cpp

Yes, that in fact is an unrelated change that slipped in and should be removed. Sorry about that.

Comment 36

[Jorg K (CEST = GMT+2)]

Looking for a space won't work for Chinese names: https://en.wikipedia.org/wiki/Chinese_name since first name and surname are not separated by a space.

Comment 37

[Magnus Melin [:mkmelin]]

Yeah seems better to just set the name as configured on the machine and not check the space.

Comment 38

[Ben Bucksch (:BenB)]

OK, that's a good point. But I'm not sure that we should pre-fill pure CJK names. There are cultural considerations, that it's impolite to speak the first name of Chinese, but it's normal in the USA. Also, CJK characters are unspeakable for everybody outside the country and not appropriate for international communication like email. So, even if your local name is 姓名, you probably want to add your transliterated name in addition to the CJK characters. It's not clear what the right solution here is. Just prefilling 姓名 without transliteration isn't a good idea.

And I really don't want to prefill "Schnucki" and "Froggy".

Comment 39

[[e-mail]]

(In reply to Jorg K (GMT+2) from comment #34)

Comment on attachment 9067275 [details] [diff] [review]
::: mailnews/mime/emitters/nsMimeBaseEmitter.cpp
@@ +422,5 @@

 uint64_t avail;
 rv = mInputStream->Available(&avail);
 if (NS_SUCCEEDED(rv) && avail) {

•  nsCOMPtr<nsIRequest> request(do_QueryInterface((nsISupports*)mChannel));
  

•  mOutListener->OnDataAvailable(request, mInputStream, 0,
  

What is this?

@@ +935,4 @@

 mozilla::DebugOnly<nsresult> rv2 = mInputStream->Available(&bytesInStream);
 NS_ASSERTION(NS_SUCCEEDED(rv2), "Available failed");
 if (bytesInStream) {

•  nsCOMPtr<nsIRequest> request(do_QueryInterface((nsISupports*)mChannel));
  

And this?

Sorry, this was some temporary code I'd written because my debug build was crashing because the HTML parser now checks that you pass the same request in each time, and because of the way XPCOM works this isn't true for a JS object that gets queried from nsIRequest to nsIChannel and then downcast back.

Comment 40

[[e-mail]]

Attachment: Without exceptions

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 9068012 [details] [diff] [review]
Without exceptions

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

Looked OK in the comparison with the first version:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9066063&action=interdiff&newid=9068012&headers=1

So what are we going to do with the space check? That wasn't in the original.

::: common/src/nsUserInfoMac.mm
@@ +62,5 @@
> +  GetEmailAddress(aDomain);
> +  int32_t index = aDomain.FindChar('@');
> +  if (index != -1) {
> +      // chop off everything before, and including the '@'
> +    aDomain.Cut(0, index + 1);

Comment misaligned.

::: common/src/nsUserInfoUnix.cpp
@@ +64,5 @@
> +  pw = getpwuid(geteuid());
> +
> +  if (!pw || !pw->pw_name) return NS_OK;
> +
> +  NS_CopyNativeToUnicode(nsDependentCString(pw->pw_name), aUsername);

That could be a `mozilla::MakeStringSpan()`, no? Like:
https://searchfox.org/comm-central/rev/2ddfe0d5bd910e7e7bdbdebb5b826b4863dfd880/mailnews/intl/nsCharsetConverterManager.cpp#49
A few more below.

Comment 42

[[e-mail]]

(In reply to Jorg K from comment #41)

That could be a mozilla::MakeStringSpan(), no?

Ah, sorry, I didn't know about bug 1402247. I'll fix it (and the indent) later today.

Comment 43

[[e-mail]]

Attachment: With MakeStringSpan()

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069247 [details] [diff] [review]
With MakeStringSpan()

Thanks. You replaced `NS_CopyNativeToUnicode` with `CopyUTF8toUTF16` which is OK since on Linux/Mac that's pretty much the same.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1fc57303ad6b
Restore prefilling user name in account creation after bug 1433350. r=mkmelin,jorgk

Comment 46

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/2ed5f9d7b78d92f40ca2a7285168d1db0bf485fc

Comment 47

[Eric Moore]

See Bug 1576393 for background info

Thunderbird 68 (build id 20190816191614) worked as expected when I used my fathers Windows 10 laptop. It has two accounts, and the new account wizard used the account name I was logged into. Its a local account. While his login name is Donald I believe the account information knows his full name. He probably has a Outlook account registered in "email & accounts". The machine originally used Windows 7 and temporarily used a Microsoft account to login until that was changed later on to use a local account.

When I got back home I used my own PC to install version 68. My windows 10 account name is Eric. I use a local account and AFAIK the windows account does not know my last name. I do not have anything registered in "email & accounts". I have a Outlook.com account but it is not registered. Winver says I'm using Windows 10 Home version 1903 (OS Build 18362.295). At the bottom it says:

This product is licensed under the Microsoft Software License Terms to :
user name
org name

The new account wizard displayed First Last as the name. I expected it to display Eric. Is their any requirement that there be a Microsoft account?

Comment 48

[Jorg K (CEST = GMT+2)]

The name is only filled in of it consists of two strings with a space:
https://hg.mozilla.org/comm-central/rev/1fc57303ad6b#l9.14