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

RESOLVED FIXED in Firefox 45

Status

defect
P2
normal
Rank:
28
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks 1 bug)

unspecified
mozilla45
Points:
3
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox45 fixed, relnote-firefox 45+)

Details

(Whiteboard: [github])

Attachments

(9 attachments, 16 obsolete attachments)

85.35 KB, patch
standard8
: review+
Details | Diff | Splinter Review
43.70 KB, patch
standard8
: review+
Details | Diff | Splinter Review
47.90 KB, patch
standard8
: review+
Details | Diff | Splinter Review
49.12 KB, patch
standard8
: review+
Details | Diff | Splinter Review
97.38 KB, patch
standard8
: review+
Details | Diff | Splinter Review
8.00 KB, patch
standard8
: review+
Details | Diff | Splinter Review
13.66 KB, patch
standard8
: review+
Details | Diff | Splinter Review
17.24 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
1.80 KB, patch
mikedeboer
: review+
jmaher
: review+
Details | Diff | Splinter Review
Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Assignee: nobody → standard8
Rank: 28
Priority: -- → P2
Assignee

Updated

4 years ago
Depends on: 1209341
Assignee

Updated

4 years ago
Blocks: 1226706
Status: NEW → ASSIGNED
Assignee

Updated

4 years ago
Iteration: --- → 45.2 - Nov 30
Points: --- → 3
Assignee

Comment 1

4 years ago
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.
Attachment #8691968 - Flags: review?(mdeboer)
Assignee

Comment 2

4 years ago
This moves the prefs from firefox.js and the relevant css from the browser to the add-on directories.
Attachment #8691971 - Flags: review?(mdeboer)
Assignee

Comment 3

4 years ago
This moves browser-loop.js to become at this stage the skeleton bootstrap.js. Future parts will make it functional.
Attachment #8691974 - Flags: review?(mdeboer)
Assignee

Comment 4

4 years ago
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 loop@mozilla.org 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.
Attachment #8691976 - Flags: review?(mh+mozilla)
Attachment #8691976 - Flags: review?(mdeboer)
Assignee

Comment 5

4 years ago
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.
Attachment #8691977 - Flags: review?(mdeboer)
Assignee

Comment 6

4 years ago
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.
Attachment #8691978 - Flags: review?(mdeboer)
Assignee

Comment 7

4 years ago
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.
Attachment #8691979 - Flags: review?(mdeboer)
Flags: qe-verify+
QA Contact: bogdan.maris
Attachment #8691968 - Flags: review?(mdeboer) → review+
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.
Attachment #8691971 - Flags: review?(mdeboer) → review+
Assignee

Comment 10

4 years ago
Updated patcheset due to bitrot.

Also, fresh try push on a hopefully good tree: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be8d2c8c797
Attachment #8692659 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8691968 - Attachment is obsolete: true
Attachment #8691971 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8691974 - Attachment is obsolete: true
Attachment #8691974 - Flags: review?(mdeboer)
Assignee

Comment 13

4 years ago
Attachment #8692662 - Flags: review?(mh+mozilla)
Attachment #8692662 - Flags: review?(mdeboer)
Assignee

Updated

4 years ago
Attachment #8691976 - Attachment is obsolete: true
Attachment #8691976 - Flags: review?(mh+mozilla)
Attachment #8691976 - Flags: review?(mdeboer)
Assignee

Updated

4 years ago
Attachment #8691977 - Attachment is obsolete: true
Attachment #8691977 - Flags: review?(mdeboer)
Assignee

Updated

4 years ago
Attachment #8691978 - Attachment is obsolete: true
Attachment #8691978 - Flags: review?(mdeboer)
Assignee

Updated

4 years ago
Attachment #8691979 - Attachment is obsolete: true
Attachment #8691979 - Flags: review?(mdeboer)
(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 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/toolbar@2x.png      chrome://loop/skin/toolbar-win8@2x.png          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
Attachment #8692662 - Flags: review?(mh+mozilla) → review+
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['loop@test.mozilla.org'] += [ 'bootstrap.js' ]
DIST_FILES.features['loop@test.mozilla.org'] += ['install.rdf.in']

Then, in jar.mn:
[features/loop@test.mozilla.org] 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)
(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.
Assignee

Updated

4 years ago
Depends on: 1228542
Assignee

Comment 21

4 years ago
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
Attachment #8692901 - Flags: review?(mdeboer)
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...
Attachment #8692661 - Flags: review?(mdeboer) → review+
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?
Attachment #8692662 - Flags: review?(mdeboer) → review+
Attachment #8692663 - Flags: review?(mdeboer) → review+
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.
Attachment #8692664 - Flags: review?(mdeboer) → review+
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.'
Attachment #8692665 - Flags: review?(mdeboer) → review-
Assignee

Comment 26

4 years ago
(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 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.
Attachment #8692901 - Flags: review?(mdeboer) → review+
Assignee

Comment 28

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

Comment 29

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

Comment 30

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

Comment 31

4 years ago
Updated patches for review comments & bitrot.
Attachment #8692980 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8692659 - Attachment is obsolete: true
Attachment #8692660 - Attachment is obsolete: true
Attachment #8692661 - Attachment is obsolete: true
Attachment #8692662 - Attachment is obsolete: true
Attachment #8692663 - Attachment is obsolete: true
Attachment #8692664 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8692665 - Attachment is obsolete: true
Attachment #8692901 - Attachment is obsolete: true
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?
Attachment #8692986 - Flags: review?(mdeboer)
Assignee

Updated

4 years ago
Attachment #8692986 - Attachment is obsolete: true
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.
Attachment #8693022 - Flags: review?(mdeboer) → review+
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
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.
Assignee

Comment 46

4 years ago
(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\loop@mozilla.org.xpi' 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.
Assignee

Comment 47

4 years ago
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.
Attachment #8693244 - Flags: review?(mdeboer)
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\\loop@mozilla.org.xpi": {"mincount": 0, "maxcount": 100, "minbytes": 0, "maxbytes": 10000000},

Looks reasonable to me, but I'd like :jmaher to s-r this.
Attachment #8693244 - Flags: review?(mdeboer)
Attachment #8693244 - Flags: review?(jmaher)
Attachment #8693244 - Flags: review+
Assignee

Comment 50

4 years ago
Try pushes were green enough with the additional fix.
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?
Attachment #8693244 - Flags: review?(jmaher) → review+
Assignee

Comment 52

4 years ago
(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 55

4 years ago
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.
Assignee

Comment 56

4 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 57

4 years ago
(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. :-\
Flags: needinfo?(gijskruitbosch+bugs)
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.
Depends on: 1229123
Depends on: 1229195
Assignee

Updated

4 years ago
Depends on: 1229351
Assignee

Updated

4 years ago
Depends on: 1229352
Assignee

Updated

4 years ago
Depends on: 1229471
Depends on: 1229487
Assignee

Updated

4 years ago
Depends on: 1229492
Depends on: 1229731
Assignee

Updated

4 years ago
Depends on: 1228998
Assignee

Updated

4 years ago
Depends on: 1229933
Assignee

Updated

4 years ago
Depends on: 1228999
Assignee

Updated

4 years ago
Depends on: 1230147
Depends on: 1230280

Updated

4 years ago
Depends on: 1232207

Updated

4 years ago
Depends on: 1233701
Added to the release notes with "Hello shipped as a Go Faster system add-on" as wording.
After discussing with Mark, we updated the wording to "Hello will be updated more frequently as part of the go faster strategy"
Depends on: 1245277

Updated

3 years ago
Depends on: 1257947
You need to log in before you can comment on or make changes to this bug.