Build with Android support library 26

ASSIGNED
Assigned to

Status

()

Firefox for Android
Build Config & IDE Support
ASSIGNED
11 months ago
3 days ago

People

(Reporter: JanH, Assigned: petru, NeedInfo)

Tracking

(Depends on: 2 bugs, Blocks: 7 bugs)

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: --do_not_change--[priority:high])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

11 months ago
Blocks: 1266683
(Reporter)

Updated

11 months ago
Blocks: 1385726
(Reporter)

Updated

11 months ago
No longer blocks: 1266683
Blocks: 1408691
Whiteboard: geckoview
Nick, I think this should be easier now with the Gradle build. It looks to me like newer support libs are only distributed through the Maven repo, and not via the SDK manager as before. If we don't have to care about mozbuild we can just specify that in the gradle files like a civilized person, right? Maybe this is a reason to nuke the mozbuild system?
Flags: needinfo?(nalexander)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #1)
> Nick, I think this should be easier now with the Gradle build. It looks to
> me like newer support libs are only distributed through the Maven repo, and
> not via the SDK manager as before. If we don't have to care about mozbuild
> we can just specify that in the gradle files like a civilized person, right?

Yes.

> Maybe this is a reason to nuke the mozbuild system?

It could be.  If we really need to bump the support libs, then yes -- I think we should finish https://bugzilla.mozilla.org/show_bug.cgi?id=1414415 rather than learning how to fetch an AAR from Google's Maven repo at configure time.
Flags: needinfo?(nalexander)
Blocks: 1450450
Depends on: 1438716
Blocks: 1459579
Working around the compile errors caused by upgrading the support library version used to 26.1.0 is straightforward enough, but unfortunately after the upgrade I'm encountering RejectedExecutionExceptions during Robocop-4 in automation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c005aa7d93f447014be211fecf1a0c4fdc545bc&selectedJob=177191551

