508039 - Port |Bug 456439 - add about:rights and a "Know Your Rights" infobar to Firefox| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: General, enhancement)

Description

[Serge Gautherie (:sgautherie)]

(I noticed this while looking at bug 432644.)

SeaMonkey should want to port bug 459439, then bug 462254.

Ftr, TB had
Bug 463367 - Remove EULA from Thunderbird installer / .dmg and related bits

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> SeaMonkey should want to port bug 459439, then bug 462254.

bug 456439 !

Comment 2

[Robert Kaiser]

Yes, this would be good to have to point users to our license without being intrusive.

Comment 3

[Frank Wein [:mcsmurf]]

Can this bug actually still be ported for the final version (as it would change/add some new strings)?

Comment 4

[Robert Kaiser]

We're not string frozen yet, so why not?

Comment 5

[Frank Wein [:mcsmurf]]

Attachment: Patch

KaiRo: Can you check if the strings are ok as used in this patch? I used the strings from Firefox as a base and removed the ones which talked about phishing and malware detection.

Comment 6

[[e-mail]]

Comment on attachment 400285 [details] [diff] [review]
Patch

>+EXTRA_PP_COMPONENTS = \
Except there's no preprocessing in it...

>+	aboutRights.js \
nsAboutRights.js (c.f. nsAboutAbout.js etc.)

>+ifneq (,$(BUILD_OFFICIAL)$(MOZILLA_OFFICIAL))
>+DEFINES += -DOFFICIAL_BUILD=1
>+endif
Except that we don't actually have unofficial branding yet...

>+# ***** BEGIN LICENSE BLOCK *****
XML comment please.

>+#ifdef OFFICIAL_BUILD
IMHO this file belongs in branding; that way when you build unbranded you'll automatically get unbranded rights (when supported). And no #ifdef of course!

>+pref("browser.EULA.version", 3);
Why?

>+pref("browser.rights.version", 3);
>+pref("browser.rights.3.shown", false);
Why 3?

>+#ifdef DEBUG
>+// Don't show the about:rights notification in debug builds.
Why not do this here for unofficial builds too?

>+    var win = this.getMostRecentBrowserWindow();
Ironically the session restore code already knows which window is focused...

>+    // Set pref to indicate we've shown the notficiation.
Nit: notification

>+<!ENTITY rights.intro-point2a "Mozilla does not grant you any rights to the Mozilla and Firefox trademarks or logos. Additional information on Trademarks may be found ">
...

>+<!ENTITY rights.intro-point3a "Privacy policies for &vendorShortName;'s products may be found ">
???

>+<!ENTITY rights.intro-point4a "&brandShortName; also offers optional web site information services, such as the SafeBrowsing service; however, we cannot guarantee they are 100&#37; accurate or error-free. More details, including information on how to disable the services, can be found in the ">
???

Comment 7

[Robert Kaiser]

Comment on attachment 400285 [details] [diff] [review]
Patch

>+<!ENTITY rights.intro-point2a "Mozilla does not grant you any rights to the Mozilla and Firefox trademarks or logos. Additional information on Trademarks may be found ">
>+<!ENTITY rights.intro-point2b "here">
>+<!ENTITY rights.intro-point2c ".">

But we don't have Firefox trademarks in our product :P
You should talk of SeaMonkey trademarks here ;-)

>+<!-- point 3 text for official branded builds -->
>+<!ENTITY rights.intro-point3a "Privacy policies for &vendorShortName;'s products may be found ">
>+<!ENTITY rights.intro-point3b "here">
>+<!ENTITY rights.intro-point3c ".">

We should care to have a bug for those and block 2.0 on them, it's an oversight we don't have them anywhere yet.

>+<!-- point 4 text for official branded builds -->
>+<!ENTITY rights.intro-point4a "&brandShortName; also offers optional web site information services, such as the SafeBrowsing service; however, we cannot guarantee they are 100&#37; accurate or error-free. More details, including information on how to disable the services, can be found in the ">
>+<!ENTITY rights.intro-point4b "service terms">
>+<!ENTITY rights.intro-point4c ".">

We have SafeBrowsing now? OTOH, I wonder why GLS isn't stated there for FF.

The whole branded/unbranded thing doesn't really apply to SeaMonkey right now, by the way, we don't make a difference in our current builds.


Still, no real problems there, so ui-r=me with the comments fixed.

Comment 8

[[e-mail]]

Pushed changeset 25b3a9e27c15 to comm-central.

Comment 9

[[e-mail]]

Whoops, commented on wrong bug# :-(

Comment 10

[Frank Wein [:mcsmurf]]

Attachment: Patch

Moved all related files to branding/ (the .xhtml file and in locales/ the .dtd and .properties file). I removed the .EULA.version pref, that was some leftover from some old Firefox JS component. Also I lowered the version number to 1. All other comments have been fixed.

