Closed Bug 1221344 Opened 4 years ago Closed 2 years ago

Remove Search Activity and homescreen widget

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

All
Android
defect
Not set

Tracking

(relnote-firefox 58+, firefox58 verified)

VERIFIED FIXED
mozilla58
Tracking Status
relnote-firefox --- 58+
firefox58 --- verified

People

(Reporter: mcomella, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The changes from the patch in [1] do not appear when building via gradle but build fine via mach. Notably, the patch pretty much only changes the search activity manifest (as opposed to the actual code).

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1210242#c16
glandium: can you help me understand this?  To observe this, run

make -C objdir-droid/mobile/android/base AndroidManifest.xml

(or something else that generates $OBJDIR/mobile/android/base/AndroidManifest.xml).  Then

echo >> mobile/android/search/manifests/SearchAndroidManifest_activities.xml.in

and run

make -C objdir-droid/mobile/android/base AndroidManifest.xml

again.  When I run with `-d`, I don't see the $OBJDIR/mobile/android/base/.deps/AndroidManifest.xml.pp being considered, or its contents being included.

My guess is that mobile/android/base/Makefile.in is not including config.mk or rules.mk correctly (see https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#295).  Can you suggest a fix, or tell me what the includes should look like?
Flags: needinfo?(mh+mozilla)
The problem is the fact that that .pp file is only included is the make is invoked with the explicit target only (which in AndroidManifest.xml's case is actually the full path in the objdir) because of the "ifneq (,$(filter $(PP_TARGETS_TIERS) $(PP_TARGETS_ALL_RESULTS),$(MAKECMDGOALS)))" in rules.mk.
Flags: needinfo?(mh+mozilla)
snorp: sebastian: We have been shipping the Search Activity in Fennec for a Very Long Time -- it's default on (see http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/mobile/android/moz.configure#62-64).  It's basically untested, which means "almost certainly broken".  Do we think we have any users?  Even if we do, do we think the search activity is providing value?  Can we remove this crufty code and simplify our lives?

This might be a product call.  If it is, can one of you flag the relevant product manager?
Component: General → Build Config & IDE Support
Flags: needinfo?(snorp)
Flags: needinfo?(s.kaspari)
I'm for removing if we're not going to actively develop it.
Flags: needinfo?(snorp)
Flags: needinfo?(jcheng)
Flags: needinfo?(abovens)
I'm absolutely in favor of removing it. It's not maintained and kind of a distraction as long as the head count working on Fennec is low.

I still think search is an interesting "app topic" and would love to explore this as a separate NMX project. But I do not think keeping it alive in Fennec is worth it.

If we go ahead and remove it then we would need to redirect the search action from the widget (also such a candidate) to Fennec (Just open a new tab and put the cursor into the URL bar?).
Flags: needinfo?(s.kaspari)
(In reply to Nick Alexander :nalexander from comment #3)
> Do we think we have any users?

Very few: I ran a query here [1] though I had no one review my SQL and I'm no expert.

[1]: https://sql.telemetry.mozilla.org/queries/47032/source#127358
Comment on attachment 8920401 [details]
Bug 1221344 - Remove Fennec Search Activity.

https://reviewboard.mozilla.org/r/191364/#review196694

So we decided to remove the whole widget too?
Attachment #8920401 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #8)
> Comment on attachment 8920401 [details]
> Bug 1221344 - Remove Fennec Search Activity.
> 
> https://reviewboard.mozilla.org/r/191364/#review196694
> 
> So we decided to remove the whole widget too?

"Decided" might be strong.  I didn't differentiate the widget from the search activity for simplicity.  Since neither of the product people flagged have responded, I don't think the search activity is high priority.  I suppose the widget could, strictly speaking, be differentiated, but I'd rather cut it to the ground in order to simplify the codebase and minimize the feature set we need to maintain.
Comment on attachment 8920401 [details]
Bug 1221344 - Remove Fennec Search Activity.

https://reviewboard.mozilla.org/r/191364/#review196864
Attachment #8920401 - Flags: review?(snorp) → review+
Comment on attachment 8920401 [details]
Bug 1221344 - Remove Fennec Search Activity.

https://reviewboard.mozilla.org/r/191364/#review197602

I'm not sure why I was flagged to review this patch. Any info here?
(In reply to Jeff Beatty [:gueroJeff] from comment #11)
> Comment on attachment 8920401 [details]
> Bug 1221344 - Remove Fennec Search Activity.
> 
> https://reviewboard.mozilla.org/r/191364/#review197602
> 
> I'm not sure why I was flagged to review this patch. Any info here?

Sorry, should have provided more context.  I recall you own (or owned?) Fennec l10n, and this impacts a few l10n things.

It removes `search_strings.dtd` (mobile/android/base/locales/en-US/search_strings.dtd), and it's not clear to me that this removal will be correctly reflected back into the l10n-* repositories.  Is there anything more needed here?  Is there any further simplification of the l10n scripts/build system/etc that can be done when such a file is removed?
Flags: needinfo?(jbeatty)
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Thanks for the clarification. Delphine Lebédel manages Fennec l10n (added to CC). With the new cross-channel l10n repository (l10n-central), strings/files are no longer removed in order to allow for more easily shipping fixes across channels. Removing in m-c shouldn't impact l10n.
Flags: needinfo?(jbeatty)
(In reply to Jeff Beatty [:gueroJeff] from comment #13)
> Thanks for the clarification. Delphine Lebédel manages Fennec l10n (added to
> CC).

Duly noted!  Hi, Delphine!

 With the new cross-channel l10n repository (l10n-central),
> strings/files are no longer removed in order to allow for more easily
> shipping fixes across channels. Removing in m-c shouldn't impact l10n.

That's great news.  Thanks, Jeff!
Comment on attachment 8920401 [details]
Bug 1221344 - Remove Fennec Search Activity.

https://reviewboard.mozilla.org/r/191364/#review198120
Attachment #8920401 - Flags: review?(jbeatty) → review+
Summary: Search activity is not built via gradle & IJ under some conditions → Remove Search Activity and homescreen widget
abovens and I talked on Slack, and agreed to remove both the Search Activity and the homescreen widget.
Flags: needinfo?(jcheng)
Flags: needinfo?(abovens)
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf87dfd8ddb9
Remove Fennec Search Activity. r=gueroJeff,sebastian,snorp
Release Note Request (optional, but appreciated)

[Why is this notable]: removing a little-used but user visible feature.

[Affects Firefox for Android]: this *only* affects Firefox for Android.

[Suggested wording]: Homescreen widget and Search Activity removed.

[Links (documentation, blog post, etc)]: none.
relnote-firefox: --- → ?
https://hg.mozilla.org/mozilla-central/rev/bf87dfd8ddb9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(In reply to Francesco Lodolo [:flod] from comment #20)
> At least all the Wikipedia searchplugins, and Yahoo, have a "Search
> Activity" section (81 files in total)
> https://dxr.mozilla.org/mozilla-central/rev/
> 0d1e55d87931fe70ec1d007e886bcd58015ff770/mobile/locales/searchplugins/
> wikipedia.xml#17-21
> https://dxr.mozilla.org/mozilla-central/rev/
> 0d1e55d87931fe70ec1d007e886bcd58015ff770/mobile/locales/searchplugins/yahoo.
> xml#30-44
> 
> Do we need to get rid of those sections?

NI to make sure the question doesn't get lost.
Flags: needinfo?(nalexander)
(In reply to Francesco Lodolo [:flod] from comment #20)
> At least all the Wikipedia searchplugins, and Yahoo, have a "Search
> Activity" section (81 files in total)
> https://dxr.mozilla.org/mozilla-central/rev/
> 0d1e55d87931fe70ec1d007e886bcd58015ff770/mobile/locales/searchplugins/
> wikipedia.xml#17-21
> https://dxr.mozilla.org/mozilla-central/rev/
> 0d1e55d87931fe70ec1d007e886bcd58015ff770/mobile/locales/searchplugins/yahoo.
> xml#30-44
> 
> Do we need to get rid of those sections?

I think we do, so I've filed Bug 1412330.  (I don't think these extra bits will be broken, just unused right now.)  I see even more comments and documentation to remove too.  Thanks, flod!
Flags: needinfo?(nalexander)
I just filed bugs 1406876 and 1407877 and got told Search Activity is going to die. Boo, a tragedy! But I understand.

Can you at least tweak Fennec so that a long press of the Home button brings up Fennec with the cursor in the Awesomebar and the keyboard invoked. Like Samsung Internet Browser does?
(In reply to Mark from comment #24)
> I just filed bugs 1406876 and 1407877 and got told Search Activity is going
> to die. Boo, a tragedy! But I understand.
> 
> Can you at least tweak Fennec so that a long press of the Home button brings
> up Fennec with the cursor in the Awesomebar and the keyboard invoked. Like
> Samsung Internet Browser does?

This is a cool idea.  File the ticket, CC me, and we'll triage it?  I didn't know such things were possible.
Depends on: 1413024
Blocks: 1413739
Verified as fixed on Nightly 58 (2017-11-01). The search widget is no longer available and it's removed from the home screen after a version update.
Devices:
Samsung S7 Edge (Android 7.0)
Huawei Honor 5X (Android 5.1.1)
Prestigio Grace X5 (Android 4.4.2)

note: On some devices, as Huawei Honor 5X (Android 5.1.1) & Prestigio Grace X5 (Android 4.4.2), in the widget's place, there's a message: "problem loading widget". Users should be notified somehow that the widget is going away.
(In reply to Oana Horvath from comment #26)
> Verified as fixed on Nightly 58 (2017-11-01). The search widget is no longer
> available and it's removed from the home screen after a version update.
> Devices:
> Samsung S7 Edge (Android 7.0)
> Huawei Honor 5X (Android 5.1.1)
> Prestigio Grace X5 (Android 4.4.2)
> 
> note: On some devices, as Huawei Honor 5X (Android 5.1.1) & Prestigio Grace
> X5 (Android 4.4.2), in the widget's place, there's a message: "problem
> loading widget". Users should be notified somehow that the widget is going
> away.

Hmm.  Oana, after a reboot of the device, does the message still appear?

The issue is that there can be no way to avoid the message!  These versions of Android aren't clearing their caches in a reasonable way, and I don't think we can work around that.  Consider a user who doesn't open Fennec for some time, and only interacts with the widget.  We could update the widget to say that the widget was going away... but then the system error message would appear after the updated widget was removed.

In any case, I don't want to spend time trying to finesse this issue unless it's seen as a major problem blocking 58.
Flags: needinfo?(oana.horvath)
The message can be removed from the screen just as the widget, by dragging and removing it. That's not a blocking issue.
The problem it's obviously not on Fennec, but I was thinking that it would be better to let the users know in advance that the widget won't work anymore. Release notes is one way, but maybe something more easy to see? Just a suggestion.
Flags: needinfo?(oana.horvath)
This is in the beta 58 release notes as "Removed the Firefox Search widget from home screen".
Flags: needinfo?(gchang)
Flags: needinfo?(gchang)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 58 → mozilla58
You need to log in before you can comment on or make changes to this bug.