Closed Bug 1529969 Opened 1 year ago Closed 7 months ago

Only request info about the current engine to the parent

Categories

(Firefox :: New Tab Page, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 73
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox73 --- fixed

People

(Reporter: Felipe, Assigned: squib)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file)

When the handoff-to-awesomebar feature is enabled, about:home doesn't need to get the data about all search engines, only the default one.

ContentSearch/ContentSearchChild doesn't have an API for that at moment, only getState which gets information about all engines, serializes it to send to the child, only to be unused. So we need to add a new getCurrentEngine API (or add a paramenter to getState), and make Activity Stream inform the ContentSearchUIController to see if the handoff is enabled.

(B) Even with the handoff disabled, it would be nice if initially it only got info about the current engine, and later it requested the info about all engines whenever it needs to build the search one-off buttons UI.

Spending time on (B) would depend on how likely the handoff is to become the official UI or if the previous UI will remain around for longer

Severity: normal → enhancement
Iteration: --- → 68.1 - Mar 18 - 31
Priority: -- → P2
Whiteboard: [fxperf] → [fxperf:p2]
Iteration: 68.1 - Mar 18 - 31 → ---
Component: Activity Streams: Newtab → New Tab Page
Assignee: nobody → jporter+bmo
Status: NEW → ASSIGNED

While looking into this, I stumbled on the following comment:

            This dummy and hidden input below is so we can load ContentSearchUIController.
            Why? It sets --newtab-search-icon for us and it isn't trivial to port over.

It looks like that's the only bit that we actually need from ContentSearch when the handoff is enabled. With that in mind, would it make sense to do the (non-trivial) porting work to use something like --newtab-search-icon in the handoff version of the search box? Comment 0 mentions that fixing this bug could also help the non-handoff case, but isn't sure if that's a priority going forward. While I could work on improving the ContentSearch implementation so this applies to both versions, that might be a waste of effort if the non-handoff case is going away.

Any thoughts here?

Flags: needinfo?(felipc)

Mardak, are you able to help out re: comment #1 ?

Flags: needinfo?(edilee)

There's also bug 1521758 as about:privatebrowsing ends up loading chrome://browser/content/contentSearchUI.js I believe just to access var(--newtab-search-icon) so the optimization here could help that too.

Looking at the related meta bug 1501747, handoff was to be turned on by default in release 66, but that hasn't happened… (and is only enabled in nightly from bug 1516045).

mconnor, do you know who is deciding if/when handoff will be the only way to search from about:newtab/privatebrowsing related pages? This would allow not supporting the in-page search suggestions interface when implementing this optimization.

Blocks: 1501747
Flags: needinfo?(mconnor)
Flags: needinfo?(felipc)
Flags: needinfo?(edilee)
See Also: → 1521758

Ultimately it'll be me making the call, but we have a lot of work to do before we get there. It's unlikely we'll make this change before Q4 at the earliest.

Flags: needinfo?(mconnor)

This patch adds a new ContentSearchHandoffUI class to be used when search
handoff is enabled in the newtab page, avoiding the much more complex
ContentSearchUI. This also reduces the amount of unnecessary information being
sent across processes.

See also the Activity Stream pull request: https://github.com/mozilla/activity-stream/pull/5343

I think the patch in attachment 9100033 [details] is good to go now. Could you take another look at it to make sure I addressed all the issues you pointed out? Thanks!

Flags: needinfo?(edilee)
Pushed by jporter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6636e1cd0c41
Only request info about the current engine to the parent; r=adw,Mardak

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=281603826&resultStatus=testfailed%2Cbusted%2Cexception&revision=6636e1cd0c4163a145fbd92a2223b417cf840fa1&searchStr=linux%2Cx64%2Copt%2Cnode%2Ctests%2Csource-test-node-newtab-unit-tests%2Cnode%28newtab%29

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281603826&repo=autoland&lineNumber=286

Backout link: https://hg.mozilla.org/integration/autoland/rev/23e791edc97f1f3d065509fba3db227fb7acda68