> >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
> java.util.concurrent.RejectedExecutionException: Task android.support.v4.content.ModernAsyncTask$3@420dbc38 rejected from java.util.concurrent.ThreadPoolExecutor@41a952a0[Running, pool size = 128, active threads = 127, queued tasks = 10, completed tasks = 28]
> 	at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(ThreadPoolExecutor.java:1979)
> 	at java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:786)
> 	at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1307)
> 	at android.support.v4.content.ModernAsyncTask.executeOnExecutor(ModernAsyncTask.java:449)
> 	at android.support.v4.content.AsyncTaskLoader.executePendingTask(AsyncTaskLoader.java:225)
> 	at android.support.v4.content.AsyncTaskLoader.onForceLoad(AsyncTaskLoader.java:153)
> 	at android.support.v4.content.Loader.forceLoad(Loader.java:329)
> 	at org.mozilla.gecko.home.BrowserSearch$SuggestionAsyncLoader.onStartLoading(BrowserSearch.java:1037)
> 	at android.support.v4.content.Loader.startLoading(Loader.java:272)
> 	at android.support.v4.app.LoaderManagerImpl$LoaderInfo.start(LoaderManager.java:270)
> 	at android.support.v4.app.LoaderManagerImpl.installLoader(LoaderManager.java:562)
> 	at android.support.v4.app.LoaderManagerImpl.createAndInstallLoader(LoaderManager.java:549)
> 	at android.support.v4.app.LoaderManagerImpl.restartLoader(LoaderManager.java:700)
> 	at org.mozilla.gecko.home.BrowserSearch.filterSuggestions(BrowserSearch.java:707)
> 	at org.mozilla.gecko.home.BrowserSearch.filter(BrowserSearch.java:1002)
> 	at org.mozilla.gecko.BrowserApp.filterEditingMode(BrowserApp.java:2695)
> 	at org.mozilla.gecko.BrowserApp$20.onFilter(BrowserApp.java:1329)
> 	at org.mozilla.gecko.toolbar.ToolbarEditText$TextChangeListener.afterTextChanged(ToolbarEditText.java:569)
> 	at android.widget.TextView.sendAfterTextChanged(TextView.java:7334)
> 	at android.widget.TextView$ChangeWatcher.afterTextChanged(TextView.java:9087)
> 	at android.text.SpannableStringBuilder.sendAfterTextChanged(SpannableStringBuilder.java:970)
> 	at android.text.SpannableStringBuilder.replace(SpannableStringBuilder.java:497)
> 	at android.text.SpannableStringBuilder.replace(SpannableStringBuilder.java:435)
> 	at android.text.SpannableStringBuilder.replace(SpannableStringBuilder.java:30)
> 	at android.text.method.QwertyKeyListener.onKeyDown(QwertyKeyListener.java:223)
> 	at android.text.method.TextKeyListener.onKeyDown(TextKeyListener.java:136)
> 	at android.widget.TextView.doKeyDown(TextView.java:5464)
> 	at android.widget.TextView.onKeyDown(TextView.java:5283)
> 	at android.view.KeyEvent.dispatch(KeyEvent.java:2623)
> 	at android.view.View.dispatchKeyEvent(View.java:7343)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchKeyEvent(PhoneWindow.java:1933)
> 	at com.android.internal.policy.impl.PhoneWindow.superDispatchKeyEvent(PhoneWindow.java:1408)
> 	at android.app.Activity.dispatchKeyEvent(Activity.java:2384)
> 	at android.support.v7.app.AppCompatActivity.dispatchKeyEvent(AppCompatActivity.java:534)
> 	at android.support.v7.view.WindowCallbackWrapper.dispatchKeyEvent(WindowCallbackWrapper.java:58)
> 	at android.support.v7.app.AppCompatDelegateImplBase$AppCompatWindowCallbackBase.dispatchKeyEvent(AppCompatDelegateImplBase.java
See Also: → bug 759338
Comment hidden (mozreview-request)
^^
See above for my current patch, which builds, but suffers from the RejectedExecutionException problem.
Another problem is that at least for local developer builds [1], the total method count is getting dangerously close to the limit.

v23.4.0:
> Total methods in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 63601 (97.05% used)
> Total fields in app-local-withGeckoBinaries-minApi21-photon-debug.apk:  36428 (55.59% used)
> Total classes in app-local-withGeckoBinaries-minApi21-photon-debug.apk:  8055 (12.29% used)

v26.1.0:
> Total methods in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 65131 (99.38% used)
> Total fields in app-local-withGeckoBinaries-minApi21-photon-debug.apk:  41111 (62.73% used)
> Total classes in app-local-withGeckoBinaries-minApi21-photon-debug.apk:  8389 (12.80% used)


[1] Automation seems to have some more breathing space left - picking the currently latest mozilla-central build, I'm seeing "Total methods in app-official-withoutGeckoBinaries-noMinApi-photon-debug.apk: 55006 (83.93% used)". Maybe we're using a more aggressive Proguard configuration there?
Additionally, it seems that the new support library is causing us to leak cursors, because I'm seeing lots of messages like "CursorWrapperInner: Cursor finalized without prior close()" in the logs, which aren't appearing in logs from our current normal automation builds with the previous support library.

Locally, I'm additionally seeing this as well:
> 05-13 05:20:04.898 4768-4777/org.mozilla.fennec_jan E/StrictMode: A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
>     java.lang.Throwable: Explicit termination method 'close' not called
>         at dalvik.system.CloseGuard.open(CloseGuard.java:184)
>         at android.content.ContentResolver$CursorWrapperInner.<init>(ContentResolver.java:1903)
>         at android.content.ContentResolver.query(ContentResolver.java:438)
>         at android.content.ContentResolver.query(ContentResolver.java:357)
>         at org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:684)
>         at org.mozilla.gecko.db.LocalBrowserDB.filter(LocalBrowserDB.java:744)
>         at org.mozilla.gecko.home.SearchLoader$SearchCursorLoader.loadCursor(SearchLoader.java:102)
>         at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:61)
>         at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:42)
>         at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:302)
>         at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:57)
>         at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:45)
>         at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:138)
>         at java.util.concurrent.FutureTask.run(FutureTask.java:234)
>         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1080)
>         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:573)
>         at java.lang.Thread.run(Thread.java:841)
> 05-13 05:20:04.898 4768-4777/org.mozilla.fennec_jan W/CursorWrapperInner: Cursor finalized without prior close()
> 05-13 05:20:05.098 4768-4777/org.mozilla.fennec_jan E/StrictMode: Finalizing a Cursor that has not been deactivated or closed. database = /sdcard/tests/profile/browser.db, table = combined, query = SELECT _id, url, title, bookmark_id, history_id FROM combined WHERE ((url LIKE ? OR title LIKE ?)) GROUP BY url ORDER BY (CASE WHEN bookmark_id > -1 THEN 100 ELSE 0 END) + remoteVisitCount * MAX(1, 100 * 110 / ((1526213958855000 - remoteDateLastVisited) / 86400000000 * (1526213958855000 - remoteDateLastVisited) / 86400000000 + 110)) + (localVisitCount + 2) * (localVisitCount + 2) * MAX(2, 100 * 225 / ((1526213958855000 - localDateLastVisited) / 86400000000 * (1526213958855000 - localDateLastVisited) / 86400000000 + 225)) DESC LIMIT 100
>     android.database.sqlite.DatabaseObjectNotClosedException: Application did not close the cursor or database object that was opened here
>         at android.database.sqlite.SQLiteCursor.<init>(SQLiteCursor.java:98)
>         at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:50)
>         at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1314)
>         at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:400)
>         at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:333)
>         at org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:1458)
>         at android.content.ContentProvider.query(ContentProvider.java:744)
>         at android.content.ContentProvider$Transport.query(ContentProvider.java:199)
>         at android.content.ContentResolver.query(ContentResolver.java:414)
>         at android.content.ContentResolver.query(ContentResolver.java:357)
>         at org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:684)
>         at org.mozilla.gecko.db.LocalBrowserDB.filter(LocalBrowserDB.java:744)
>         at org.mozilla.gecko.home.SearchLoader$SearchCursorLoader.loadCursor(SearchLoader.java:102)
>         at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:61)
>         at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:42)
>         at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:302)
>         at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:57)
>         at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:45)
>         at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:138)
>         at java.util.concurrent.FutureTask.run(FutureTask.java:234)
>         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1080)
>         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:573)
>         at java.lang.Thread.run(Thread.java:841)
The RejectedExecutionProblem and the leaked cursors can be reproduced by running the emulator (I simply used the default Android 4.3 one provided by ./mach android-emulator), clicking the URL bar and then mashing the keyboard/holding down a key. Optionally you can add something like https://hg.mozilla.org/try/rev/97023473565ad87852e264729a090c2b9782c2e2#l2.12 to more easily monitor the current number of threads.

