Only setup autocompletion if the autocomplete config is not set

RESOLVED FIXED in Firefox 67

Status

defect
P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 67

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

This will prevent the source editor from loading the autocomplete.js module if the tool doesn't have the autocomplete config set, and should improve loading performance for the debugger and console. The autocompletion behaviour will be preserved for the style editor and scratchpad.

Comment on attachment 9040232 [details] [diff] [review]
1524068.patch [1.0]

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

::: devtools/client/sourceeditor/editor.js
@@ +1253,5 @@
>     * it just because it is preffed on (it still needs to be requested by the
>     * editor), but we do want to always disable it if it is preffed off.
>     */
>    setupAutoCompletion: function() {
> +    if (!this.config.autocomplete && !this.initializeAutoCompletion) {

What's the case where this.config.autocomplete is false but this.initializeAutoCompletion will exist? IOW: could this check be just `if (this.initializeAutoCompletion) return;

(In reply to Brian Grinstead [:bgrins] from comment #2)

Comment on attachment 9040232 [details] [diff] [review]
1524068.patch [1.0]

Review of attachment 9040232 [details] [diff] [review]:

::: devtools/client/sourceeditor/editor.js
@@ +1253,5 @@

* it just because it is preffed on (it still needs to be requested by the
* editor), but we do want to always disable it if it is preffed off.
*/

setupAutoCompletion: function() {

  • if (!this.config.autocomplete && !this.initializeAutoCompletion) {

What's the case where this.config.autocomplete is false but
this.initializeAutoCompletion will exist? IOW: could this check be just `if
(this.initializeAutoCompletion) return;

For the case where this.config.autocomplete is false and this.initializeAutoCompletion exists, we would go to calling this.destroyAutoCompletion(), which would be the intent here if this.config.autocomplete is set to false. So, I believe this is checked and the correct behaviour is performed.

We can't simplify to 'if (this.initializeAutoCompletion) return', since there might be a switch to the autocomplete as a result of a pref or setOption.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #3)

For the case where this.config.autocomplete is false and this.initializeAutoCompletion exists, we would go to calling this.destroyAutoCompletion(), which would be the intent here if this.config.autocomplete is set to false. So, I believe this is checked and the correct behaviour is performed.

We can't simplify to 'if (this.initializeAutoCompletion) return', since there might be a switch to the autocomplete as a result of a pref or setOption.

Sorry, I miswrote here. I meant: if (!this.config.autocomplete) return.

(In reply to Brian Grinstead [:bgrins] from comment #4)

(In reply to Gabriel [:gl] (ΦωΦ) from comment #3)

For the case where this.config.autocomplete is false and this.initializeAutoCompletion exists, we would go to calling this.destroyAutoCompletion(), which would be the intent here if this.config.autocomplete is set to false. So, I believe this is checked and the correct behaviour is performed.

We can't simplify to 'if (this.initializeAutoCompletion) return', since there might be a switch to the autocomplete as a result of a pref or setOption.

Sorry, I miswrote here. I meant: if (!this.config.autocomplete) return.

This was also considered. However, if the autocompletion is already initialized, we would still need to destroy it since this could called from setOption.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #5)

(In reply to Brian Grinstead [:bgrins] from comment #4)

(In reply to Gabriel [:gl] (ΦωΦ) from comment #3)

For the case where this.config.autocomplete is false and this.initializeAutoCompletion exists, we would go to calling this.destroyAutoCompletion(), which would be the intent here if this.config.autocomplete is set to false. So, I believe this is checked and the correct behaviour is performed.

We can't simplify to 'if (this.initializeAutoCompletion) return', since there might be a switch to the autocomplete as a result of a pref or setOption.

Sorry, I miswrote here. I meant: if (!this.config.autocomplete) return.

This was also considered. However, if the autocompletion is already initialized, we would still need to destroy it since this could called from setOption.

Ah OK, I see this function handles destruction too.

Attachment #9040232 - Flags: review?(bgrinstead) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e90ad6323fd0
Only setup autocompletion if the autocomplete config is not set. r=bgrins

Backed out changeset e90ad6323fd0 (bug 1524068) for devtools failures at devtools/client/styleeditor/test/browser_styleeditor_autocomplete-disabled.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d43adb2b26ee297c8585412c0f22521ea5d7ecb

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e90ad6323fd0f59eacc301aa24cbb607db0329c9

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=225131084&repo=mozilla-inbound&lineNumber=4600

task 2019-01-31T03:00:18.265Z] 03:00:18 INFO - TEST-START | devtools/client/styleeditor/test/browser_styleeditor_autocomplete-disabled.js
[task 2019-01-31T03:00:20.271Z] 03:00:20 INFO - TEST-INFO | started process screentopng
[task 2019-01-31T03:00:20.809Z] 03:00:20 INFO - TEST-INFO | screentopng: exit 0
[task 2019-01-31T03:00:20.811Z] 03:00:20 INFO - Buffered messages logged at 03:00:18
[task 2019-01-31T03:00:20.813Z] 03:00:20 INFO - Entering test bound
[task 2019-01-31T03:00:20.815Z] 03:00:20 INFO - Adding a new tab with URL: 'http://example.com/browser/devtools/client/styleeditor/test/autocomplete.html'
[task 2019-01-31T03:00:20.817Z] 03:00:20 INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/devtools/client/styleeditor/test/autocomplete.html" line: 0}]
[task 2019-01-31T03:00:20.818Z] 03:00:20 INFO - URL 'http://example.com/browser/devtools/client/styleeditor/test/autocomplete.html' loading complete
[task 2019-01-31T03:00:20.821Z] 03:00:20 INFO - Buffered messages logged at 03:00:19
[task 2019-01-31T03:00:20.827Z] 03:00:20 INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/NetUtil.jsm" line: 259}]
[task 2019-01-31T03:00:20.829Z] 03:00:20 INFO - Console message: [JavaScript Warning: "XUL box for h3 element contained an inline #text child, forcing all its children to be wrapped in a block." {file: "chrome://devtools/content/styleeditor/index.xul" line: 0}]
[task 2019-01-31T03:00:20.831Z] 03:00:20 INFO - Console message: [JavaScript Warning: "XUL box for h3 element contained an inline #text child, forcing all its children to be wrapped in a block." {file: "chrome://devtools/content/styleeditor/index.xul" line: 0}]
[task 2019-01-31T03:00:20.833Z] 03:00:20 INFO - Buffered messages logged at 03:00:20
[task 2019-01-31T03:00:20.835Z] 03:00:20 INFO - TEST-PASS | devtools/client/styleeditor/test/browser_styleeditor_autocomplete-disabled.js | Autocompletion option does not exist -
[task 2019-01-31T03:00:20.837Z] 03:00:20 INFO - Buffered messages finished
[task 2019-01-31T03:00:20.842Z] 03:00:20 INFO - TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_autocomplete-disabled.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/styleeditor/test/browser_styleeditor_autocomplete-disabled.js:20 - TypeError: editor.sourceEditor.getAutocompletionPopup is not a function
[task 2019-01-31T03:00:20.843Z] 03:00:20 INFO - Stack trace:
[task 2019-01-31T03:00:20.843Z] 03:00:20 INFO - @chrome://mochitests/content/browser/devtools/client/styleeditor/test/browser_styleeditor_autocomplete-disabled.js:20:7
[task 2019-01-31T03:00:20.844Z] 03:00:20 INFO - AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1106:34
[task 2019-01-31T03:00:20.845Z] 03:00:20 INFO - async
Tester_execTest@chrome://mochikit/content/browser-test.js:1097:16
[task 2019-01-31T03:00:20.847Z] 03:00:20 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:995:9
[task 2019-01-31T03:00:20.848Z] 03:00:20 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
[task 2019-01-31T03:00:20.849Z] 03:00:20 INFO - Leaving test bound
[task 2019-01-31T03:00:20.850Z] 03:00:20 INFO - Removing tab.
[task 2019-01-31T03:00:20.851Z] 03:00:20 INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2019-01-31T03:00:20.853Z] 03:00:20 INFO - Got event: 'TabClose' on [object XULElement].
[task 2019-01-31T03:00:20.854Z] 03:00:20 INFO - Tab removed and finished closing

Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/550ac85a1218
Only setup autocompletion if the autocomplete config is not set. r=bgrins

Clearing needinfo since I have resolved the issue and pushed.

Flags: needinfo?(gl)
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.