Closed Bug 1332892 Opened 3 years ago Closed 3 years ago

Remove obsolete hackaround from nsDefaultURIFixup.cpp

Categories

(Core :: DOM: Navigation, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: emk, Unassigned)

Details

Attachments

(1 file)

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.
I also removed vestigial nsICmdLineService declarations while I am here.
> Bug 58866 and 86948 did not regress without this hack.

Tested on which OSes and which OS localizations?
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.
Attachment #8829192 - Flags: review?(bzbarsky) → review+
(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).
Flags: needinfo?(masayuki)
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?
Flags: needinfo?(masayuki) → needinfo?(VYV03354)
Are STRs in bug 58866 and 86948 insufficient?
Flags: needinfo?(VYV03354) → needinfo?(masayuki)
Thanks, I'll check it.
> 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?
(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
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.
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
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.
I installed Win7 x64 ru-RU to VM, and the test build works fine on it.
Flags: needinfo?(masayuki)
Thank you very much!
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/8282c10efdb1
Remove obsolete hackaround from nsDefaultURIFixup.cpp. r=bz
https://hg.mozilla.org/mozilla-central/rev/8282c10efdb1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.