Closed Bug 1493483 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Search, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: Siddhant085, Assigned: Siddhant085, Mentored)

References

Details

Attachments

(1 file)

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
Component: Untriaged → Search
Priority: -- → P3
Version: unspecified → Trunk
Mentor: standard8
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
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
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)
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)
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)
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)
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)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/408f7a1a2d0f
Use nsIBrowserSearchService::defaultEngine instead of currentEngine (in production code) r=Standard8
https://hg.mozilla.org/mozilla-central/rev/408f7a1a2d0f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
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.