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

RESOLVED FIXED in seamonkey2.0

Status

SeaMonkey
General
--
enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: mcsmurf)

Tracking

(Blocks: 2 bugs, {fixed-seamonkey2.0})

Trunk
seamonkey2.0
fixed-seamonkey2.0
Dependency tree / graph
Bug Flags:
wanted-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [l10n-impact])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
(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
Flags: wanted-seamonkey2?
(Reporter)

Comment 1

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

bug 456439 !

Comment 2

8 years ago
Yes, this would be good to have to point users to our license without being intrusive.
Flags: wanted-seamonkey2? → wanted-seamonkey2+
Depends on: 513023
(Assignee)

Comment 3

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

Comment 4

8 years ago
We're not string frozen yet, so why not?
(Assignee)

Comment 5

8 years ago
Created attachment 400285 [details] [diff] [review]
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.
Attachment #400285 - Flags: ui-review?(kairo)
Attachment #400285 - Flags: review?(neil)

Comment 6

8 years ago
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 ">
???

Updated

8 years ago
Attachment #400285 - Flags: ui-review?(kairo) → ui-review+

Comment 7

8 years ago
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

8 years ago
Pushed changeset 25b3a9e27c15 to comm-central.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 9

8 years ago
Whoops, commented on wrong bug# :-(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

8 years ago
Status: REOPENED → ASSIGNED
(Reporter)

Updated

8 years ago
Assignee: nobody → bugzilla

Updated

8 years ago
Whiteboard: [l10n-impact]
(Assignee)

Comment 10

8 years ago
Created attachment 403120 [details] [diff] [review]
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.
Attachment #400285 - Attachment is obsolete: true
Attachment #403120 - Flags: review?(neil)
Attachment #400285 - Flags: review?(neil)
Comment on attachment 403120 [details] [diff] [review]
Patch

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

Comment 12

8 years ago
Created attachment 403136 [details] [diff] [review]
Patch

Forgot to add two files...
Attachment #403120 - Attachment is obsolete: true
Attachment #403136 - Flags: review?(neil)
Attachment #403120 - Flags: review?(neil)
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.
(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

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

Comment 16

8 years ago
Created attachment 404129 [details] [diff] [review]
Patch

Ignore the changes in suite/profile/migration/src, somehow managed to modify the wrong patch, will remove this of course.
Attachment #403136 - Attachment is obsolete: true
Attachment #404129 - Flags: review?(neil)
Attachment #403136 - Flags: review?(neil)
(Assignee)

Comment 17

8 years ago
http://hg.mozilla.org/comm-central/rev/6e1ba8d29afa
Checked in aboutRights.dtd and aboutRights.properties because of the l10n freeze.
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.
Attachment #404129 - Flags: review?(neil) → review+

Comment 19

8 years ago
What's still missing here? Just the checkin? Frank, could you do that?
(Assignee)

Comment 20

8 years ago
Created attachment 404930 [details] [diff] [review]
Patch for checkin
[Checkin: See comment 21+24+25]
Attachment #404129 - Attachment is obsolete: true
Attachment #404930 - Flags: review+
(Assignee)

Comment 21

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

Will file a follow-up to remove the EULA from the installer.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Keywords: fixed-seamonkey2.0
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0

Comment 22

8 years ago
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

8 years ago
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.
(Assignee)

Comment 24

8 years ago
Doh!
http://hg.mozilla.org/comm-central/rev/176861cdb271
Fixed this
(Assignee)

Comment 25

8 years ago
And a followup as I deleted the wrong line http://hg.mozilla.org/comm-central/rev/661df4fe22c7
(Reporter)

Comment 26

8 years ago
(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.
(Reporter)

Comment 27

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

Comment 28

8 years ago
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?
(Reporter)

Comment 29

8 years ago
(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?)
(Reporter)

Comment 30

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

I'm eventually fixing this nit in bug 521524.
Anyone know why about:rights gives XML parse errors in all of my builds?
(Reporter)

Comment 32

8 years ago
(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 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

8 years ago
(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.
(Reporter)

Comment 35

8 years ago
Created attachment 406022 [details] [diff] [review]
(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.
Attachment #406022 - Flags: review?(neil)
Attachment #406022 - Flags: review?(bugzilla)

Updated

8 years ago
Attachment #406022 - Flags: review?(neil)
Attachment #406022 - Flags: review?(bugzilla)
Attachment #406022 - Flags: review+
(Reporter)

Updated

8 years ago
Attachment #406022 - Flags: review+ → approval-seamonkey2.0?

Updated

8 years ago
Attachment #406022 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
(Reporter)

Comment 36

8 years ago
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
Attachment #406022 - Attachment description: (Cv1) Remove extraneous (and wrong) pref-security.dtd include → (Cv1) Remove extraneous (and wrong) pref-security.dtd include [Checkin: Comment 36]
(Reporter)

Updated

8 years ago
Attachment #404930 - Attachment description: Patch for checkin → Patch for checkin [Checkin: See comment 21+24+25]
(Reporter)

Updated

8 years ago
Blocks: 525536
(Reporter)

Comment 37

8 years ago
(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.

Updated

7 years ago
Blocks: 559816

Comment 38

7 years ago
(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...
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.

Updated

7 years ago
Blocks: 573384

Comment 40

7 years ago
(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.

Updated

7 years ago
Blocks: 574645
(Reporter)

Updated

6 years ago
Blocks: 729000
You need to log in before you can comment on or make changes to this bug.