1272107 - [FlyWeb] Review Desktop changes for landing in mozilla-central

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Kannan Vijayan [:djvj]]

Attachment: flyweb-ui.patch

Sub-bug for reviewing Desktop changes for landing in m-c.

Comment 1

[u408661]

Firefox code, let's keep this under the Firefox component.

Comment 2

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1272107.patch

Mike, this is the patch from Bug 1265441 re-based to be applied to m-c instead of larch. I didn't want to automatically carry the R+ over without checking with you first. The only difference between this patch and the one from Bug 1265441 is that this patch doesn't contain any reverts of the old temp UI from larch.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8758410 [details] [diff] [review]
bug1272107.patch

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

It'll be good to see a try build, and do a measurement against a mozilla-central baseline via https://treeherder.mozilla.org/perf.html#/comparechooser to ensure that we don't cause any talos regressions.

::: browser/extensions/flyweb/skin/shared/flyweb.css
@@ +1,1 @@
> +#flyweb-panel {

Needs MPL license block

::: browser/extensions/flyweb/skin/windows/flyweb.css
@@ +1,1 @@
> +@import url("chrome://flyweb-shared/skin/flyweb.css");

Out of curiosity, is all of this redirection necessary, or can we just ask jar.mn to point all platforms at the shared flyweb.css?

Comment 4

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1272107.patch

Addressed review comments. Carrying R+ forward.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b65f7e689b5
FlyWeb desktop UI code.  Only enabled on nightly and aurora builds. r=mconley

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1f056b981b1f

You know how on Larch the OS X 10.10 debug e10s tests frequently hang on startup, like https://treeherder.mozilla.org/logviewer.html#?job_id=22703&repo=larch did? Not a coincidence, that's FlyWeb's fault, to the tune of three or four or six or nine per push while it was in mozilla-inbound.

Comment 7

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1272107.patch

Mike, this patch got backed out due to intermittent failures. Here's an updated diff for the add-on code that adds a pref observer so we do nothing unless "dom.flyweb.enabled" is set. This should prevent the issue on m-c.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8dab5e5d63a
FlyWeb desktop UI.  Only enabled on nightly and aurora. r=mconley

Comment 9

[Phil Ringnalda (:philor)]

Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/322d6c120f48 but this time with either a better log, or at least one I actually looked at: https://treeherder.mozilla.org/logviewer.html#?job_id=29619749&repo=mozilla-inbound#L13017 says that rather than just a hang, it's aborts and assertion failures.

Comment 10

[Justin D'Arcangelo [:justindarc]]

Seeing this in the full raw log:

{"thread": "ProcessReader", "level": "info", "pid": 2024, "source": "mochitest", "time": 1465259400949, "action": "log", "exc_info": false, "message": "1465259400943\taddons.xpi\tWARN\tException running bootstrap method shutdown on [e-mail]: TypeError: gDiscoveryManagerInstance is undefined (resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/features/[e-mail]!/bootstrap.js:202:5) JS Stack trace: uninit@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/features/[e-mail]:202:5 < shutdown@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/features/[e-mail]:41:3 < [e-mail]:4754:9 < this.XPIProvider.startup/<[e-mail]:2778:15 < [e-mail]:249:7 < [e-mail]:356:13 < [e-mail]:6126:36 < [e-mail]:4982:12 < [e-mail]:249:7"}

Not sure if this is causing the hangs on OS X 10.10 e10s though. Either way, updated patch on its way.

Comment 11

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1272107.patch

This patch adds a check at shutdown to avoid cleaning up the discovery manager if it never got created in the first place. This should avoid the JS exception I mentioned in Comment 10. Still don't know if this is what is causing the startup hangs on OS X 10.10 e10s.

Comment 12

[Kannan Vijayan [:djvj]]

Running this on try (OSX debug 10.10 M-e10s(1) only): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea2872c9dc65

Comment 13

[Kannan Vijayan [:djvj]]

That build failed due to existing build failure on inbound.  New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0438674018f7

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8760561 [details] [diff] [review]
bug1272107.patch

The uninit changes look fine to me. Assuming a good try run, r=me.

Comment 15

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1272107-experiment.patch

Here's a "NOOP" system add-on that should, in theory, do nothing. Let's see if it also breaks the OS X 10.10 DEBUG e10s build :-/

Comment 16

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1272107-experiment-locales.patch

Same "NOOP" add-on, this time with "locales".

Comment 17

[Kannan Vijayan [:djvj]]

Attachment: flyweb-ui.patch

Ok.  Turns out that removing locales stuff fixes the problem.  I have no idea why, there seems to be NOTHING off of about the locales usage.  That said, here is a new patch.

Comment 18

[Kannan Vijayan [:djvj]]

Just for refs, the try run for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e18578afb8d

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8761317 [details] [diff] [review]
flyweb-ui.patch

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

r=me assuming green on try.

Comment 20

[Axel Hecht [:Pike]]

Comment on attachment 8761317 [details] [diff] [review]
flyweb-ui.patch

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

Sorry, but I can't really help on the hang. That'd be really deep in e10s land, mixed maybe with all the unfortunate caching that the stringbundle service does.

Note, to get this code localized, we'll need to have an actual conversation, as basically each system addon does something different.

Please send an email to [e-mail], and describe your expectations?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

(In reply to Axel Hecht [:Pike] from comment #20)
> Comment on attachment 8761317 [details] [diff] [review]
> flyweb-ui.patch
> 
> Review of attachment 8761317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, but I can't really help on the hang. That'd be really deep in e10s
> land, mixed maybe with all the unfortunate caching that the stringbundle
> service does.
> 
> Note, to get this code localized, we'll need to have an actual conversation,
> as basically each system addon does something different.
> 
> Please send an email to [e-mail], and describe your
> expectations?

Hey Pike,

Sorry, we should have been more clear - what we're feedback?ing you for is to see if it's okay to land as-is without localization strings, with the hope of adding those strings later once we sort out this IPC issue.

Does that sound reasonable?

Comment 22

[Justin D'Arcangelo [:justindarc]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #21)

> Hey Pike,
> 
> Sorry, we should have been more clear - what we're feedback?ing you for is
> to see if it's okay to land as-is without localization strings, with the
> hope of adding those strings later once we sort out this IPC issue.
> 
> Does that sound reasonable?

Also, to be extra clear, this code is only to land in aurora/nightly and is pref'd off by default.

Comment 23

[Axel Hecht [:Pike]]

At least some of our localizers prefer to be able to test their strings in action, and it seems that we'll not have much of a target audience that'd we unleash by adding localized UI to this feature?

In this state, we shouldn't expose it to localizers at all for now, I think.

Comment 24

[Kannan Vijayan [:djvj]]

Cool, thanks for the feedback!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc18bf5abac
FlyWeb desktop UI code.  Third time is the charm. r=mconley feedback=Pike

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8dc18bf5abac