819493 - Know your rights notification box should be default snippet instead

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Johnathan Nightingale [:johnath]]

I was talking with some folks today about how we have too many notification bars happening on first/second/third run (know your rights, telemetry, obsolete plugins, FHR). When we wrote the Know Your Rights notification box, we didn't have about:home and its dynamic snippet area, but now we do. I suggest that we remove the notification bar for that and instead make it the snippet displayed on first run, before we go and fetch the dynamic snippets we display on future page loads.

I think the code here should be simple, but I'd like to from marketing and legal about whether there are any gotchas here. Gavin notes that we'll need to be a little careful about how we implement it, since the current default snippet isn't guaranteed to be shown, but presumably we'd want that, here.

Comment 1

[Johnathan Nightingale [:johnath]]

Filed legal bug 819495 to ensure that we're covered there.

Comment 2

[Laura Forrest]

This sounds good to me. Agree with the alert overload upon first run experience. 

This may require a update to the Snippet Service. From the Engagement perspective, serving out custom communication based on user life-cycle is something we've wanted to have the ability to do for a while. Perhaps we look at it that way instead of focusing on having the "default" snippet do this job. Those default snippets contain evergreen content designed to show when the snippet service is down. They have generic messaging to fit any user experience. Ideally in this snippet we would want to say "Welcome to Firefox! Did you know..." 

Perhaps file a bug for the Snippet Service, and focus this bug on simply removing the Alert bar?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I don't think we want to block this on snippet service improvements, since that complicates the dependency chain significantly. We don't need to use the existing default snippet mechanism for this; we could have a hard-coded "first run" snippet separate from the default snippets, that is used only first run.

Comment 4

[Laura Forrest]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> I don't think we want to block this on snippet service improvements, since
> that complicates the dependency chain significantly. We don't need to use
> the existing default snippet mechanism for this; we could have a hard-coded
> "first run" snippet separate from the default snippets, that is used only
> first run.

Fine by me. 

Marketing requirement: Make sure this First Run snippet is shown only once, to new users only.  I'm happy to provide the copy, graphic, and link - let me know when you need this.

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

Yes, please. Pre-emptive ui-r+ from us. :)

Comment 6

[Johnathan Nightingale [:johnath]]

Who, I wonder aloud, but mostly to Gavin and Dolske, can own such a thing? It doesn't trump other important work, but it is a nice little win and (I would hope, given comment 3 and comment 4) a well-scoped one.

Comment 7

[Justin Dolske [:Dolske]]

Gavin and I already talked about it last week (ohai MWC). Progress soon.

Comment 8

[Mike de Boer [:mikedeboer]]

I will fix up the logic bits, then I'll ping Laura if I need any graphic work done.

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: This is how it looks like after the patch is applied

All the snippet needs is an image that gives an extra "oompf!" to the message. Laura, could you take a look at that, or do you think it's a-ok like this?

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: this patch moves the 'Know Your Rights' notification box to a snippet

Comment 11

[Mike de Boer [:mikedeboer]]

coming up: JSON data exchange and unit test.

Comment 12

[Laura Forrest]

I'm very happy to see this progress and can work with Creative to get a graphic. 

Questions first:

1. Can we optimize the copy string? That messaging is sorely out of date and could read a lot stronger. 

2. When exactly will a user see this snippet?

Comment 13

[Mike de Boer [:mikedeboer]]

Answers to your questions:

1. You can optimize the copy string to whatever you like, as far as I'm concerned, but I don't know whether the legal dept needs to check that change too.
2. A user will see this snippet at the exact same time when the notification bar appeared. This is the same as before:
  1. `browser.rights.override` is set to `true` in about:config
  2. the message was not seen by the user before (for example the first launch after a fresh install)

Comment 14

[Laura Forrest]

Thanks. Then two more questions:

3. Can we verify the legal necessity and legal requirements of this messaging? (the answer may be in legal bug 819495 which I do not have access to)

4. How many times will a new user (individual new profile, etc) see this snippet?

Comment 15

[Laura Forrest]