It seems like the problem was already introduced in v24.0.0 of the support library:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=681d35feca2558505e0764ed77bb6bfed195a845

If I didn't overlook anything, the only relevant changes to the Support Library between 23.4.0 and 24.0.0 regarding loaders, tasks, etc. were
https://github.com/aosp-mirror/platform_frameworks_support/commit/0bab42508734d3f701a1fbb1d24c96fabff96163
https://github.com/aosp-mirror/platform_frameworks_support/commit/b3f99a77c492ae252bac148540bcd6e8fa7c18b1
https://github.com/aosp-mirror/platform_frameworks_support/commit/47ab4c24dac8eff1adbe4cc216cbf9c6318993a3

of which I think the last one to be the most likely culprit.

Updated

a month ago
Whiteboard: geckoview → --do_not_change--[priority:high]
Duplicate of this bug: 1333704
(Assignee)

Updated

a month ago
Blocks: 1407046
(Assignee)

Comment 10

a month ago
Beside the errors that Jan talked about I see the app crashing when receiving the sign in response with the following stacktrace - https://pastebin.mozilla.org/9086010

I think this is because the current version of play-services and gcm we are using is too low (8.4.0) and incompatible with support library 26 and so it also needs to be updated.
(Assignee)

Comment 11

a month ago
For updating play-services I filed bug 1463376
(Assignee)

Comment 12

a month ago
I have also tried to upgrade the support library used to v. 26 and got the same results as Jan.

- RejectedExecutionException is the biggest problem which started occuring a lot in tests.
I went with Jan's suggestion (thanks!) to force it by spamming the address bar with new text and saw that it is possible to get that crash in the old app also (using previous support library v. 23.4.0) because in the current implementation, for any new input in the address bar a new SearchLoader will be created (an AsyncTaskLoader which will start processing the query on another new thread). Spamming the address bar could create so many AsyncTaskLoaders that the ThreadPoolExecutor is saturated. But indeed it seems that when using support library 26 one can saturate the Executor faster and many Robocop tests are failing because of that [1].
I saw some other complaints about AsyncTaskLoader and RejectedExecutionException [2], [3] but no clear culprit or solution.
Saw that in v. 27 the LoaderManager has been rewritten to better suit ViewModel and LiveData arch components [4] but I've tried using that and saw that same behavior as with v. 26 when spamming the address bar.
The issue seems to stem from repeated calls to Loader().forceLoad() [5] which creates new AsyncTasks (with new threads) and right now to me it seems that getting past it would involve a major refactoring on how we use the loaders for suggestions. 

