1223573 - Move Hello to a system add-on as part of Go Faster

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

As part of our ongoing Hello efforts, we want to move to Go faster, and as part of that we need to be a system add-on.

Bug 1186172 is experimenting with an add-on, this bug will do the actual move in mozilla-central.

Bug 1216371 is implementing part of the build system that we need to be built as an add-on.

Comment 1

[Mark Banner (:standard8)]

Attachment: Part 1. Move loop to browser/extensions/loop.

First part, this is simple mvs which can be summarised as:

- browser/components/loop -> browser/extensions/loop
- browser/extensions/loop/content/{*.html,css,js,libs} -> browser/extensions/loop/content/panels/
- browser/extensions/loop/content/panels/libs -> browser/extensions/loop/content/vendor
- browser/extensions/loop/content/shared/libs -> browser/extensions/loop/shared/vendor
- browser/extensions/loop/content/shared/img/svg -> browser/extensions/loop/shared/img

The last four are reflecting the work going on in the github repo.

No code changes here, they are in the following patches.

Comment 2

[Mark Banner (:standard8)]

Attachment: Part 2. Move prefs and css into the new Loop system add-on.

This moves the prefs from firefox.js and the relevant css from the browser to the add-on directories.

Comment 3

[Mark Banner (:standard8)]

Attachment: Part 3. Move browser-loop.js to begin forming bootstrap.js.

This moves browser-loop.js to become at this stage the skeleton bootstrap.js. Future parts will make it functional.

Comment 4

[Mark Banner (:standard8)]

Attachment: Part 4. Build system changes for Loop as a system add-on.

This does the necessary changes to add Loop to the build system as a system add-on.

The extension id I've just gone with [e-mail] for now - since that just uses the code name like the rest of the code. The min version I've capped at 45.0a1 since we won't be able to install before then. For the max version, I'm using the current Firefox version. We might need to revisit this when we actually start shipping system add-ons, but for now it'll do.

The jar.mn is setup to produce the correct chrome.manifest for what we want.

Mike Hommey: can you review the build system parts? I'll tag Mike de Boer for the eslint & other parts.

Comment 5

[Mark Banner (:standard8)]

Attachment: Part 5. Update uris and paths for files in Loop's system add-on.

This is pure uri/path replacements to cope with the new structures generated due to the moves, and due to the jar.mn/chrome.manifest layout requirements.

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 6. Tidy up bootstrap.js add enough setup to get xpcshell-tests passing again.

There's a bit of eslinting on the bootstrap.js going on here (with eslint -c .eslintrc-gecko) just to tidy it up a bit, and also we start loading the default prefs.

The head.js change in xpcshell adds the startup of the add-on manager so that our system add-on gets registered.

When we move to the github repo, we might need to revisit some of this, but that can wait until then.

Comment 7

[Mark Banner (:standard8)]

Attachment: Part 7. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again.

This completes the set - the support for hooking up loop button and our functions into the browser UI is here.

We also get all tests passing again with this.

Comment 8

[Mark Banner (:standard8)]

Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ec45fc9ee7a

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8691971 [details] [diff] [review]
Part 2. Move prefs and css into the new Loop system add-on.

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

As a more general question, not blocking review per sé, but does our styling code (CSS, SVG) have to live in a directory called 'skin' and not 'themes'? Is it an add-on convention of some sort?

::: browser/extensions/loop/skin/shared/loop.css
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");

nit: in the other files you leave a newline before this namespace declaration.

@@ +196,5 @@
> +  .chat-toolbarbutton.chat-loop-hangup:-moz-any(:hover,:hover:active) {
> +    background-color: #ef6745;
> +    border-color: #ef6745;
> +  }
> +

nit: please remove these two newlines and instead leave a newline at the end of the file.

Comment 10

[Mark Banner (:standard8)]

Attachment: Part 1. Move loop to browser/extensions/loop.

Updated patcheset due to bitrot.

Also, fresh try push on a hopefully good tree: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be8d2c8c797

Comment 11

[Mark Banner (:standard8)]

Attachment: Part 2. Move prefs and css into the new Loop system add-on.

Comment 12

[Mark Banner (:standard8)]

Attachment: Part 3. Move browser-loop.js to begin forming bootstrap.js.

Comment 13

[Mark Banner (:standard8)]

Attachment: Part 4. Build system changes for Loop as a system add-on.

