Closed
Bug 1475867
Opened 6 years ago
Closed 6 years ago
Port |Bug 1238712 - Remove duplication between nsWindowsShellService, nsGnomeShellService, and nsMacShellService| to SeaMonkey
Categories
(SeaMonkey :: OS Integration, enhancement)
SeaMonkey
OS Integration
Tracking
(seamonkey2.49esr wontfix, seamonkey2.60 fixed, seamonkey2.53 affected, seamonkey2.57esr fixed)
RESOLVED
FIXED
seamonkey2.60
People
(Reporter: iannbugzilla, Assigned: iannbugzilla)
References
(Blocks 1 open bug)
Details
User Story
Also ports parts of: Bug 1063529 - Default Browser Notification Bar and the Preferences callers into the shellservice should try...catch their calls so things don't break when the shell service throws an exception Bug 1256988 - Replace tests on MOZ_WIDGET_GTK with tests on MOZ_WIDGET_TOOLKIT containing gtk Bug 1286627 - Check for undefined shell service before dereferencing it in the ShellService proxy. Linux builds that disable gio do not have the shell service defined and the proxy was throwing Bug 1297239 - Enable eslint for browser/components/shell Bug 1386600 - Change nsIStringBundle methods to return |AString| instead of |wstring| - Using nsAutoString instead of nsString Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods Bug 1432992, part 1 - Remove definitions of Ci, Cr, Cc, and Cu Bug 1440284 - change this.EXPORTED_SYMBOLS back to var EXPORTED_SYMBOLS in JS modules
Attachments
(1 file, 6 obsolete files)
40.53 KB,
patch
|
iannbugzilla
:
review+
iannbugzilla
:
feedback+
iannbugzilla
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
Much of the different shell service implementations is duplicated and has to be updated on each change. Also, it is not all necessary that this code be written in C++. We should use JS where possible and reasonable.
Reasonably straight forward port - needs some testing on mac / windows too.
Attachment #8995820 -
Flags: review?(frgrahl)
Attachment #8995820 -
Flags: feedback?(stefanh)
Attachment #8995820 -
Flags: approval-comm-esr60?
Comment on attachment 8995820 [details] [diff] [review] Port Bug 1238712 Not ready yet
Attachment #8995820 -
Attachment is obsolete: true
Attachment #8995820 -
Flags: review?(frgrahl)
Attachment #8995820 -
Flags: feedback?(stefanh)
Attachment #8995820 -
Flags: approval-comm-esr60?
Comment 3•6 years ago
|
||
FYI: I'll be away from tomorrow until around August 12.
Removed some unnecessary changes. Feedback needed on Windows / Mac platforms
Attachment #9004237 -
Attachment is obsolete: true
Attachment #9004253 -
Flags: review?(frgrahl)
Attachment #9004253 -
Flags: feedback?(stefanh)
Added fix for: Bug 1256988 - Replace tests on MOZ_WIDGET_GTK with tests on MOZ_WIDGET_TOOLKIT containing gtk
Attachment #9004253 -
Attachment is obsolete: true
Attachment #9004253 -
Flags: review?(frgrahl)
Attachment #9004253 -
Flags: feedback?(stefanh)
Attachment #9004331 -
Flags: review?(frgrahl)
Attachment #9004331 -
Flags: feedback?(stefanh)
Comment 7•6 years ago
|
||
patching file suite/components/shell/nsIShellService.idl Hunk #1 FAILED at 3 1 out of 2 hunks FAILED -- saving rejects to file suite/components/shell/nsIShellService.idl.rej patching file suite/components/shell/nsMacShellService.cpp Stycke #1 lyckades vid 31 med luddigheten 2 (offset 2 rader). Hunk #3 FAILED at 132 1 out of 3 hunks FAILED -- saving rejects to file suite/components/shell/nsMacShellService.cpp.rej avbryter: patch failed to apply
Comment 8•6 years ago
|
||
Comment on attachment 9004331 [details] [diff] [review] Added fix As stated on irc: > const nsIShellService = Ci.nsIShellService; These should all go and Ci.nsIShellService used directly. There are other consts in the files and we should probably file a general cleanup bug. Care needs to be taken with toplevel consts though. > suite/components/shell/moz.build > +elif 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']: Please replace with +elif CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gtk3': Only gtk3 left on Linux so no longer needs a generic check. This makes it consistent with the other comm-central checks. > -elif CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gtk3': > +elif 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']: Please revert this change. r+ with the above issues taken care of. We should probably wait with checkin till Stefan was able to look at it and until Bug 1475865 is done. Might also be a candidate for the 2.53+ releases. > + if (ShellService) try { Prefer curly braces around if block. Also now enforced in mozilla-central code via eslint but personal choice so fine as is.
Attachment #9004331 -
Flags: review?(frgrahl) → review+
Comment 9•6 years ago
|
||
I'll try to look at this during the weekend.
Comment 10•6 years ago
|
||
There is actually one problem on mac here which I got stuck with (just did a initial test)... it's not caused by the patch, though, but I think it needs to be fixed (here or in another bug). When setting the default browser the OS will show a dialog asking whether you want to change your default settings. If you set default browser at startup, the OS dialog is behind the main window so the SeaMonkey prompt will not close... Basically, the OS blocks setting default browser until the OS dialog is confirmed and you'll get this exception: ----------- JavaScript error: chrome://communicator/content/defaultClientDialog.js, line 53: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIShellService.setDefaultClient] ----------- The same thing happens when setting the default browser in preferences, but here you'll notice the OS dialog (well, depending on where your pref window is on the screen of course). I haven't had the time to investigate further, but it looks like this would be bug 1063529. I guess he OS dialog will still be hidden behind the window, though.
Comment 11•6 years ago
|
||
Comment on attachment 9004331 [details] [diff] [review] Added fix This actually partly regress SetDesktopBackground (see my comment in Bug 1475865).
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Stefan [:stefanh] from comment #11) > Comment on attachment 9004331 [details] [diff] [review] > Added fix > > This actually partly regress SetDesktopBackground (see my comment in Bug > 1475865). Having spent some time looking at this, I cannot see how this would regress SetDesktopBackground as this bug doesn't make any changes to it. The only changes to do with setting the desktop background is where the detection on ability happens (in JS rather than CPP).
Flags: needinfo?(stefanh)
Attachment #9004331 -
Flags: feedback?(stefanh)
Assignee | ||
Comment 13•6 years ago
|
||
Changes since last patch: * Removal of const nsIShellService = Ci.nsIShellService * Removal of duplication with shouldBeDefaultClientFor * Addition of try/catch to setDefaultClient (to cover fix from bug 1063529)
Attachment #9004331 -
Attachment is obsolete: true
Attachment #9005878 -
Flags: review?(frgrahl)
Attachment #9005878 -
Flags: feedback?(stefanh)
Assignee | ||
Comment 14•6 years ago
|
||
Fully qrefreshed patch with missing change to nsIShellService.idl
Attachment #9005878 -
Attachment is obsolete: true
Attachment #9005878 -
Flags: review?(frgrahl)
Attachment #9005878 -
Flags: feedback?(stefanh)
Attachment #9005887 -
Flags: review?(frgrahl)
Attachment #9005887 -
Flags: feedback?(stefanh)
Comment 15•6 years ago
|
||
Comment on attachment 9005887 [details] [diff] [review] Fixed extra duplication removal (In reply to Ian Neal from comment #12) > Having spent some time looking at this, I cannot see how this would regress > SetDesktopBackground as this bug doesn't make any changes to it. The only > changes to do with setting the desktop background is where the detection on > ability happens (in JS rather than CPP). You're right, my mistake - sorry :/. I think there are som errors in the files since I now don't get the prompt and the "Set SeaMonkey as default browser" section is missing in preferences: JavaScript error: resource:///modules/ShellService.jsm, line 61: SyntaxError: getter functions must have no arguments JavaScript error: chrome://communicator/content/pref/pref-navigator.js, line 283: ReferenceError: ShellService is not defined
Flags: needinfo?(stefanh)
Comment 16•6 years ago
|
||
Yes this is wrong: > get shouldBeDefaultClientFor(appTypes) { > suite/components/pref/content/pref-navigator.js > ShellService.shouldBeDefaultClientFor |= Ci.nsIShellService.BROWSER; Hmm did this ever work work with browser, mail and news?
Comment 17•6 years ago
|
||
Comment on attachment 9005887 [details] [diff] [review] Fixed extra duplication removal (In reply to Frank-Rainer Grahl (:frg) from comment #16) > Yes this is wrong: > > get shouldBeDefaultClientFor(appTypes) { Ah, yes - of course - works fine without the argument :-)
Attachment #9005887 -
Flags: feedback?(stefanh) → feedback+
Comment 18•6 years ago
|
||
Comment on attachment 9005887 [details] [diff] [review] Fixed extra duplication removal Works as before in Windows after fixing the get. Fx 52 does display the Windows setting dialog after switching to the default browser. Thanks to Microsoft wanting to promote Edge you need to set the new browser there too as default. Maybe look at this in a followup bug. Same for the comparision with browser. Looks fishy. r+ with get fixed. Take it to 2.49 too?
Attachment #9005887 -
Flags: review?(frgrahl) → review+
Assignee | ||
Comment 19•6 years ago
|
||
Carrying forward r/f and setting approvals. If we wanted this for 2.49.x then we would have to create a version of the patch for that. Is it worth the effort?
Attachment #9005887 -
Attachment is obsolete: true
Flags: needinfo?(frgrahl)
Attachment #9005892 -
Flags: review+
Attachment #9005892 -
Flags: feedback+
Attachment #9005892 -
Flags: approval-comm-esr60+
Keywords: checkin-needed
Comment 20•6 years ago
|
||
> If we wanted this for 2.49.x then we would have to create a version of the patch for that. Is it worth the effort?
Just checked. 2.49 still has the old "common" file structure so probably not. Maybe later if we can fix the macOS desktop background setting.
Flags: needinfo?(frgrahl)
Comment 21•6 years ago
|
||
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/886ca0e44c5a Port |Bug 1238712 - Remove duplication between nsWindowsShellService, nsGnomeShellService, and nsMacShellService| to SeaMonkey r=frg
Comment 22•6 years ago
|
||
https://hg.mozilla.org/releases/comm-esr60/rev/6b1fb4a5818eab8da54c26d4d8d24c708d4162f8
status-seamonkey2.49esr:
--- → wontfix
status-seamonkey2.53:
--- → affected
status-seamonkey2.57esr:
--- → fixed
status-seamonkey2.60:
--- → fixed
Target Milestone: --- → seamonkey2.60
Comment 23•6 years ago
|
||
Note that nsIDOMElement was removed in 61 (Bug 1455674).
Comment 24•6 years ago
|
||
Drat you are right. Needs some more rebasing. IanN do you have time?
Status: RESOLVED → REOPENED
Flags: needinfo?(iann_bugzilla)
Resolution: FIXED → ---
Assignee | ||
Comment 25•6 years ago
|
||
As far as I can tell, what you landed in https://hg.mozilla.org/comm-central/rev/886ca0e44c5a already has the relevant rebasing.
Flags: needinfo?(iann_bugzilla) → needinfo?(frgrahl)
Comment 26•6 years ago
|
||
You are right. I thought I saw a new nsIDOMElement but in the patch I rebased and pushed they are all gone. Sorry for the noise. Thanks stefanh. This might probably happen more often now when we test only on esr60 so it is good to be alerted.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Flags: needinfo?(frgrahl)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•