- The Cursors leaks seems to be tied with the upgrade to support library 26 and I wasn't able to find a clear source for this leaks, like places in which the Cursors should be closed but aren't.
My reasoning for what could be the cause for the leaks would again refer to the multitude of threads possibly created which I think could bring some concurrency issues with reading from the same database.
What I've tried doing to overcome this is to call LoaderManager.destroyLoader() before the existing call to LoaderManager.restartLoader(..) [6] to stop and remove the previous loader. After doing this, all would work as intended, with no crashes.

- Another problem after the update would be (as Jan said) the total number of method references. Building from Android Studio was not possible for me as I would get DexIndexOverflowException. Building with mach was possible though.?

[1] "RejectedExecutionException" and a high number for "current thread count" appear a lot in https://taskcluster-artifacts.net/NoM6AWddRKazXq7IkaMyTQ/0/public/logs/live_backing.log
[2] https://stackoverflow.com/questions/17138095
[3] https://stackoverflow.com/questions/31757881
[4] https://medium.com/google-developers/loaders-in-support-library-27-1-0-b1a1f0fee638
[5] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#1037
[6] https://hg.mozilla.org/try/rev/ae0d59d9f0c3276dcad29899baf6aa613da6ab12#l7.12
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

a month ago
I see some tests still failing - https://treeherder.mozilla.org/#/jobs?repo=try&revision=2542cffd07a0fdabc5bb54a9f5fd35083984af01. Please hold off on the review.
Assignee: nobody → petru.lingurar
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8973769 - Attachment is obsolete: true
Attachment #8980589 - Flags: review?(sdaswani) → review?(michael.l.comella)
Nick, would you be an appropriate reviewer for these patches? If so, please take my r?.

fwiw, it looks like we still might be facing some intermittents based on the comments and a Slack conversation with Petru.
Flags: needinfo?(nalexander)
Comment on attachment 8980587 [details]
Bug 1385464 - Start using support library v. 26;

Hi Jan, can you review this patch and some others I am tagging for r? from you as Mike is busy with pressing deadline work and I don't have r+ access.
Flags: needinfo?(nalexander)
Attachment #8980587 - Flags: review?(sdaswani) → review?(jh+bugzilla)
Attachment #8980588 - Flags: review?(sdaswani) → review?(jh+bugzilla)
Attachment #8980589 - Flags: review?(michael.l.comella) → review?(jh+bugzilla)
(Reporter)

Comment 22

22 days ago
mozreview-review
Comment on attachment 8980588 [details]
Bug 1385464 - Resolve missing resources and api changes;

https://reviewboard.mozilla.org/r/246740/#review254916

Looks fine, except it seems that something went wrong with the bookmark editing layouts?

::: mobile/android/app/src/main/res/layout/bookmark_add_folder.xml:21
(Diff revision 2)
>          android:id="@+id/toolbar"
>          android:layout_width="match_parent"
>          android:layout_height="?actionBarSize"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
> -        app:navigationIcon="@drawable/abc_ic_clear_mtrl_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"

This doesn't seem right...

::: mobile/android/app/src/main/res/layout/bookmark_add_folder.xml:23
(Diff revision 2)
>          android:layout_height="?actionBarSize"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
> -        app:navigationIcon="@drawable/abc_ic_clear_mtrl_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"
>          app:subtitleTextColor="@android:color/white"
> -        app:title="@string/bookmark_add_folder"
> +        app:title="@string/appbar_scrolling_view_behavior"

... and this even less.