(In reply to Laura Forrest from comment #14)
> Thanks. Then two more questions:
> 
> 3. Can we verify the legal necessity and legal requirements of this
> messaging? (the answer may be in legal bug 819495 which I do not have access
> to)

Done - I now have access to this bug. Thanks Johnath.

Comment 16

[Laura Forrest]

Removing the Creative bug as a dependency. Let's not have that block this goodness and implement this first basic step as is (no graphic, same copy). Will follow up with graphic/copy changes in a patch.

Comment 17

[Johnathan Nightingale [:johnath]]

In which case I'd say this bug now blocks bug 848118 instead!

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Added unit tests and now passing serialized JSON between documents

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 721710 [details] [diff] [review]
Added unit tests and now passing serialized JSON between documents

>diff --git a/browser/base/content/abouthome/aboutHome.css b/browser/base/content/abouthome/aboutHome.css

>+#rightsSnippet {
>+  background-image: url("chrome://browser/content/abouthome/rights.png");

Hrm, this image (and the 2x version) doesn't seem to be in the patch. Did you forget to |hg add| it?

>diff --git a/browser/base/content/abouthome/aboutHome.js b/browser/base/content/abouthome/aboutHome.js

> #ifdef XP_MACOSX
>-  , imageHD: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAIwAAAA4CAYAAAAvmxBdAAAAGXRFWHRTb2<snip>
>+  , imageHD: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAIwAAAA4CAYAAAAvmxBdAAAAGXRF" +

I actually kind of prefer having it all on one line, much easier to copy/paste and replace.

>-function ensureSnippetsMapThen(aCallback)
>-{
>+function ensureSnippetsMapThen(aCallback) {

aw yeah proper bracing style

>+function showSnippets() {

>+    snippetsElt.innerHTML = "<span id=\"rightsSnippet\">" + rightsData.text +
>+                            " <a href=\"about:rights\">" + rightsData.linkText +
>+                            "</a></span>";

This string concatenation may not be acceptable for localization and/or RTL. I think we might need to do something like http://hg.mozilla.org/mozilla-central/annotate/1b49fb552e18/browser/base/content/aboutDialog.xul#l71, where we allow localizers to add text before and after the link. But I'm not certain about this - ask Axel?

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+    docElt.setAttribute("knowYourRightsData", AboutHomeUtils.knowYourRightsData);

Seems like it might be simpler to just have this code be something like:

let knowYourRightsData = AboutHomeUtils.knowYourRightsData;
if (knowYourRightsData)
  docElt.setAttribute(...)

and have AboutHomeUtils return null when nothing needs to be shown (and have the code in aboutHome.js just check for the presence of the attribute, rather than rightsData.show).

>diff --git a/browser/base/content/test/browser_aboutHome.js b/browser/base/content/test/browser_aboutHome.js

>-  setup: function (aSnippetsMap)
>-  {
>+  setup: function (aSnippetsMap) {

Hrm, now it's starting to seem gratuitous :)

>+  desc: "Check if the 'Know Your Rights default snippet is shown when 'browser.rights.override' pref is set",
>+  beforeRun: function() {
>+    Services.prefs.clearUserPref("browser.rights.override");
>+    Services.prefs.setBoolPref("browser.rights.override", false);

clearUserPref before setBoolPref is redundant, FWIW.

>diff --git a/browser/modules/Makefile.in b/browser/modules/Makefile.in

> EXTRA_PP_JS_MODULES = \
>+	AboutHomeUtils.jsm \

You should remove it from EXTRA_JS_MODULES if you're adding it here, otherwise we will copy it over twice unecessarily during build time.

r- for the potential l10n issue and missing images, but with all of these comments addressed this should be ready to go.

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Update as oer Gavin's review and comments

Gavin, thanks for the thorough review!

* The background images won't be added until bug 848118 is resolved (see comment 16)
* You're right about the l10n issue here, so I changed the locale properties
* put back the one-line PNG for OSX
* implemented knowYourRights attribute handling on the pages' documentElement as per your suggestion and adjusted the tests accordingly

Over the years I've adopted the practice to 'correct' coding style mis-alignments whenever I touch that code directly. My idea behind this is that doing it like this gives me the idea that we keep striving for a uniform codebase, while not acting like style-nazi and start correcting each and every flaw in random files.

This is off-topic, but I wanted to mention it anyway: when I look at the codebase as a whole, I get the feeling that too many patches were landed in the past where reviewers eventually skipped over the nits. I actually respect reviewers that dare putting 'Nit: ...' style comments in a review, because it shows that he or she cares about keeping the tree clean and exemplary.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 726131 [details] [diff] [review]
Update as oer Gavin's review and comments

(In reply to Mike de Boer [:mikedeboer] from comment #20)
> * The background images won't be added until bug 848118 is resolved (see
> comment 16)

Seems like you shouldn't add the related style yet, then (or the empty #rightsSnippet {} block).

Please avoid making the bracing changes in this patch.

>diff --git a/browser/base/content/test/browser_aboutHome.js b/browser/base/content/test/browser_aboutHome.js

>+    ok(rightsData.start, "There is a snippet text");

This could check "rightsData.start || rightsData.end" to be localization-agnostic (we don't currently run browser-chrome tests in non-en-US builds, but we might want to someday).

>+  desc: "Check if the 'Know Your Rights default snippet is NOT shown when 'browser.rights.override' pref is NOT set",

Wouldn't a more accurate description be "after it has already been shown once (in the previous test)"?

>diff --git a/browser/modules/AboutHomeUtils.jsm b/browser/modules/AboutHomeUtils.jsm

>+  get knowYourRightsData() {

>+    let productName = brandBundle.GetStringFromName("brandFullName");
>+    let linkText = rightsBundle.GetStringFromName("aboutRights.linkText");
>+    let start = rightsBundle.formatStringFromName("aboutRights.start", [productName], 1);
>+    let end = rightsBundle.formatStringFromName("aboutRights.end", [productName], 1);

Hrm, don't think is kosher. IIRC formatStringFromName can do bad things if it's expecting a variable to substitute and doesn't find one in the string. Axel would probably know the details. But in any case it seems like this would be simpler if you added the relevant strings to aboutHome.dtd, similar to how abouthome.defaultSnippet1.v1 works:

<!-- LOCALIZATION NOTE (rightsSnippet.message) text in <a> will be linked to about:rights -->
<!ENTITY rightsSnippet.message "&brandShortName; is free and open source software from the non-profit Mozilla Foundation. <a>Know your rights…</a>">

and then have the about:home code linkify the <a> appropriately.

(I think both this link and the default snippet links should actually be branding-specific, i.e. in brand.dtd, but I can file a separate bug about that.)

>+    // Set pref to indicate we've shown the notification.
>+    let currentVersion = Services.prefs.getIntPref("browser.rights.version");
>+    Services.prefs.setBoolPref("browser.rights." + currentVersion + ".shown", true);

It's a bit risky that the setting of the "shown" pref is now decoupled from the actual showing. This currently relies on the AboutHomeUtils.knowYourRightsData getter only ever being called when about:home is going to be displayed, which seems a bit fragile. It would be nice to avoid that somehow - I suppose you could just move this block of code to the browser.js block where the AboutHomeUtils.knowYourRightsData is currently accessed.

>+function shouldShowRights() {

>+#ifndef OFFICIAL_BUILD
>+    // Non-official builds shouldn't show the notification.
>+    return false;
>+#endif

This #define will never be true, because you didn't also move over the block at:
http://hg.mozilla.org/mozilla-central/annotate/5dbcbd03d7ba/browser/components/Makefile.in#l23
to browser/modules/Makefile.in. Should test this manually to make sure official/non-official builds work properly.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

> Over the years I've adopted the practice to 'correct' coding style
> mis-alignments whenever I touch that code directly. My idea behind this is
> that doing it like this gives me the idea that we keep striving for a
> uniform codebase, while not acting like style-nazi and start correcting each
> and every flaw in random files.

People will generally tolerate this to a certain degree, but in general if roughly half of the hunks in the patch are style changes, people will call you on it :)

"striving for a uniform codebase" is relatively low on our list of priorities, particularly given the diversity and breadth of Mozilla's code and dev community. So "make patches that are simple to review" often trumps "converge on a single bracing style".

Comment 23

[Mike de Boer [:mikedeboer]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #22)
> "striving for a uniform codebase" is relatively low on our list of
> priorities, particularly given the diversity and breadth of Mozilla's code
> and dev community. So "make patches that are simple to review" often trumps
> "converge on a single bracing style".

*sigh* I knew this is one trait that would end up biting me instead, but the solution is simple: I will refrain from doing this from now on.

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Know Your Rights notification box has moved to be shown as default snippet on first startup

Locale data is now in a DTD. I am trying to build an Official Build to test the build flags, I will report back here when I've succeeded.

Gavin: I think I addressed the issues from your review. Could you check that for me?

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 732300 [details] [diff] [review]
Know Your Rights notification box has moved to be shown as default snippet on first startup

>diff --git a/browser/base/content/abouthome/aboutHome.js b/browser/base/content/abouthome/aboutHome.js

> function showSnippets()

>+    rightsElt.setAttribute("hidden", false);

Hrm, does this work? In HTML its the existence of the "hidden" attribute that matters, rather than its value, so I would think this would need to be removeAttribute("hidden")... Oh, I see that you added a #rightsSnippet[hidden=false] rule - just switch this and remove that, I think.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+    let showRights = AboutHomeUtils.showKnowYourRights;
>+    if (showRights) {

Might be worth adding a comment that this stuff all needs to happen before setting searchEngineURL, since that's what triggers the display of the snippets.

>diff --git a/browser/base/content/test/browser_aboutHome.js b/browser/base/content/test/browser_aboutHome.js

>+  desc: "Check if the 'Know Your Rights default snippet is shown when 'browser.rights.override' pref is set",

>+  run: function (aSnippetsMap) {

>+    let showRights = AboutHomeUtils.showKnowYourRights;
>+
>+    ok(!!showRights, "Rights snippet ought to be shown");
>+    is(showRights, "true", "Attribute value should be 'true'");

nit: these checks are redundant, and the term "attribute value" is kind of vague. Since you're checking that the snippet was actually shown anyways, I don't think it's useful to check the AboutHomeUtils getter directly.

>+  run: function (aSnippetsMap) {

>+    let rightsData = AboutHomeUtils.knowYourRightsData;
>+    
>+    ok(!rightsData, "Rights snippet ought NOT to be shown");

nit: similarly, this doesn't accurately describe what it's checking. The value of AboutHomeUtils.knowYourRightsData doesn't reflect whether a snippet was shown, so adjust the message accordingly.

> function test()

>+      if (test.tearDown)
>+        yield test.tearDown();

This is not actually used anymore so perhaps you shouldn't add it.

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>-  _shouldShowRights: function BG__shouldShowRights() {

>-#ifndef OFFICIAL_BUILD

You can get rid of the corresponding DEFINES line in browser/components/Makefile.in now, since this is its only use.

>diff --git a/browser/locales/en-US/chrome/browser/aboutHome.dtd b/browser/locales/en-US/chrome/browser/aboutHome.dtd

>+<!-- LOCALIZATION NOTE (abouthome.rightsSnippet):
>+     text in <a/> will be linked to the featured add-ons on addons.mozilla.org

"will be linked to about:rights"

>diff --git a/browser/modules/AboutHomeUtils.jsm b/browser/modules/AboutHomeUtils.jsm

>+  get showKnowYourRights() {
>+    return shouldShowRights() ? "true" : null;

Seems like this should just return a boolean.

>+function shouldShowRights() {

>+#ifndef OFFICIAL_BUILD
>+    // Non-official builds shouldn't show the notification.
>+    return false;
>+#endif

nit: copy/paste means these lines are indented too much

>+  // Legacy: If the user accepted a EULA, we won't annoy them with the
>+  // equivalent about:rights page until the version changes.
>+  try {
>+    return !Services.prefs.getBoolPref("browser.EULA." + currentVersion + ".accepted");
>+  } catch (e) { }

Worth filing a followup to remove this now (it hasn't been set since forever).

r=me with those addressed, thanks for being patient with the multiple review cycles!

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Know Your Rights notification box has moved to be shown as default snippet on first startup

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 732768 [details] [diff] [review]
Know Your Rights notification box has moved to be shown as default snippet on first startup

Review of attachment 732768 [details] [diff] [review]:
-----------------------------------------------------------------

Since I want to be annoying today, https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#JavaScript_objects has braces on a new line for functions :)
This is not to say it's important, it's not. Just wanted to point out how it's hard (and pointless) to have a common style that everyone likes.
Fwiw, some of us also likes using a space to label anonymous functions, thus why you found "function ()" instead of "function()".
Again, this doesn't matter, we usually just care a single code module is coherent with itself.

::: browser/base/content/abouthome/aboutHome.js
@@ +294,5 @@
> +    snippetsElt.appendChild(rightsElt);
> +    rightsElt.removeAttribute("hidden");
> +    return;
> +  }
> +  

trailing space

::: browser/base/content/browser.js
@@ +2654,4 @@
>      docElt.setAttribute("snippetsURL", AboutHomeUtils.snippetsURL);
> +    let showRights = AboutHomeUtils.showKnowYourRights;
> +    if (showRights) {
> +      docElt.setAttribute("showKnowYourRights", showRights);

this will always set to true, no need to cache showRights into a temp var, you may just
if (AboutHomeUtils.showKnowYourRights) {
  docElt.setAttribute("showKnowYourRights", "true");

::: browser/base/content/test/browser_aboutHome.js
@@ +212,5 @@
> +  desc: "Check if the 'Know Your Rights default snippet is shown when 'browser.rights.override' pref is set",
> +  beforeRun: function() {
> +    Services.prefs.setBoolPref("browser.rights.override", false);
> +  },
> +  setup: function () { },

why the need for beforeRun instead of using setup?

@@ +221,5 @@
> +    ok(showRights, "AboutHomeUtils.showKnowYourRights should be TRUE");
> +
> +    let snippetsElt = doc.getElementById("snippets");
> +    ok(snippetsElt, "Found snippets element");
> +    is(snippetsElt.getElementsByTagName("a")[0].href, "about:rights", "Snippet link is present.");

the test should cleanup after itself, this test is setting a pref and not clearing it before existing.

@@ +230,5 @@
> +  desc: "Check if the 'Know Your Rights default snippet is NOT shown when 'browser.rights.override' pref is NOT set",
> +  beforeRun: function () {
> +    Services.prefs.clearUserPref("browser.rights.override");
> +  },
> +  setup: function () { },

ditto

@@ +234,5 @@
> +  setup: function () { },
> +  run: function (aSnippetsMap) {
> +    let doc = gBrowser.selectedTab.linkedBrowser.contentDocument;
> +    let rightsData = AboutHomeUtils.knowYourRightsData;
> +    

trailing spaces

Comment 28

[Mike de Boer [:mikedeboer]]

Attachment: Know Your Rights notification box has moved to be shown as default snippet on first startup

Adjust code as per Marco's comments.

Marco: you are right about the style guide. I did not try to intentionally piss you off, please understand that. I didn't realize I used a different formatting in the test file, so I changed it to be consistent throughout.
In the test I introduced `beforeRun`, because when I used `setup` instead, the tests I wrote would fail, due to a timing issue.
I re-added `tearDown` to clean up the prefs after the test has run. I hope that is to your liking and/ or acceptable.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #27)
> Since I want to be annoying today,
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style#JavaScript_objects has braces on a new line for functions :)

I hate that document because it's usually a lie, but I fixed that example anyways.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #27)
> why the need for beforeRun instead of using setup?

setup runs after the snippets have been initialized, which is too late for this AIUI.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 732857 [details] [diff] [review]
Know Your Rights notification box has moved to be shown as default snippet on first startup

The more times you ask me to review this the more comments I'll find :P

>diff --git a/browser/base/content/abouthome/aboutHome.css b/browser/base/content/abouthome/aboutHome.css

>+#rightsSnippet[hidden] {
>+  display: none;

This shouldn't be needed, since "hidden" in HTML should just work?

>diff --git a/browser/base/content/test/browser_aboutHome.js b/browser/base/content/test/browser_aboutHome.js

> registerCleanupFunction(function() {

>+  try {
>+    Services.prefs.clearUserPref("browser.rights.override");
>+  } catch (ex) {}

FWIW clearUserPref now never throws (since bug 487059), so the try/catches aren't necessary.

>+  desc: "Check if the 'Know Your Rights default snippet is NOT shown when 'browser.rights.override' pref is NOT set",

>+  beforeRun: function ()
>+  {
>+    Services.prefs.clearUserPref("browser.rights.override");

The default value of this pref differs depending on whether DEBUG is defined, so I think this test may not work consistently.

I also just realized that the test profile has browser.EULA.override set (http://hg.mozilla.org/mozilla-central/annotate/3a5929ebc886/build/automation.py.in#l485), which may be messing with these tests in unexpected ways. It's a bit frustrating that there's no easy way to test the thing we actually want to test, which is that the rights snippet shows up on first launch.

>diff --git a/browser/modules/Makefile.in b/browser/modules/Makefile.in

>+ifdef MOZILLA_OFFICIAL
>+DEFINES += -DOFFICIAL_BUILD=1
>+endif

While we're at it, can you rename OFFICIAL_BUILD to MOZILLA_OFFICIAL to match the thing it's actually depends on? Mapping MOZILLA_OFFICIAL to a slightly differently named define is just confusing.

Comment 32

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #30)
> (In reply to Marco Bonardo [:mak] from comment #27)
> > why the need for beforeRun instead of using setup?
> 
> setup runs after the snippets have been initialized, which is too late for
> this AIUI.

setup() should run after the snippets map has been initialized but before loadSnippets has been invoked. some of the tests are indeed setting "snippets" in setup(). If that's not the case, there must be something broken.

Comment 33

[Marco Bonardo [:mak]]

and fwiw, you don't need a teardown(), just do whatever you need at the end of run()?

Comment 34

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [:mak] from comment #32)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #30)
> > (In reply to Marco Bonardo [:mak] from comment #27)
> > > why the need for beforeRun instead of using setup?
> > 
> > setup runs after the snippets have been initialized, which is too late for
> > this AIUI.

ah, rather the problem is that the attribute is set on the document element before setup(), so changing the prefs in setup is not enough, they must be changed before the tab is loaded, though setup may probably directly change the attribute.

Comment 35

[Mike de Boer [:mikedeboer]]

Attachment: Know Your Rights notification box has moved to be shown as default snippet on first startup

added proposed changes of Gavin and Marco.

Comment 36

[Mike de Boer [:mikedeboer]]

Attachment: Know Your Rights notification box has moved to be shown as default snippet on first startup

updated commit message, carrying over r+

Comment 37

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 732985 [details] [diff] [review]
Know Your Rights notification box has moved to be shown as default snippet on first startup

>diff --git a/browser/base/content/abouthome/aboutHome.css b/browser/base/content/abouthome/aboutHome.css

>+#rightsSnippet[hidden] {
>+  display: none;
>+}

You seem to have forgotten to remove this, per comment 25 and comment 31.

Comment 38

[Mike de Boer [:mikedeboer]]

Gavin: I didn't forget it, because 'hidden' apparently doesn't work in this case; the CSS 'display' attribute may override this. It inherits the 'display:block' style from the other default snippets and I didn't want to touch that too much, so I opted for this solution - to explicitly set 'display' depending on the hidden attribute being set.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to inbound. Please rebase.

Comment 40

[Mike de Boer [:mikedeboer]]

Attachment: Know Your Rights notification box has moved to be shown as default snippet on first startup

unbitrotted, carrying over r+

Comment 41

[Mike de Boer [:mikedeboer]]

Attachment: Know Your Rights notification box has moved to be shown as default snippet on first startup

updated commit message. carrying over r+=gavin

Comment 42

[Mike de Boer [:mikedeboer]]

RyanVM: I'm sorry to have wasted your time with this outdated patch! updated and tested the current one.

Comment 43

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b50153f9e011

Comment 44

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/278ea1d0e072 - I don't really know what that buildstep is doing, apparently some sort of test for whether we're breaking l10n repacks, but if you search for "mozpack.errors.AccumulatedErrors" in https://tbpl.mozilla.org/php/getParsedLog.php?id=21625292&tree=Mozilla-Inbound, you'll see the thing that failed in a couple of Windows opt builds.

I look forward to hearing the explanation you find out, about what on earth it is, and whether the way it only failed in some builds means that it has broken dependencies, or that whenever you remove a dtd/properties file, you now have to clobber, or have to clobber Windows, or... dunno.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\browser\build.mk:55:0: command 'C:/mozilla-build/python27/python.exe e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/build/pymake/pymake/../make.py -C browser/locales l10n-check' failed, return code 2
Error: Missing file: ../../dist/xpi-stage/locale-x-test\chrome/x-test/locale/x-test/global/aboutRights.properties

The patch removes aboutRights.properties. philor suggests a clobber might solve it, but I'd like to understand why that would be needed. Is the "$(RM) -rf x-test" in "make l10n-check" somehow failing (without showing an error in the log...)? Note also this seems to only happen on some slaves/builds, which suggests some build config/harness issue.

Comment 46

[Axel Hecht [:Pike]]

No idea, I can't really read logs these days. From glancing at it, it seems that the installer repacks OK, but the zip doesn't. Maybe glandium has an idea.

Comment 47

[Mike Hommey [:glandium]]

I think this is what is happening:
- on incremental builds, we re-use dist/bin, which contains aboutRights.properties from a previous build.
- dist/bin is packaged in dist/firefox and then as a package, which thus contains aboutRights.properties
- the package is unpacked for l10n-check
- then a new directory is created for the x-test locale, and filled according to the jar manifests, thus not copying aboutRights.properties.
- the l10n-repack code wants all chrome locale files from the original locale (en-US, coming from the unpacked package) to be found in the new locale (x-test, filled above), and fails because x-test doesn't have aboutRights.properties.

Maybe we should remove dist/bin when starting an incremental build as we currently do for sdk, include, etc. That would avoid a clobber in such cases.

Comment 48

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #47)
> Maybe we should remove dist/bin when starting an incremental build as we
> currently do for sdk, include, etc. That would avoid a clobber in such cases.

At least, until we have something better.

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b50153f9e011

Comment 50

[Phil Ringnalda (:philor)]

m-cMerge, y u no see my backouts?

Comment 51

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mike Hommey [:glandium] from comment #48)
> (In reply to Mike Hommey [:glandium] from comment #47)
> > Maybe we should remove dist/bin when starting an incremental build as we
> > currently do for sdk, include, etc. That would avoid a clobber in such cases.
> 
> At least, until we have something better.

Sounds reasonable. Do you know when this became a problem? Is it related to your relatively recent packaging changes? I have a hard time believing we haven't removed a properties file since then.

Comment 52

[Mike Hommey [:glandium]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #51)
> Sounds reasonable. Do you know when this became a problem? Is it related to
> your relatively recent packaging changes? I have a hard time believing we
> haven't removed a properties file since then.

The recent l10n repack changes (for some value of recent) are what causes l10n-check to fail, because the new l10n repack code is stricter (which is good). We've been badly handling file removals for like ever, but there must have been no localized properties file removal since the l10n repack landing.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

Re-landed on top of bug 860371:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c950d2ea4c5b

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c950d2ea4c5b

Comment 55

[Jean Collings]

Hi Gavin,

Taking a look at our current "first run" default snippet as well as our two other default snippets (pointing to features and addons) - I would like to explore in updating copy/icon and CTAs. Are there legal ramifications or reasons that these snippets have to be what they currently are or do we have the flexibility to change them?

Please let me know, thanks!

Jean

Comment 56

[:Gavin Sharp [email: [e-mail]]]

Hi Jean,

The wording of the "first run" snippet was chosen carefully, since it was a straight replacement for the "know your rights" info bar. Changing that would require a discussion to make sure that we're not losing important first-run messaging.

The wording and style of the other two "default snippets" are completely open to modification - feel free to file a bug and CC me to discuss any changes.