Closed Bug 1109388 Opened 5 years ago Closed 5 years ago

Delay reading prefs until prefs page is opened

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: jryans, Assigned: jfong)

Details

Attachments

(1 file, 12 obsolete files)

26.99 KB, patch
jfong
: review+
Details | Diff | Splinter Review
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.
Jen, could you take a look at this?  I can't always reproduce the issue, so maybe I am misunderstanding something.
Flags: needinfo?(jfong)
(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?
Flags: needinfo?(jfong)
Assignee: nobody → jfong
(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.
Attached patch Bug1109388.patch (obsolete) — Splinter Review
This ended up making tests pass for all three - just wanted to make sure it was on the right track.
Attachment #8554574 - Flags: feedback?(jryans)
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.
Attachment #8554574 - Flags: feedback?(jryans) → review+
Attached patch Bug1109388.patch (obsolete) — Splinter Review
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?
Attachment #8554574 - Attachment is obsolete: true
Attachment #8555979 - Flags: feedback?(jryans)
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.
Attachment #8555979 - Flags: feedback?(jryans) → feedback-
Attached patch Bug1109388.patch (obsolete) — Splinter Review
Attachment #8555979 - Attachment is obsolete: true
Attachment #8556592 - Flags: review+
Attached patch Bug1109388.patch (obsolete) — Splinter Review
Attachment #8556592 - Attachment is obsolete: true
Attachment #8556597 - Flags: review+
Attached patch Bug1109388.patch (obsolete) — Splinter Review
Attachment #8556597 - Attachment is obsolete: true
Attachment #8556613 - Flags: review+
Attached patch Bug1109388.patch (obsolete) — Splinter Review
Attachment #8556613 - Attachment is obsolete: true
Attachment #8556614 - Flags: review+
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1843963&repo=fx-team
Flags: needinfo?(jfong)
Whiteboard: [fixed-in-fx-team]
Attached patch Bug1109388.patch (obsolete) — Splinter Review
removed nextTick() that was causing test errors
Attachment #8556614 - Attachment is obsolete: true
Flags: needinfo?(jfong)
Attachment #8557166 - Flags: review+
Keywords: checkin-needed
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
Flags: needinfo?(jfong)
(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 ?
Attached patch Bug1109388.patch (obsolete) — Splinter Review
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.
Attachment #8557166 - Attachment is obsolete: true
Flags: needinfo?(jfong)
Attachment #8558678 - Flags: review?(jryans)
Just as a note, tested on a laptop with Ubuntu installed and all tests passed with 20 loops.
Hi Jen, i retriggered now the m-oth tests for you on this try run to check if its still failing
(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)
Flags: needinfo?(jfong)
Whiteboard: [fixed-in-fx-team]
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.
Attachment #8558678 - Flags: review?(jryans)
Attached patch Bug1109388.patch (obsolete) — Splinter Review
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)
Attachment #8558678 - Attachment is obsolete: true
Flags: needinfo?(jfong)
Attachment #8561446 - Flags: feedback?(jryans)
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.
Attachment #8561446 - Flags: feedback?(jryans)
Attached patch Bug1109388.patch (obsolete) — Splinter Review
Attachment #8561446 - Attachment is obsolete: true
Attachment #8561661 - Flags: review?(jryans)
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
Attachment #8561661 - Flags: review?(jryans) → review+
Attached patch Bug1109388.patch (obsolete) — Splinter Review
Attachment #8561661 - Attachment is obsolete: true
Attachment #8561703 - Flags: review+
Keywords: checkin-needed
Backed out per an IRC request from the patch author.
https://hg.mozilla.org/integration/fx-team/rev/38e326cfea46
Whiteboard: [fixed-in-fx-team]
Attached patch Bug1109388.patch (obsolete) — Splinter Review
Attachment #8561703 - Attachment is obsolete: true
Attachment #8562809 - Flags: review?(jryans)
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.
Attachment #8562809 - Flags: review?(jryans) → review+
Attached patch Bug1109388.patchSplinter Review
Attachment #8562809 - Attachment is obsolete: true
Attachment #8562834 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0f02f382772
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.