1332892 - Remove obsolete hackaround from nsDefaultURIFixup.cpp

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Masatoshi Kimura [:emk]]

https://dxr.mozilla.org/mozilla-central/rev/3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/docshell/base/nsDefaultURIFixup.cpp#663-693
We do not need this hack anymore. nsICmdLineService was removed long before. Bug 58866 and 86948 did not regress without this hack.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1332892 - Remove obsolete hackaround from nsDefaultURIFixup.cpp.

Review commit: https://reviewboard.mozilla.org/r/106344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106344/

Comment 2

[Masatoshi Kimura [:emk]]

I also removed vestigial nsICmdLineService declarations while I am here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Bug 58866 and 86948 did not regress without this hack.

Tested on which OSes and which OS localizations?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8829192 [details]
Bug 1332892 - Remove obsolete hackaround from nsDefaultURIFixup.cpp.

https://reviewboard.mozilla.org/r/106344/#review107626

r=me if this is indeed obsolete nowadays, but I'd really like to know what replaced the nsICmdLineService caller and how it behaves before we land this.

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> > Bug 58866 and 86948 did not regress without this hack.
> 
> Tested on which OSes and which OS localizations?

Windows ja-JP. I can verify the fix with en-US, but I have no test environment on other platforms. Nakano-san, Could you verify? Try builds are here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be728d4fc2be

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #4)
> r=me if this is indeed obsolete nowadays, but I'd really like to know what
> replaced the nsICmdLineService caller and how it behaves before we land this.

nsICmdLine* was replaced with nsICommandLine*. See bug 87127 (that was written in the removed comment).

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Unfortunately, I'm on business trip now, I'll be back on this Thursday. If nobody hasn't tested it yet, I'll check it.

What should I do for that? Could you let me know the steps to test it?

Comment 7

[Masatoshi Kimura [:emk]]

Are STRs in bug 58866 and 86948 insufficient?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks, I'll check it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Windows ja-JP

Ah.  I'm pretty sure at least bug 58866 only really bites some codepages, as of when it was created...

> nsICmdLine* was replaced with nsICommandLine*. See bug 87127

OK.  So chances are nsICommandLine* is in fact doing the right things with Unicode?

Comment 10

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> OK.  So chances are nsICommandLine* is in fact doing the right things with
> Unicode?

I think so.

nsICommandLine.idl uses AString:
https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/toolkit/components/commandlines/nsICommandLine.idl

We make up UTF-8 argc/argv on Windows:
https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/toolkit/xre/nsWindowsWMain.cpp

And we use operating system argc/argv as-is on other platforms (i.e. it is encoded in the native charset.)

And the nsICommandLine implementation converts argc/argv to UTF-16 correctly:
https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/toolkit/components/commandlines/nsCommandLine.cpp#393-397

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I tested following steps:

1. Unzip the test build on Desktop
2. Launch the test build
3. Make it default browser
4. Create a folder "™" on Desktop
5. Move a JPEG file to the folder
6. Open the JPEG file with Explorer using "Open with..." and choose the test build (and check always use the application to open the file)
7. Double click the file again
8. Copy the file URL to new tab and type Enter

Then, it works file on:

Win7 ja-JP
Win7 en-US
Win7 zh-CN
Win7 zh-TW
Win7-N ko-KR
Win8.1 ja-JP
Win8.1 en-US
Win8.1 zh-CN
Win8.1 zh-HK
Win8.1 zh-TW
Win8.1-N ko-KR

All of them are x64, but I tested with x86's test build.

Let me know what you'd like me to do something more.

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano [:masayuki] from comment #11)
> Let me know what you'd like me to do something more.

It would be great if you could test with Russian locale that the bug 58866 reporter was using.
But I'll land this even if you can't. I don't believe the bug still exists based on the analysis in comment #10.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I also think the reasoning in comment 10 is pretty convincing about this not being an issue anymore, for what it's worth.  So if testing ru-RU is easy, great, but if not, then don't worry about it.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I installed Win7 x64 ru-RU to VM, and the test build works fine on it.

Comment 15

[Masatoshi Kimura [:emk]]

Thank you very much!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8282c10efdb1
Remove obsolete hackaround from nsDefaultURIFixup.cpp. r=bz

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/8282c10efdb1