Comment 11

[[e-mail]]

Comment on attachment 403120 [details] [diff] [review]
Patch

>+    var notifyRightsText = rightsBundle.formatStringFromName("notifyRightsText",
>+                                                             [productName], 1);
>+    
>+    var buttons = [
Nit: blank line isn't quite blank ;-)

Comment 12

[Frank Wein [:mcsmurf]]

Attachment: Patch

Forgot to add two files...

Comment 13

[[e-mail]]

Comment on attachment 403120 [details] [diff] [review]
Patch

Well, you forgot to include added files...

>+#ifdef DEBUG
>+// Don't show the about:rights notification in debug builds.
>+pref("browser.rights.override", true);
>+#elifndef OFFICIAL_BUILD
>+// Don't show the about:rights notification in non-official builds.
>+pref("browser.rights.override", true);
>+#endif
No offical release value for this?

>+      case "sessionstore-windows-restored":
>+        this._onBrowserStartup();
>+        break;
So, I don't have a working rights patch to test with, but I think it should be fairly simple to get the session store service to pass the focused window or the last window as the subject of the observation, which would avoid the hacks to recalculate it.

>+  _shouldShowRights : function () {
Nit: too many spaces, should be _shouldShowRights: function() {

>+    var nsIStringBundleService = Components.interfaces.nsIStringBundleService;
Nit: could use const

function(aNotificationBar, aButton) {
>+                        browser.selectedTab = browser.addTab("about:rights");
How does the notification get removed?

>+<!ENTITY rights.webservices-term2 "You are welcome to use these Services with the accompanying version of &brandShortName;, and you have all the rights necessary to do so.  &vendorShortName; and its licensors reserve all other rights in the Services.  These terms are not intended to limit any rights granted under open source licenses applicable to &brandShortName; and to corresponding source code versions of &brandShortName;.">
Nit: no point putting two spaces after a full stop.

Comment 14

[[e-mail]]

(In reply to comment #13)
> So, I don't have a working rights patch to test with
Now using your latest patch, it's reasonably straightforward to pass the sessionstore's focused window as the subject of the notification.

Comment 15

[Robert Kaiser]

Neil, Frank, could we get something to happen here soon? We have a string freeze for the whole series upcoming in about 30 hours...

Comment 16

[Frank Wein [:mcsmurf]]

Attachment: Patch

Ignore the changes in suite/profile/migration/src, somehow managed to modify the wrong patch, will remove this of course.

Comment 17

[Frank Wein [:mcsmurf]]

http://hg.mozilla.org/comm-central/rev/6e1ba8d29afa
Checked in aboutRights.dtd and aboutRights.properties because of the l10n freeze.

Comment 18

[[e-mail]]

Comment on attachment 404129 [details] [diff] [review]
Patch

>+  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIAboutModule]),
>+ 
Nit: trailing whitespace

>diff --git a/suite/common/public/nsISuiteGlue.idl b/suite/common/public/nsISuiteGlue.idl
Don't need this change any more. r=me with this fixed.

>+  _onBrowserStartup: function(aSubject)
Perhaps call the parameter aWindow?

>+  _showRightsNotification : function (aSubject) {
Nit: still too many spaces here!

>+    // Stick the notification onto the selected tab of the active browser window.
>+    var browser = aSubject.gBrowser; // for closure in notification bar callback
Since you need it in a variable anyway, I wonder whether it's worth passing in the browser as the parameter rather than the window. Either way, please use .getBrowser() instead of gBrowser.

Comment 19

[Robert Kaiser]

What's still missing here? Just the checkin? Frank, could you do that?

Comment 20

[Frank Wein [:mcsmurf]]

Attachment: Patch for checkin [Checkin: See comment 21+24+25]

Comment 21

[Frank Wein [:mcsmurf]]

http://hg.mozilla.org/comm-central/rev/e6822b5c172a

Will file a follow-up to remove the EULA from the installer.

Comment 22

[rsx11m]

After update from the 20091004 nightly, I get the infobar, but clicking on it gives me popup "The URL is not valid and cannot be loaded." I'm getting the same when typing about:rights manually. Anything missing from the checkin?

[Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20091007 SeaMonkey/2.0pre]

Comment 23

[Robert Kaiser]

The problem is that suite/installer/*/packages files didn't make the name change from aboutRights.js to nsAboutRights.js, so this one is missing.

Comment 24

[Frank Wein [:mcsmurf]]

Doh!
http://hg.mozilla.org/comm-central/rev/176861cdb271
Fixed this

Comment 25

[Frank Wein [:mcsmurf]]

And a followup as I deleted the wrong line http://hg.mozilla.org/comm-central/rev/661df4fe22c7

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> http://hg.mozilla.org/comm-central/rev/176861cdb271

Nit:
{
     2.2 +++ b/suite/installer/windows/packages	Wed Oct 07 14:31:06 2009 +0200
     2.3 @@ -294,11 +294,11 @@ bin\components\GPSDGeolocationProvider.j
    2.10  bin\components\nsAboutFeeds.js
    2.11  bin\components\nsAboutSessionRestore.js
    2.12 +bin\components\nsAboutRights.js
    2.13  bin\components\nsAddonRepository.js
}
Please, move nsAboutRights.js up one line.

Comment 27

[Serge Gautherie (:sgautherie)]

Ftr, bug 513023 landed on m-1.9.2+ (only), will we need to do a follow-up about that too?

Comment 28

[Frank Wein [:mcsmurf]]

Hm, that one uses fixed links like
   <li>&rights.intro-point3a;<a href="http://www.mozilla.com/legal/privacy/">&rights.intro-point3b;</a>&rights.intro-point3c;</li>
though in toolkit/ for aboutRights.xhtml (not sure if we want that?). Not sure if our about:rights handler would override that about:rights handler?

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> Not sure if our about:rights handler would override that about:rights handler?

Possibly related: (TB) bug 513025 comment 1 and (TK) bug 514817.
(Maybe just open a follow-up bug ftb?)

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to comment #26)
> Please, move nsAboutRights.js up one line.

I'm eventually fixing this nit in bug 521524.

Comment 31

[[e-mail]]

Anyone know why about:rights gives XML parse errors in all of my builds?

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to comment #31)
> Anyone know why about:rights gives XML parse errors in all of my builds?

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1.4) Gecko/20091007 SeaMonkey/2.0] (release, rc1) (W2Ksp4)

WorksForMe.

Comment 33

[[e-mail]]

Comment on attachment 404930 [details] [diff] [review]
Patch for checkin
[Checkin: See comment 21+24+25]

>+  <!ENTITY % securityPrefsDTD SYSTEM "chrome://navigator/locale/pref/pref-security.dtd">
>+  %securityPrefsDTD;
OK, I figured it out: this file doesn't exist; when you're using flat chrome then the document breaks but when you're using jar chrome then it does not.

Comment 34

[Robert Kaiser]

(In reply to comment #33)
> OK, I figured it out: this file doesn't exist; when you're using flat chrome
> then the document breaks but when you're using jar chrome then it does not.

Right, it's actually chrome://communicator/locale/pref/pref-security.dtd - where is it actually used? Do we need a patch for 2.0 still? If so, we need to be really fast.

Comment 35

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Remove extraneous (and wrong) pref-security.dtd include [Checkin: Comment 36]

The file exists at
/suite/locales/en-US/chrome/common/pref/pref-security.dtd
chrome://communicator/locale/pref/pref-security.dtd
but is useless here.

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 406022 [details] [diff] [review]
(Cv1) Remove extraneous (and wrong) pref-security.dtd include
[Checkin: Comment 36]


http://hg.mozilla.org/comm-central/rev/45df08842b48

Comment 37

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> http://hg.mozilla.org/comm-central/rev/e6822b5c172a
> 
> Will file a follow-up to remove the EULA from the installer.

I eventually filed bug 525536.

Comment 38

[Robert Kaiser]

(In reply to comment #13)
> >+      case "sessionstore-windows-restored":
> >+        this._onBrowserStartup();
> >+        break;
> So, I don't have a working rights patch to test with, but I think it should be
> fairly simple to get the session store service to pass the focused window or
> the last window as the subject of the observation, which would avoid the hacks
> to recalculate it.

Neil, that had the caveat of yourself reviewing the change in bug 558644 that made this stop being done again and broke the notification. We should either port this improvement to Firefox or do it the Firefox way on our side, or we are prone to doing this again...

Comment 39

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a6pre) Gecko/20100619 Lightning/1.1a1pre SeaMonkey/2.1a2pre - Build ID: 20100619011136

I VERIFY that this build accepts about:rights in the URL bar, responding with a short page titled "About Your Rights" with a list of 4 items for license, trademarks, privacy policy and service terms. I checked that the first three links go to pages with what looks like adequate content (though not yet finalized in the case of the privacy policy); the fourth one opens an additional section on the same about:rights page: "SeaMonkey Website Services" with a heading paragraph and a six-point numbered list.

I am not sure in what circumstances the infobar mentioned in the Summary should appear.

Comment 40

[Robert Kaiser]

(In reply to comment #39)
> I VERIFY that this build accepts about:rights in the URL bar

Yup, that hasn't been broken and should still work fine.

> I am not sure in what circumstances the infobar mentioned in the Summary should
> appear.

It should appear on first launch (possibly any first launch of a new version, I don't remember the heuristics in detail - surely first launch of a new profile, though) and that is what is broken due to bug 558644 changes and for which I filed bug 573384 now.