1109388 - Delay reading prefs until prefs page is opened

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Looks like the prefs are read on device connection.

We should wait until the device prefs is first opened before reading them, since it's a lot of data to pull in.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Jen, could you take a look at this?  I can't always reproduce the issue, so maybe I am misunderstanding something.

Comment 2

[Jen Fong-Adwent [:ednapiranha]]

(In reply to J. Ryan Stinnett [:jryans] from comment #1)
> Jen, could you take a look at this?  I can't always reproduce the issue, so
> maybe I am misunderstanding something.

Sure, did you want this to be included in bug 1022797 (since prefs are being refactored and settings will likely need the same changes) or should I do a separate fix after?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #2)
> (In reply to J. Ryan Stinnett [:jryans] from comment #1)
> > Jen, could you take a look at this?  I can't always reproduce the issue, so
> > maybe I am misunderstanding something.
> 
> Sure, did you want this to be included in bug 1022797 (since prefs are being
> refactored and settings will likely need the same changes) or should I do a
> separate fix after?

Bug 1022797 is already a large patch, so let's make the changes here.

As far whether it lands before or after bug 1022797, that's up to you, but I think we want to delay load for both prefs and settings.

Comment 4

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

This ended up making tests pass for all three - just wanted to make sure it was on the right track.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8554574 [details] [diff] [review]
Bug1109388.patch

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

Great, I think this works well to cover the case where we don't load preferences, etc. ever until you use the panel.

There's also another case of:

1. Connect to device A
2. Use device preferences
3. Close preferences
4. Connect to device B

With your change, we'll still keep loading preferences from device B, since the preference frame is no long lazy.

Feel free to file that as a follow up though, as this is already a great improvement!

I think we could handle this additional case using the page visibility API (visibilitychange event) to detect which frame is displayed, and skip requests until the frame becomes visible again.

Comment 6

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

So I found a way to reapply lazysrc again and added a test in device preferences to check if it was reapplied but not sure if this was the wrong place to apply it (in webide.js) or if you have better suggestions?

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8555979 [details] [diff] [review]
Bug1109388.patch

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

While you did reset the "lazysrc" attribute, the frame still has its content loaded (which is good, since you're manipulating the frame we just selected).

Since the frame's page is still loaded, this patch does not stop data requests on a new connection.

Turn on "devtools.debugger.log" and watch the requests in the terminal when you make a second runtime connection.

I don't think this "lazysrc" stuff will help solve this new problem of later devices.  Additionally, we can probably solve all problems with "visibilitychange" and drop "lazysrc" all together.

Comment 8

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

Comment 9

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

Comment 10

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

Comment 11

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

Comment 12

[Jen Fong-Adwent [:ednapiranha]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94229e38cf38

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/4817fb8acffd

Comment 14

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1843963&repo=fx-team

Comment 15

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

removed nextTick() that was causing test errors

Comment 16

[Jen Fong-Adwent [:ednapiranha]]

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

Comment 17

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/02502cd30c5b

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/ba8a1b53973c

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out again for the same test failure as in comment #14 -> https://treeherder.mozilla.org/logviewer.html#?job_id=1857370&repo=fx-team

Comment 20

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #19)
> sorry had to back this out again for the same test failure as in comment #14
> -> https://treeherder.mozilla.org/logviewer.html#?job_id=1857370&repo=fx-team

maybe we also need to trigger m-oth tests more times in a try run ?

Comment 21

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

A couple of additions and a few testing observations:

1. Since settings doesn't have a save as Int/Bool/String and we're returning and checking by typeof, all Ints become String which causes one test to fail because it is expecting an input[type="number"]. I put in a check on config-view.js to see if it can be parsed into an Int and assume to return as such. Probably not ideal, but doesn't seem to cause much harm either way (unless we have a scenario where a custom value should be a string but is written in an integer format). I am open to suggestions on this where we could file another bug to handle this particular side effect.

2. I've put in a break in deviceinfo tests where it loops through the information to find a matching "appid" and "geolocation" so that it doesn't try to run through everything once it has found a match.

3. For the testing repetitions, I've run the following (which may be different for you):

3.1 deviceinfo repeat 3 loops of testing x 3 = 3 PASS, 0 FAIL; repeat 20 loops of testing x 3 = 2 PASS, 1 FAIL

3.2 device_preferences repeat 3 loops of testing x 3 = 3 PASS, 0 FAIL; repeat 20 loops of testing x 3 = 3 PASS, 0 FAIL

3.3 device_settings repeat 3 loops of testing x 3 = 3 PASS, 0 FAIL; repeat 20 loops of testing x 3 = 3 PASS, 0 FAIL

So I am not sure what is going on exactly, and why it usually passes most of the time but not always. Going to test on my Ubuntu laptop but will submit this r? now.

Comment 22

[Jen Fong-Adwent [:ednapiranha]]

Just as a note, tested on a laptop with Ubuntu installed and all tests passed with 20 loops.

Comment 23

[Jen Fong-Adwent [:ednapiranha]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=698ecfe461ac

Comment 24

[Carsten Book [:Tomcat]]

Hi Jen, i retriggered now the m-oth tests for you on this try run to check if its still failing

Comment 25

[Carsten Book [:Tomcat]]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #22)
> Just as a note, tested on a laptop with Ubuntu installed and all tests
> passed with 20 loops.

seems this is a OS X problem (see retriggered try runs)

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8558678 [details] [diff] [review]
Bug1109388.patch

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

Overall, the tweaks you've made make sense to me.

The intermittent issue is caused by the same thing I mentioned on IRC: we need to have the tests wait for the page UI to build as well, not just for actor communication to finish.

So, for all of these pages (prefs, settings, perms, runtime info) we should change their test promises to also include the UI work as well.

Comment 27

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

A few notes:

- Only did the changes to the promise on deviceinfo in runtime.js and permissionstable.js

- When tests fail, I noticed that permIframe was null, causing everything to not be found (even the #close link)

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8561446 [details] [diff] [review]
Bug1109388.patch

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

I believe we should capture the promise that builds the page immediately on entering |buildUI|.

Right now, it's unclear if the "getDescriptionFieldsPromise" will be set by the time the test yields on it.  If the promise is not set before yielding on it, the test will just proceed without waiting.

Comment 29

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

Comment 30

[Jen Fong-Adwent [:ednapiranha]]

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

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8561661 [details] [diff] [review]
Bug1109388.patch

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

r+, just some style tweaks to resolve.  Happy to look again if you're unsure about some parts.

::: browser/devtools/webide/content/devicepreferences.js
@@ +74,5 @@
>      configView.kind = "Pref";
>      configView.includeTypeName = true;
>  
> +    getAllPrefs = AppManager.preferenceFront.getAllPrefs().then(json =>
> +      configView.generateDisplay(json));

See devicesettings.js on style.

::: browser/devtools/webide/content/devicesettings.js
@@ +74,5 @@
>      configView.kind = "Setting";
>      configView.includeTypeName = false;
>  
> +    getAllSettings = AppManager.settingsFront.getAllSettings().then(json =>
> +      configView.generateDisplay(json));

When putting a => expression on the next line, it's more typical to convert to block format:

getAllSettings = AppManager.settingsFront.getAllSettings().then(json => {
  configView.generateDisplay(json);
});

or else wrap the whole .then() call:

getAllSettings = AppManager.settingsFront.getAllSettings()
                           .then(json => configView.generateDisplay(json));

::: browser/devtools/webide/content/permissionstable.js
@@ +30,5 @@
>      BuildUI();
>    }
>  }
>  
> +let table;

Don't create globals unnecessarily.

I'd say just re-query for this value inside |generateFields|.

@@ +31,5 @@
>    }
>  }
>  
> +let table;
> +function generateFields(json) {

This function does its work synchronously, so a promise is not needed here.

@@ +66,3 @@
>  let getRawPermissionsTablePromise; // Used by tests
>  function BuildUI() {
> +  table = document.querySelector("table");

Revert

@@ +73,5 @@
>  
>    if (AppManager.connection &&
>        AppManager.connection.status == Connection.Status.CONNECTED &&
>        AppManager.deviceFront) {
> +    getRawPermissionsTablePromise = AppManager.deviceFront.getRawPermissionsTable().then(json =>

See settings for style.

::: browser/devtools/webide/content/runtimedetails.js
@@ +41,5 @@
>      CheckLockState();
>    }
>  }
>  
> +let table;

Don't create globals unnecessarily.

I'd say just re-query for this value inside |generateFields|.

@@ +42,5 @@
>    }
>  }
>  
> +let table;
> +function generateFields(json) {

This function does its work synchronously, so a promise is not needed here.

@@ +62,3 @@
>  let getDescriptionPromise; // Used by tests
>  function BuildUI() {
> +  table = document.querySelector("table");

Revert

@@ +65,5 @@
>    table.innerHTML = "";
>    if (AppManager.connection &&
>        AppManager.connection.status == Connection.Status.CONNECTED &&
>        AppManager.deviceFront) {
> +    getDescriptionPromise = AppManager.deviceFront.getDescription().then(json =>

See settings for style.

::: browser/devtools/webide/modules/config-view.js
@@ +84,5 @@
>        }
>      }
>    },
>  
> +  generateDisplay: function(json) {

This function does its work synchronously, so a promise is not needed here.

::: browser/devtools/webide/test/test_device_permissions.html
@@ +72,5 @@
> +
> +          yield closeWebIDE(win);
> +
> +          SimpleTest.finish();
> +

Nit: remove these two blank lines

::: browser/devtools/webide/test/test_device_runtime.html
@@ +27,5 @@
> +          }
> +
> +          let win = yield openWebIDE();
> +
> +          let infoIframe = win.document.querySelector("#deck-panel-runtimedetails");

Call this |detailsIframe|... It never really made sense before!

@@ +31,5 @@
> +          let infoIframe = win.document.querySelector("#deck-panel-runtimedetails");
> +
> +          yield connectToLocalRuntime(win);
> +
> +          let info = win.document.querySelector("#cmd_showRuntimeDetails");

|details|

@@ +74,5 @@
> +
> +          yield closeWebIDE(win);
> +
> +          SimpleTest.finish();
> +

Nit: remove these two blank lines

Comment 32

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/6a701bd8f339

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out per an IRC request from the patch author.
https://hg.mozilla.org/integration/fx-team/rev/38e326cfea46

Comment 35

[Ryan VanderMeulen [:RyanVM]]

This was hitting intermittent mochitest failures too:
https://treeherder.mozilla.org/logviewer.html#?job_id=1953201&repo=fx-team

Comment 36

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

Comment 37

[Jen Fong-Adwent [:ednapiranha]]

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

Comment 38

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8562809 [details] [diff] [review]
Bug1109388.patch

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

Just a small tweak left, no need to re-review.

However, please *make sure* to re-run try for the updated version, and then re-trigger the mochitest-other suite lots of times (maybe 10 - 20) before requesting checkin.

::: browser/devtools/webide/test/head.js
@@ +138,5 @@
>  }
>  
> +function lazyIframeIsLoaded(iframe) {
> +  let deferred = promise.defer();
> +  iframe.addEventListener("load", function onLoad() {

You should remove the event listener as the first line of the handler function.

@@ +140,5 @@
> +function lazyIframeIsLoaded(iframe) {
> +  let deferred = promise.defer();
> +  iframe.addEventListener("load", function onLoad() {
> +    deferred.resolve();
> +  }, true);

I believe you don't need the true option and can remove it, but test to be sure.

Either way, the call to |removeEventListener| should match |addEventListener| by either passing true or no value for this.

Comment 39

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1109388.patch

Comment 40

[Jen Fong-Adwent [:ednapiranha]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6305091d6851

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/e0f02f382772

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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