[task 2019-12-17T19:48:04.824Z] npm ERR! A complete log of this run can be found in:
[task 2019-12-17T19:48:04.824Z] npm ERR! /builds/worker/.npm/_logs/2019-12-17T19_48_04_800Z-debug.log
[task 2019-12-17T19:48:04.832Z] TEST-UNEXPECTED-FAIL karma | activity-stream:<Search>:Search Hand-off should hand-off search on paste: ReferenceError: ContentSearchHandoffUIController is not defined (http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:123999)
[task 2019-12-17T19:48:04.832Z] invokeGuardedCallbackDev@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:4881:16
[task 2019-12-17T19:48:04.832Z] invokeGuardedCallback@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:4938:31
[task 2019-12-17T19:48:04.832Z] commitRoot@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:23630:28
[task 2019-12-17T19:48:04.832Z] completeRoot/<@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25100:15
[task 2019-12-17T19:48:04.832Z] unstable_runWithPriority@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:26234:12
[task 2019-12-17T19:48:04.832Z] completeRoot@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25099:13
[task 2019-12-17T19:48:04.832Z] performWorkOnRoot@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25028:21
[task 2019-12-17T19:48:04.832Z] requestWork@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:24772:24
[task 2019-12-17T19:48:04.832Z] scheduleWork@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:24593:16
[task 2019-12-17T19:48:04.832Z] scheduleRootUpdate@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25254:15
[task 2019-12-17T19:48:04.832Z] updateContainerAtExpirationTime@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25282:10
[task 2019-12-17T19:48:04.832Z] updateContainer@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25339:10
[task 2019-12-17T19:48:04.832Z] ReactRoot.prototype.render@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25635:18
[task 2019-12-17T19:48:04.832Z] legacyRenderSubtreeIntoContainer/<@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25772:14
[task 2019-12-17T19:48:04.832Z] unbatchedUpdates@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25136:14
[task 2019-12-17T19:48:04.832Z] legacyRenderSubtreeIntoContainer@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25768:21
[task 2019-12-17T19:48:04.832Z] render@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25837:12
[task 2019-12-17T19:48:04.832Z] render/<@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:1289:116
[task 2019-12-17T19:48:04.832Z] wrapAct/<@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:1195:17
[task 2019-12-17T19:48:04.832Z] batchedUpdates$1@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:25121:12
[task 2019-12-17T19:48:04.832Z] act@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:33231:27
[task 2019-12-17T19:48:04.832Z] wrapAct@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:1194:26
[task 2019-12-17T19:48:04.832Z] render@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:1275:22
[task 2019-12-17T19:48:04.832Z] ReactWrapper@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:35496:16
[task 2019-12-17T19:48:04.832Z] mount@http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:76278:10
[task 2019-12-17T19:48:04.832Z] @http://localhost:9876/base/test/unit/unit-entry.js?5701958a7419071f433695e96fc7bda939d336e8:129171:75

Flags: needinfo?(jporter+bmo)

Oh, hmm. I wasn't aware those tests existed. I must not have run them when I pushed to try. I'll update the patch now...

Flags: needinfo?(jporter+bmo)

(In reply to Cosmin Sabou [:CosminS] from comment #9)

[task 2019-12-17T19:48:04.832Z] TEST-UNEXPECTED-FAIL karma | activity-stream:<Search>:Search Hand-off should hand-off search on paste: ReferenceError: ContentSearchHandoffUIController is not defined

This is the main failure that should be fixed by updating unit-entry. Try chooser should allow selecting this test as "source-test-node-newtab-unit-tests"

https://phabricator.services.mozilla.com/D48776#1753956

Ok, I re-pushed this as it was just a one-line fix (excluding comments) to a test file. Here's hoping this is all green now!

Pushed by jporter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fa1be74b9b0
Only request info about the current engine to the parent; r=adw,Mardak
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73
Regressions: 1605280
Flags: needinfo?(edilee)
You need to log in before you can comment on or make changes to this bug.