::: mobile/android/app/src/main/res/layout/bookmark_add_folder.xml:47
(Diff revision 2)
>              <EditText
>                  android:id="@+id/edit_folder_name"
>                  android:layout_width="match_parent"
>                  android:layout_height="wrap_content"
>                  android:ellipsize="end"
> -                android:hint="@string/bookmark_edit_name"
> +                android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_add_folder.xml:70
(Diff revision 2)
>                  android:drawableEnd="@drawable/arrow"
>                  android:drawableRight="@drawable/arrow"
>                  android:ellipsize="end"
>                  android:focusable="false"
>                  android:focusableInTouchMode="false"
> -                android:hint="@string/bookmark_parent_folder"
> +                android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:21
(Diff revision 2)
>          android:layout_width="match_parent"
>          android:layout_height="56dp"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
>          android:minHeight="?actionBarSize"
> -        app:navigationIcon="@drawable/abc_ic_clear_mtrl_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:50
(Diff revision 2)
>                      android:id="@+id/edit_bookmark_name"
>                      android:layout_width="match_parent"
>                      android:layout_height="wrap_content"
>                      android:ellipsize="end"
>                      android:gravity="start"
> -                    android:hint="@string/bookmark_edit_name"
> +                    android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:72
(Diff revision 2)
>                      android:id="@+id/edit_bookmark_location"
>                      android:layout_width="match_parent"
>                      android:layout_height="wrap_content"
>                      android:ellipsize="end"
>                      android:gravity="start"
> -                    android:hint="@string/bookmark_edit_location"
> +                    android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:98
(Diff revision 2)
>                      android:drawablePadding="8dp"
>                      android:ellipsize="end"
>                      android:focusable="false"
>                      android:focusableInTouchMode="false"
>                      android:gravity="start"
> -                    android:hint="@string/bookmark_parent_folder"
> +                    android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:119
(Diff revision 2)
>                      android:id="@+id/edit_bookmark_keyword"
>                      android:layout_width="match_parent"
>                      android:layout_height="wrap_content"
>                      android:ellipsize="end"
>                      android:gravity="start"
> -                    android:hint="@string/bookmark_edit_keyword"
> +                    android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_folder_select.xml:20
(Diff revision 2)
>          android:id="@+id/toolbar"
>          android:layout_width="match_parent"
>          android:layout_height="?actionBarSize"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
> -        app:navigationIcon="@drawable/abc_ic_ab_back_mtrl_am_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_folder_select.xml:22
(Diff revision 2)
>          android:layout_height="?actionBarSize"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
> -        app:navigationIcon="@drawable/abc_ic_ab_back_mtrl_am_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"
>          app:subtitleTextColor="@android:color/white"
> -        app:title="@string/bookmark_select_folder" />
> +        app:title="@string/appbar_scrolling_view_behavior" />

Ditto.
Attachment #8980588 - Flags: review?(jh+bugzilla)
(Reporter)

Comment 23

22 days ago
mozreview-review
Comment on attachment 8980588 [details]
Bug 1385464 - Resolve missing resources and api changes;

https://reviewboard.mozilla.org/r/246740/#review254920

Also note that lint appears to throw up some additional errors. Two of these are because of the bookmark edit layout issues, but I think the others here need to be addressed, too:
https://taskcluster-artifacts.net/Ba9FGDtARt-rLSSVfFMQtA/0/public/android/lint/lint-results-officialWithoutGeckoBinariesNoMinApiPhotonDebug.html#RestrictedApi
(Assignee)

Comment 24

21 days ago
Thanks Jan! Will look into them.
Attachment #8980589 - Flags: review?(jh+bugzilla) → review?(gkruglov)
(Reporter)

Comment 25

21 days ago
mozreview-review
Comment on attachment 8980589 [details]
Bug 1385464 - Resolve obscure leaks and crashes after the upgrade;

https://reviewboard.mozilla.org/r/246742/#review255186

I'm not entirely happy about simply disabling that code - even if we weren't actively testing it, getting it exercised during the other tests was still useful as a very basic check. Short of a bigger rewrite of the suggestion system there might be no better alternative, though.
I also hope that this problem is indeed specific to the emulator and not actually relevant during real-life usage where we've hopefully got a better ratio of device speed to typing speed.
Still, I'd like someone else to take a second look at this as well if possible.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 29

17 days ago
mozreview-review
Comment on attachment 8980588 [details]
Bug 1385464 - Resolve missing resources and api changes;

https://reviewboard.mozilla.org/r/246740/#review256636

