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)

enhancement
Not set
normal

Tracking

(seamonkey2.49esr wontfix, seamonkey2.60 fixed, seamonkey2.53 affected, seamonkey2.57esr fixed)

RESOLVED FIXED
seamonkey2.60
Tracking Status
seamonkey2.49esr --- wontfix
seamonkey2.60 --- fixed
seamonkey2.53 --- affected
seamonkey2.57esr --- fixed

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)

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.
User Story: (updated)
User Story: (updated)
User Story: (updated)
Depends on: 1479292
Attached patch Port Bug 1238712 (obsolete) — Splinter Review
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?
FYI: I'll be away from tomorrow until around August 12.
User Story: (updated)
User Story: (updated)
Attached patch Updated port of bug 1238712 (obsolete) — Splinter Review
Attached patch Reduced port of bug 1238712 (obsolete) — Splinter Review
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)
User Story: (updated)
Attached patch Added fix (obsolete) — Splinter Review
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)
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 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+
I'll try to look at this during the weekend.
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 on attachment 9004331 [details] [diff] [review]
Added fix

This actually partly regress SetDesktopBackground (see my comment in Bug 1475865).
(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)
Attached patch Extra duplication removal (obsolete) — Splinter Review
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)
User Story: (updated)
Attached patch Fixed extra duplication removal (obsolete) — Splinter Review
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 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)
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 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 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+
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
> 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)
Blocks: 1488073
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Note that nsIDOMElement was removed in 61 (Bug 1455674).
Drat you are right. Needs some more rebasing. IanN do you have time?
Status: RESOLVED → REOPENED
Flags: needinfo?(iann_bugzilla)
Resolution: FIXED → ---
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)
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 ago6 years ago
Flags: needinfo?(frgrahl)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: