Closed Bug 1332892 Opened 3 years ago Closed 3 years ago
Remove obsolete hackaround from ns
Default URIFixup .cpp
59 bytes, text/x-review-board-request
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).
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.
(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.
(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.
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.
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
You need to log in before you can comment on or make changes to this bug.