Looks fine, thanks.
Attachment #8980588 - Flags: review?(jh+bugzilla) → review+
(In reply to Petru-Mugurel Lingurar[:petru] from comment #11)
> For updating play-services I filed bug 1463376

Please don't forget to set the proper dependencies, otherwise there's a risk of overlooking this.
Depends on: 1463376
(Reporter)

Comment 31

14 days ago
mozreview-review
Comment on attachment 8980587 [details]
Bug 1385464 - Start using support library v. 26;

https://reviewboard.mozilla.org/r/246738/#review256890

Who ever okays the cursor loader changes can give the final r+ here as well. If Grisha isn't around, maybe ask nalexander or mcomella?

The one recommendation I'd make is that after all patches have been successfully reviewed, you should squash them together before landing (but please preserve the full commit messages for the individual parts), since you'd end up with broken commits that cannot be built otherwise.
Attachment #8980587 - Flags: review?(jh+bugzilla)
(Assignee)

Updated

14 days ago
Attachment #8980589 - Flags: review?(gkruglov) → review?(nalexander)

Comment 32

12 days ago
mozreview-review
Comment on attachment 8980587 [details]
Bug 1385464 - Start using support library v. 26;

https://reviewboard.mozilla.org/r/246738/#review257056

This impacts GeckoView (and geckoview_example), but I think this is fine.  I'll verify with the GV team.

::: build.gradle:54
(Diff revision 3)
>              url "file://${gradle.mozconfig.topsrcdir}/mobile/android/gradle/m2repo"
>          }
>      }
>  
>      ext.kotlin_version = '1.2.41'
> -    ext.support_library_version = '23.4.0'
> +    ext.support_library_version = '26.1.0'

It's amazing that this is all that is required to bump the support library version these days!
Attachment #8980587 - Flags: review+

Comment 33

12 days ago
mozreview-review
Comment on attachment 8980588 [details]
Bug 1385464 - Resolve missing resources and api changes;

https://reviewboard.mozilla.org/r/246740/#review257058

This all looks fine.
Attachment #8980588 - Flags: review+
Depends on: 1468487

Comment 34

12 days ago
mozreview-review
Comment on attachment 8980589 [details]
Bug 1385464 - Resolve obscure leaks and crashes after the upgrade;

https://reviewboard.mozilla.org/r/246742/#review257060

I hate changing tests in automation, but fine -- whatever gets us through the day.

Thanks for pushing this forward, Petru and JanH!

::: mobile/android/base/java/org/mozilla/gecko/home/SearchLoader.java:75
(Diff revision 3)
>      }
>  
>      public static void restart(LoaderManager manager, int loaderId,
>                                 LoaderCallbacks<Cursor> callbacks, String searchTerm,
>                                 EnumSet<FilterFlags> flags) {
> +        manager.destroyLoader(loaderId);

Oh, this is awful, but based on my reading, this whole API is deprecated and other people have had to do similar things to make loaders work at all :/
Attachment #8980589 - Flags: review?(nalexander) → review+
I see lint failures in https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7bcf91575c831e905930a74a460b093ec60423a&selectedJob=181632112 -- those need to be fixed before landing (but no re-review needed).

dbolter: this patch will bump the support library version for GeckoView built in automation.  I think that's the right thing to do but could you stamp yourself or redirect to somebody on your team who can OK this bump?
Flags: needinfo?(petru.lingurar)
Flags: needinfo?(dbolter)
(Assignee)

Comment 36

11 days ago
(In reply to Nick Alexander :nalexander from comment #35)
> I see lint failures in
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b7bcf91575c831e905930a74a460b093ec60423a&selectedJob=1
> 81632112 -- those need to be fixed before landing (but no re-review needed).
> 
> dbolter: this patch will bump the support library version for GeckoView
> built in automation.  I think that's the right thing to do but could you
> stamp yourself or redirect to somebody on your team who can OK this bump?

This Lint issues were reported by :JanH in comment 23 and resolved in latest version of the patches.

I triggered a try build with the same syntax to confirm the issues gone and now I see some strange L10n tests errors - https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf8baddf524530058f342735ca795ff64c7c3990 
Don't know what's up with these and I don't think they were caused by this patch..
Flags: needinfo?(petru.lingurar)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 days ago
Attachment #8980588 - Attachment is obsolete: true
(Assignee)

Updated

11 days ago
Attachment #8980589 - Attachment is obsolete: true
(Assignee)

Comment 38

11 days ago
Squashed the 3 patches together as Jan suggested, waiting for the 63 nightly and the OK from dbolter to merge.
(In reply to Petru-Mugurel Lingurar[:petru] from comment #38)
> Squashed the 3 patches together as Jan suggested, waiting for the 63 nightly
> and the OK from dbolter to merge.

Looks good.  The l10n errors are definitely TC scheduling errors; let's not worry about them.
Over to James. (see comment 35)
Flags: needinfo?(dbolter) → needinfo?(snorp)

Updated

6 days ago
Status: NEW → ASSIGNED

Updated

3 days ago
Blocks: 1463376
No longer depends on: 1463376
You need to log in before you can comment on or make changes to this bug.