1529969 - Only request info about the current engine to the parent

Status: RESOLVED FIXED

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

Description

[:Felipe Gomes (needinfo for replies!)]

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

Comment 1

[Jim Porter (:squib)]

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?

Comment 2

[:Gijs (he/him)]

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

Comment 3

[Ed Lee :Mardak]

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.

Comment 4

[Mike Connor [:mconnor]]

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.

Comment 5

[Jim Porter (:squib)]

Attachment: Bug 1529969 - Only request info about the current engine to the parent; r?adw

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.

Comment 6

[Jim Porter (:squib)]

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

Comment 7

[Jim Porter (:squib)]

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!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6636e1cd0c41
Only request info about the current engine to the parent; r=adw,Mardak

Comment 9

[Cosmin Sabou [:CosminS]]

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

Comment 10

[Jim Porter (:squib)]

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...

Comment 11

[Ed Lee :Mardak]

(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

Comment 12

[Jim Porter (:squib)]

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!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fa1be74b9b0
Only request info about the current engine to the parent; r=adw,Mardak

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1fa1be74b9b0