Comment 14

[Mark Banner (:standard8)]

Attachment: Part 5. Update uris and paths for files in Loop's system add-on.

Comment 15

[Mark Banner (:standard8)]

Attachment: Part 6. Tidy up bootstrap.js add enough setup to get xpcshell-tests passing again.

Comment 16

[Mark Banner (:standard8)]

Attachment: Part 7. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again.

Comment 17

[Mike Hommey [:glandium]]

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> Comment on attachment 8691971 [details] [diff] [review]
> Part 2. Move prefs and css into the new Loop system add-on.
> 
> Review of attachment 8691971 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As a more general question, not blocking review per sé, but does our styling
> code (CSS, SVG) have to live in a directory called 'skin' and not 'themes'?
> Is it an add-on convention of some sort?

It is a browser chrome convention.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8692662 [details] [diff] [review]
Part 4. Build system changes for Loop as a system add-on.

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

::: .hgignore
@@ +87,5 @@
> +^browser/extensions/loop/test/coverage/desktop
> +^browser/extensions/loop/test/coverage/shared_standalone
> +^browser/extensions/loop/test/visual-regression/diff
> +^browser/extensions/loop/test/visual-regression/new
> +^browser/extensions/loop/test/visual-regression/refs

I don't want to block you, but this suggests things happening in the source tree that should *not* be happening in the source tree. If you don't want to fix that now, please file a followup bug to fix it.

::: browser/extensions/loop/jar.mn
@@ +5,5 @@
> +[.] chrome.jar:
> +% content loop %content/ contentaccessible=yes
> +% skin loop classic/1.0 %skin/linux/ os=Linux
> +% skin loop classic/1.0 %skin/osx/ os=Darwin
> +% skin loop classic/1.0 %skin/windows/ os=WINNT

is it desirable to ship OS-specific files on all platforms?

