In production code, use nsIBrowserSearchService::curentEngine instead of defaultEngine

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: Siddhant085, Assigned: Siddhant085, Mentored)

Tracking

Trunk
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

9 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Safari/605.1.15
Assignee

Updated

9 months ago
Component: Untriaged → Search
Priority: -- → P3
Version: unspecified → Trunk
Assignee

Updated

9 months ago
Mentor: standard8
Assignee

Updated

9 months ago
Blocks: 1453264
Assignee: nobody → dpsrkp.sid
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [meta] (not test cases) Update the assignment and use of nsIBrowserSearchService::curentEngine (in favour of defaultEngine) → In production code, use of nsIBrowserSearchService::curentEngine instead of defaultEngine
Assignee

Comment 1

8 months ago
Was defaultEngine and currentEngine different in the past. I found this code

     // If the search bar is visible, use the current engine, otherwise, fall
     // back to the default engine.
     if (isElementVisible(this.searchBar))
       engine = Services.search.defaultEngine;
     else
       engine = Services.search.defaultEngine;
Flags: needinfo?(standard8)
Yes, they were different at some stage. So we should clean up that code and drop the if statement.
Flags: needinfo?(standard8)
Summary: In production code, use of nsIBrowserSearchService::curentEngine instead of defaultEngine → In production code, use nsIBrowserSearchService::curentEngine instead of defaultEngine
Assignee

Comment 3

8 months ago
nsIBrowserSearchService::currentEngine and nsIBrowserSearchService::defaultEngine are the same thing.
The use of defaultEngine makes more sense and thus we are phasing out the use of currentEngine and replace it with defaultEngine.
Attachment #9015495 - Attachment description: Use nsIBrowserSearchService::curentEngine instead of defaultEngine (in production code) → Use nsIBrowserSearchService::defaultEngine instead of currentEngine (in production code)

Comment 4

8 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c35bab726b03
Use nsIBrowserSearchService::defaultEngine instead of currentEngine (in production code) r=Standard8
Backed out for multiple browser-chrome failures e.g browser_extension_controlled.js

backout: https://hg.mozilla.org/integration/autoland/rev/dd418f0ef29cf4082b1379211b601fcf950836da

push with failures https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception,running,pending,runnable&selectedJob=204306963&revision=c35bab726b030312c53f48a654d5fcc2e1264ac2&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204306963&repo=autoland&lineNumber=2425

[task 2018-10-09T19:44:23.235Z] 19:44:23     INFO - Console message: [JavaScript Error: "TypeError: Services.search.deafultEngine is undefined; can't access its "name" property" {file: "chrome://browser/content/parent/ext-chrome-settings-overrides.js" line: 90}]
[task 2018-10-09T19:44:23.235Z] 19:44:23     INFO - Buffered messages finished
[task 2018-10-09T19:44:23.235Z] 19:44:23     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Test timed out - 
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Extension not fully unloaded at test shutdown - 
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - Stack trace:
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:112
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - chrome://mochikit/content/browser-test.js:nextTest:695
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1190
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - setTimeout handler*chrome://mochikit/content/browser-test.js:Tester_execTest:1152
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:986
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-10-09T19:44:23.236Z] 19:44:23     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Extension left running at test shutdown -
Flags: needinfo?(dpsrkp.sid)
Assignee

Comment 6

8 months ago
One problem causing browser_extension_controlled.js to fail is the incorrect spelling of default in ext-chrome-settings-overrides.js. I have fixed that and retried the test on my local system. It is still timing out. I am looking into the problem.
Flags: needinfo?(dpsrkp.sid)
Assignee

Comment 7

8 months ago
The test is timing out because of

INFO Failed to retrieve MOZ_UPLOAD_DIR env var

pushing the changes to run the tests on try server.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2920d79f0e6be184085c35ea19eec8272deb743e

Siddhant: can you post an updated patch which fixes the bitrot in PlacesSearchAutocompleteProvider.jsm ?
Flags: needinfo?(dpsrkp.sid)
Assignee

Comment 9

8 months ago
Hey Mark,

I went through the treeherder test summary and I see that uriloader/exthandler/tests/mochitest/browser_auto_close_window.js is failing and it also has a bug filed for it (https://bugzilla.mozilla.org/show_bug.cgi?id=1472761). I can't see any errors involving PlacesSearchAutocompleteProvider.jsm.
Flags: needinfo?(dpsrkp.sid) → needinfo?(standard8)
(In reply to Siddhant [:Siddhant085] from comment #9)
> I went through the treeherder test summary and I see that
> uriloader/exthandler/tests/mochitest/browser_auto_close_window.js is failing
> and it also has a bug filed for it
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1472761).

I believe that failure was actually a side-effect of something that was wrong on the tree at that time.

> I can't see any
> errors involving PlacesSearchAutocompleteProvider.jsm.

Did you pull the latest and rebase it? If so, if you could push again from the latest just to make sure that would be useful.

I'll push it to try one more time just to make sure there isn't an issue with that auto close test.
Flags: needinfo?(standard8)

Comment 11

8 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/408f7a1a2d0f
Use nsIBrowserSearchService::defaultEngine instead of currentEngine (in production code) r=Standard8

Comment 12

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/408f7a1a2d0f
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65

Comment 13

8 months ago
Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/4b365ad515f7379fbc4bb370f976418009aa518f
Backport Bug 1493483 - Use nsIBrowserSearchService::defaultEngine instead of currentEngine (in production code) r=Standard8

https://github.com/mozilla/activity-stream/commit/73f173931bd55c1e122454be563ed53b769041ff
Bug 1493483 - Fix unit tests to stub .defaultEngine
You need to log in before you can comment on or make changes to this bug.