@@ +28,5 @@
> +% override chrome://loop/skin/[e-mail]      chrome://loop/skin/[e-mail]          os=WINNT osversion=6.3
> +  skin/  (skin/*)
> +  content/modules/ (content/modules/*)
> +  # XXX Do we need this? An extra copy as this file is used from modules as well.
> +  #content/modules/utils.js (content/shared/js/utils.js)

please remove

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8692662 [details] [diff] [review]
Part 4. Build system changes for Loop as a system add-on.

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

::: browser/extensions/loop/moz.build
@@ +11,5 @@
> +]
> +
> +DIST_FILES += [
> +  'install.rdf.in'
> +]

I just got an idea to avoid setting FINAL_TARGET and use a more "natural", albeit a little more annoying construct:

FINAL_TARGET_FILES.features['[e-mail]'] += [ 'bootstrap.js' ]
DIST_FILES.features['[e-mail]'] += ['install.rdf.in']

Then, in jar.mn:
[features/[e-mail]] chrome.jar:

(Note that DIST_FILES.something doesn't work yet, but it is logical to make it work, considering DIST_FILES is essentially meant to be the same as FINAL_TARGET_FILES with preprocessing, and FINAL_TARGET_FILES.something is supported, and I'm going to fix that just now)

Comment 20

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 8692662 [details] [diff] [review]
> Part 4. Build system changes for Loop as a system add-on.
> 
> Review of attachment 8692662 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/loop/moz.build
> @@ +11,5 @@
> > +]
> > +
> > +DIST_FILES += [
> > +  'install.rdf.in'
> > +]
> 
> I just got an idea to avoid setting FINAL_TARGET and use a more "natural",
> albeit a little more annoying construct:
(snip)

Filed bug 1228444, with patches.

Comment 21

[Mark Banner (:standard8)]

Attachment: Part 8. Work around assertions caused by attempting to load the style sheets as author sheets - load them as user sheets for now.

Ok, this works around bug 1228542, its a bit ugly, but it should at least let us get this landed. I've also wrapped some of the platform css files in the browser.xul limitation to make sure they don't get loaded into things we don't want them in.

Try server push is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c43bc01cb7e

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8692661 [details] [diff] [review]
Part 3. Move browser-loop.js to begin forming bootstrap.js.

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

r=me with comments addressed.

::: browser/extensions/loop/bootstrap.js
@@ +1,1 @@
> +// This Source Code Form is subject to the terms of the Mozilla Public

Could you make this a proper license block:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

@@ +1,3 @@
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.

No "use strict";? Please add it.

@@ +1,5 @@
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +var WindowListener = {

Please add JSDoc.

@@ +3,5 @@
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +var WindowListener = {
> +
> +  setupBrowserUI: function(window) {

Please add JSDoc.

@@ +12,5 @@
> +
> +    // the "exported" symbols
> +    var LoopUI;
> +
> +    (function() {

Why the closure? I think you can compress this down a bit by not using a closure. Does this function return `LoopUI`? I think you're making changes in la†er patches, but still...

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8692662 [details] [diff] [review]
Part 4. Build system changes for Loop as a system add-on.

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

I have question below, but I don't want that to block things here.

::: browser/extensions/loop/.eslintrc-gecko
@@ +11,3 @@
>      "restParams": true,
>      "spread": true,
> +    "templateStrings": true,

\o/

::: browser/extensions/loop/install.rdf.in
@@ +1,1 @@
> +<?xml version='1.0' encoding='utf-8'?>

nit: I think we can use double quotes here.

@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +#filter substitution
> +
> +<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:em="http://www.mozilla.org/2004/em-rdf#">

Is RDF case-sensitive? Really?

@@ +5,5 @@
> +
> +#filter substitution
> +
> +<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +

nit: unnecessary newline.

::: browser/installer/package-manifest.in
@@ +675,5 @@
>  @RESPATH@/browser/chrome/icons/default/default16.png
>  @RESPATH@/browser/chrome/icons/default/default32.png
>  @RESPATH@/browser/chrome/icons/default/default48.png
>  #endif
> +@RESPATH@/browser/features/*

What does this do?

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8692664 [details] [diff] [review]
Part 6. Tidy up bootstrap.js add enough setup to get xpcshell-tests passing again.

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

r=me with comments addressed.

::: browser/extensions/loop/bootstrap.js
@@ +1,5 @@
>  // This Source Code Form is subject to the terms of the Mozilla Public
>  // License, v. 2.0. If a copy of the MPL was not distributed with this
>  // file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +"use strict";

nit: you can make this hug the license block comment.

@@ +3,5 @@
>  // file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +"use strict";
> +
> +const { interfaces: Ci, manager: Cm, results: Cr, utils: Cu, classes: Cc } = Components;

Do we use manager? Please review this list.

@@ +392,5 @@
>  
>              try {
>                window.focus();
> +            } catch (ex) {
> +              // Do nothing

nit: missing dot.

@@ +636,5 @@
> + */
> +function loadDefaultPrefs() {
> +  var branch = Services.prefs.getDefaultBranch("");
> +  Services.scriptloader.loadSubScript("chrome://loop/content/preferences/prefs.js", {
> +    pref: (key, val) => {

oooooh, smartness!

::: browser/extensions/loop/test/xpcshell/head.js
@@ +13,5 @@
> +
> +// Setup the add-ons manager for this test.
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +
> +function registerDirectory(aKey, aDir) {

please use normal argument names.

@@ +14,5 @@
> +// Setup the add-ons manager for this test.
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +
> +function registerDirectory(aKey, aDir) {
> +  var dirProvider = {

s/var/let/

@@ +41,5 @@
> +
> +// Now trigger the addons-startup, and hence startup the manager itself. This
> +// should load the manager correctly.
> +var gInternalManager = Cc["@mozilla.org/addons/integration;1"]
> +                    .getService(Ci.nsIObserver)

nit: please align these with two-space indent.

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8692665 [details] [diff] [review]
Part 7. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again.

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

::: browser/extensions/loop/bootstrap.js
@@ +13,2 @@
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");

Please use lazy imports wherever possible.

@@ +651,5 @@
> +    // document.getElementById() etc. will work here
> +    // XXX Add in tear-down of the panel.
> +  },
> +
> +  // nsIWindowMediatorListener functions

nit: missing dots in comments in this section.

@@ +752,5 @@
>    loadDefaultPrefs();
> +
> +  createLoopButton();
> +
> + // Attach to hidden window (for os x).

nit: indentation. s/os x/OS X/

@@ +755,5 @@
> +
> + // Attach to hidden window (for os x).
> +  try {
> +    WindowListener.setupBrowserUI(Services.appShell.hiddenDOMWindow);
> +  }

nit: please keep the braces on the same line. Didn't eslint complain?

@@ +792,5 @@
> +/**
> + * Called when the add-on is shutting down, could be for re-installation
> + * or just uninstall.
> + */
> +function shutdown()

nit: please put the curly after `) `

@@ +801,5 @@
> +  [...Chat.chatboxes].filter(isLoopURL).forEach(chatbox => {
> +    chatbox.content.contentWindow.close();
> +  });
> +
> +  // Detach from hidden window (for os x).

nit: s/os x/OS X/

@@ +817,5 @@
> +  wm.removeListener(WindowListener);
> +
> +  CustomizableUI.destroyWidget("loop-button");
> +
> +  // unload stylesheet

nit: 'Unload stylesheets.'

@@ +819,5 @@
> +  CustomizableUI.destroyWidget("loop-button");
> +
> +  // unload stylesheet
> +  let styleSheetService = Cc["@mozilla.org/content/style-sheet-service;1"]
> +                          .getService(Components.interfaces.nsIStyleSheetService);

nit: please indent with two spaces.

@@ +831,5 @@
> +                                        styleSheetService.USER_SHEET);
> +    }
> +  }
> +
> +  // unload modules

nit: 'Unload modules.'

Comment 26

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> As a more general question, not blocking review per sé, but does our styling
> code (CSS, SVG) have to live in a directory called 'skin' and not 'themes'?
> Is it an add-on convention of some sort?

I think its an add-on convention-ish. You've got chrome://loop/content and chrome://loop/skin (or whatever for the add-on), so it seems to make sense to use skin for the directory.

We can map it differently, I guess I don't have a particular preference, but like you say, we can deal with this later.

Comment 27

[Mike de Boer [:mikedeboer]]

Comment on attachment 8692901 [details] [diff] [review]
Part 8. Work around assertions caused by attempting to load the style sheets as author sheets - load them as user sheets for now.

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

This patch just makes me very, very sad. Oh well, let's hope bug 1228542 will get us in a better state.

Comment 28

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #22)
> ::: browser/extensions/loop/bootstrap.js
> 
> No "use strict";? Please add it.
> 
> > +var WindowListener = {
>
> Please add JSDoc.

These are both added in part 7.

> > +  setupBrowserUI: function(window) {
> 
> Please add JSDoc.

I'll update part 7 & add it.

> > +    // the "exported" symbols
> > +    var LoopUI;
> > +
> > +    (function() {
> 
> Why the closure? I think you can compress this down a bit by not using a
> closure. Does this function return `LoopUI`? I think you're making changes
> in la†er patches, but still...

Yep, we shouldn't be needing that (I think that was a bad copy-over from browser-loop.

Comment 29

[Mark Banner (:standard8)]

(In reply to Mike Hommey [:glandium] from comment #18)
> > +^browser/extensions/loop/test/visual-regression/refs
> 
> I don't want to block you, but this suggests things happening in the source
> tree that should *not* be happening in the source tree. If you don't want to
> fix that now, please file a followup bug to fix it.

Will file a follow-up, but I think Loop's current plans means these should go away soon anyway.

> ::: browser/extensions/loop/jar.mn
> > +% skin loop classic/1.0 %skin/linux/ os=Linux
> > +% skin loop classic/1.0 %skin/osx/ os=Darwin
> > +% skin loop classic/1.0 %skin/windows/ os=WINNT
> 
> is it desirable to ship OS-specific files on all platforms?

We're going to be shipping an additional add-on on AMO as part of Loop, that will need all platforms. Also I haven't seen anything in go faster land yet that says we will ship three different versions of the add-on, so I'm going to assume that we will for now, although I'll drop a note in to the list.

Comment 30

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #23)
> ::: browser/installer/package-manifest.in
> > +@RESPATH@/browser/features/*
> 
> What does this do?

Includes all system add-ons (currently just us) in the packaged build. Without it, we'd be in objdir, but we wouldn't get shipped.

Comment 31

[Mark Banner (:standard8)]

Attachment: Part 1 v3. Move loop to browser/extensions/loop.

Updated patches for review comments & bitrot.

Comment 32

[Mark Banner (:standard8)]

Attachment: Part 2 v3. Move prefs and css into the new Loop system add-on.

Comment 33

[Mark Banner (:standard8)]

Attachment: Part 3 v3. Move browser-loop.js to begin forming bootstrap.js.

Comment 34

[Mark Banner (:standard8)]

Attachment: Part 4 v3. Build system changes for Loop as a system add-on. r=mikedeboer,r=glandium

Comment 35

[Mark Banner (:standard8)]

Attachment: Part 5 v3. Update uris and paths for files in Loop's system add-on.

Comment 36

[Mark Banner (:standard8)]

Attachment: Part 6 v3. Tidy up bootstrap.js add enough setup to get xpcshell-tests passing again.

Comment 37

[Mark Banner (:standard8)]

Attachment: Part 7 v3. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again.

Comment 38

[Mark Banner (:standard8)]

Attachment: Part 8 v3. Work around assertions caused by attempting to load the style sheets as author sheets - load them as user sheets for now.

Comment 39

[Mike de Boer [:mikedeboer]]

Comment on attachment 8692986 [details] [diff] [review]
Part 7 v3. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again.

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

::: browser/extensions/loop/bootstrap.js
@@ +12,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +Cu.import("resource:///modules/CustomizableUI.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

My biggest request was to make these lazy-loading...

@@ +634,3 @@
>      };
>  
>      XPCOMUtils.defineLazyModuleGetter(LoopUI, "hookWindowCloseForPanelClose", "resource://gre/modules/MozSocialAPI.jsm");

I'd like to get rid of this struct as well - but let's do that in a follow-up tech-debt bug.

@@ +640,5 @@
>      XPCOMUtils.defineLazyModuleGetter(LoopUI, "PanelFrame", "resource:///modules/PanelFrame.jsm");
>      XPCOMUtils.defineLazyModuleGetter(LoopUI, "PlacesUtils", "resource://gre/modules/PlacesUtils.jsm");
> +
> +    LoopUI.init();
> +    document.defaultView.LoopUI = LoopUI;

`window.LoopUI = LoopUI;` would do the same thing, no?

Comment 40

[Mark Banner (:standard8)]

Attachment: Part 7. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again.

Update for review comments.

Comment 41

[Mike de Boer [:mikedeboer]]

Comment on attachment 8693022 [details] [diff] [review]
Part 7. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again.

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

Nice, thanks!

::: browser/extensions/loop/bootstrap.js
@@ +16,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
> +  "resource:///modules/CustomizableUI.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +  "resource://gre/modules/Task.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",

nit: 'Lazifying' Services in not necessary, because it's so commonly used - it's highly likely that it's already occupying a compartment.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f3ccccf5b6fe
https://hg.mozilla.org/integration/fx-team/rev/a13b3bba5529
https://hg.mozilla.org/integration/fx-team/rev/1b4d6308002e
https://hg.mozilla.org/integration/fx-team/rev/d6754894897c
https://hg.mozilla.org/integration/fx-team/rev/21ebe3534e58
https://hg.mozilla.org/integration/fx-team/rev/081b0af71d6e
https://hg.mozilla.org/integration/fx-team/rev/14251062e347
https://hg.mozilla.org/integration/fx-team/rev/19876a153a00

Comment 43

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c95f8e8955b0 for what will not prove to be a fun reason: we run xperf on regular Win7 opt builds, which were perfectly happy, but also on Win7 PGO, which was unhappy about your disk accesses, https://treeherder.mozilla.org/logviewer.html#?job_id=5954647&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=5954632&repo=fx-team

Comment 44

[Phil Ringnalda (:philor)]

And also a single intermittent test failure, https://treeherder.mozilla.org/logviewer.html#?job_id=5953113&repo=fx-team

Comment 45

[Mike de Boer [:mikedeboer]]

Ah, I guess the amount of open file handlers increased because we're not doing a delayed startup anymore. At least, that was the thing I could think of.

Comment 46

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #45)
> Ah, I guess the amount of open file handlers increased because we're not
> doing a delayed startup anymore. At least, that was the thing I could think
> of.

Nope, I don't think its that. The log says:

File '{firefox}\browser\features\[e-mail]' was accessed and we were not expecting it.

So I expect this is just the fact that add-on manager is now loading it on startup.

I think we should just add it to the whitelist. I'm getting a patch on try.

The other intermittent failure mention in comment 44, I don't think is anything to do with the changes in these patches, we're not affecting timing of how we load the windows or anything, so I suspect it has potentially been there for a while and not seen until now.

Comment 47

[Mark Banner (:standard8)]

Attachment: Add Loop's new xpi to the talos read whitelist

Ok, this adds the xpi to the whitelist - as we can't do anything else because we want to be an xpi ;-)

I've increased the limits over what the failures said, to give us room to expand as well.

Comment 48

[Mark Banner (:standard8)]

Non-PGO try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22fcb714a621

PGO try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf1de0bdc5ea

Comment 49

[Mike de Boer [:mikedeboer]]

Comment on attachment 8693244 [details] [diff] [review]
Add Loop's new xpi to the talos read whitelist

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

::: testing/talos/talos/xtalos/xperf_whitelist.json
@@ +5,5 @@
>   "C:\\$Secure": {"ignore": true},
>   "C:\\$logfile": {"ignore": true},
>   "{firefox}\\omni.ja": {"mincount": 0, "maxcount": 46, "minbytes": 0, "maxbytes": 3014656},
>   "{firefox}\\browser\\omni.ja": {"mincount": 0, "maxcount": 28, "minbytes": 0, "maxbytes": 1835008},
> + "{firefox}\\browser\\features\\[e-mail]": {"mincount": 0, "maxcount": 100, "minbytes": 0, "maxbytes": 10000000},

Looks reasonable to me, but I'd like :jmaher to s-r this.

Comment 50

[Mark Banner (:standard8)]

Try pushes were green enough with the additional fix.

Comment 51

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8693244 [details] [diff] [review]
Add Loop's new xpi to the talos read whitelist

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

looks good to me.  I assume this .xpi will always be included with Firefox distributions?

Comment 52

[Mark Banner (:standard8)]

(In reply to Joel Maher (:jmaher) from comment #51)
> Comment on attachment 8693244 [details] [diff] [review]
> Add Loop's new xpi to the talos read whitelist
> 
> Review of attachment 8693244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good to me.  I assume this .xpi will always be included with Firefox
> distributions?

Yes. There's a question over esr branches, but we'll resolve that when we get there.

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a0de6bc9779f
https://hg.mozilla.org/integration/fx-team/rev/ea7d46e2b42d
https://hg.mozilla.org/integration/fx-team/rev/60489607e7d3
https://hg.mozilla.org/integration/fx-team/rev/de0a9431b7c7
https://hg.mozilla.org/integration/fx-team/rev/e19308d96a1d
https://hg.mozilla.org/integration/fx-team/rev/117061bb2218
https://hg.mozilla.org/integration/fx-team/rev/722d3a01dcfc
https://hg.mozilla.org/integration/fx-team/rev/ab436aa1b1ed
https://hg.mozilla.org/integration/fx-team/rev/29ce9059dc2c

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0de6bc9779f
https://hg.mozilla.org/mozilla-central/rev/ea7d46e2b42d
https://hg.mozilla.org/mozilla-central/rev/60489607e7d3
https://hg.mozilla.org/mozilla-central/rev/de0a9431b7c7
https://hg.mozilla.org/mozilla-central/rev/e19308d96a1d
https://hg.mozilla.org/mozilla-central/rev/117061bb2218
https://hg.mozilla.org/mozilla-central/rev/722d3a01dcfc
https://hg.mozilla.org/mozilla-central/rev/ab436aa1b1ed
https://hg.mozilla.org/mozilla-central/rev/29ce9059dc2c

Comment 55

[:Gijs (he/him)]

Comment on attachment 8693022 [details] [diff] [review]
Part 7. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again.

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

This is going to re-add loop to the navbar for people who have already removed it. I don't see this being considered. Is there a migration plan of sorts? See also bug 1215694 where Shane has been working on doing the same thing for Pocket. Did you talk about this? It seems like the two bugs are taking pretty different approaches in this respect, which I find surprising.

Comment 56

[Mark Banner (:standard8)]

(In reply to :Gijs Kruitbosch from comment #55)
> This is going to re-add loop to the navbar for people who have already
> removed it. I don't see this being considered. Is there a migration plan of
> sorts? 

I must admit I didn't test the off-navbar case, however I did check that it got put back in the same place (i.e. no change going from pre-to-post system add-on). 

So, without having time to test, I think this is probably a non-issue, mainly as we didn't remove "loop-button" from navbarPlacements in CustomizableUI.jsm.

> See also bug 1215694 where Shane has been working on doing the same
> thing for Pocket. Did you talk about this? It seems like the two bugs are
> taking pretty different approaches in this respect, which I find surprising.

No, I certainly had no idea that work was going on.

It seems like that bug is basically doing the same, but then doing a lot more manual work around menu items and things. From what I've worked out so far, is that our stuff does more or less the right things with the limited code we have - but we're not fully restartless yet.

Can you outline if there's other areas you're concerned about? At the moment, I'm more tempted just to test it and see what we really need, rather than trying to copy a whole load of browser functionality for unknown benefit.

Comment 57

[:Gijs (he/him)]

(In reply to Mark Banner (:standard8) from comment #56)
> (In reply to :Gijs Kruitbosch from comment #55)
> > This is going to re-add loop to the navbar for people who have already
> > removed it. I don't see this being considered. Is there a migration plan of
> > sorts? 
> 
> I must admit I didn't test the off-navbar case, however I did check that it
> got put back in the same place (i.e. no change going from pre-to-post system
> add-on). 
> 
> So, without having time to test, I think this is probably a non-issue,
> mainly as we didn't remove "loop-button" from navbarPlacements in
> CustomizableUI.jsm.

Well, no, the point is that because it was a builtin button and is now becoming an add-on button, if the user had removed it, CUI will think this is a "new" button and needs placing somewhere in the UI. So it will be moved back into the UI.

It will then get added to "seen" buttons and won't ever be placed anywhere else again. But it'll be annoying for users who have removed it. We have UI telemetry on that, and it feels to me like Hello is marginally less disliked than Pocket by a particular set of users, so it might be worth checking that before bothering - though it seems that data is hard to get at. Ilana and/or Blake would know more.

> > See also bug 1215694 where Shane has been working on doing the same
> > thing for Pocket. Did you talk about this? It seems like the two bugs are
> > taking pretty different approaches in this respect, which I find surprising.
> 
> No, I certainly had no idea that work was going on.

Comms fail. How is that possible? Dave/Shane, are you aware of anyone else doing things like this who might need to be aware of the other examples in this area? It seems we're reinventing the wheel several times over, which seems unfortunate.

> It seems like that bug is basically doing the same, but then doing a lot
> more manual work around menu items and things.

I'm not really sure what you mean. Hello has a menuitem now, too, right? How is that being handled?

> From what I've worked out so
> far, is that our stuff does more or less the right things with the limited
> code we have - but we're not fully restartless yet.

What does this mean? I don't see any followup bugs about this.

> Can you outline if there's other areas you're concerned about? At the
> moment, I'm more tempted just to test it and see what we really need, rather
> than trying to copy a whole load of browser functionality for unknown
> benefit.

It's not so much copying a whole load of functionality as it is about ensuring we don't mess with people's customizations if we don't have to. There was an off-bug mail conversation with Shane about how to ensure that the pocket button would appear in the "seen" list of widgets once no longer a builtin widget, so we don't re-add it if the user removed it. I believe he's working up a patch for that. But this bug has now landed, so at least for Nightly it's too late for the Hello button unless we back it out / turn it off, and it might be hard to get adequate testing for aurora. That's sad. :-\

Comment 58

[Shane Caraveo (:mixedpuppy)]

I have a patch that adds obsolete button handling to CUI, I'll post it later today after I do a bit more cleanup.  Basically the problem here is:

- if the button was built-in (not created via createWidget API)
- prior to update to firefox with addon, user moves the button to palette
- update updates firefox to version with addon
- Neither the addon or CUI can determine that the button was previously placed into palette, falling back to placing it into defaultArea for the button widget.

The obsolete button patch handles this issue and allows createWidget to properly place the button in this scenario.  Once that lands (it could land separately) all you have to do is add the button id to the obsolete list, assuming the addon retains the same id as the built-in code did. If the hello button was not created using CUI.createWidget, it is certainly necessary for the hello addon in order to retain location in palette if that is what the user had done.

I was aware of the hello work and spoke with someone a while back (I think it was rhelmer) about system addons.

Part of my goal with the pocket addon is to make it work with a generic (e.g. 3rd party) build of firefox that doesn't include built-in addons.  A user could then choose to add it via AMO.  Thus my focus on making sure it is restartless, etc.

Comment 59

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Hello shipped as a Go Faster system add-on" as wording.

Comment 60

[Sylvestre Ledru [:Sylvestre]]

After discussing with Mark, we updated the wording to "Hello will be updated more frequently as part of the go faster strategy"