1183069 - Android M: Multiple fields and methods in android.provider.Browser removed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

A lot of fields and methods have been removed from android.provider.Browser. Our import code (AndroidImport) and LocalBrowserDB seem to depend on Browser.BOOKMARKS_URI and Browser.BookmarkColumns. These fields are now all gone with Android M Preview 2. Probably the whole content provider does not exist anymore.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

We either will have to copy these fields to our code base to build with the Android M SDK and continue to support this feature ("Import from Android") on older versions (see bug 1183559) or remove the feature completely.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1183069-legacy-browser.patch

This patch copies the removed fields from android.provider.Browser to a new class called LegacyBrowserProvider.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1183069-legacy-browser-v2.patch

Comment 4

[Richard Newman [:rnewman]]

I guess the API docs haven't been updated to show deprecations in 22 yet, mm?

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8642443 [details] [diff] [review]
1183069-legacy-browser-v2.patch

Review of attachment 8642443 [details] [diff] [review]:
-----------------------------------------------------------------

Well done for finding two bugs :)

::: mobile/android/base/db/LocalBrowserDB.java
@@ +41,5 @@
>  import org.mozilla.gecko.gfx.BitmapUtils;
>  import org.mozilla.gecko.mozglue.RobocopTarget;
>  import org.mozilla.gecko.sync.Utils;
>  import org.mozilla.gecko.util.GeckoJarReader;
> +import org.mozilla.gecko.util.LegacyBrowserProvider;

... and this becomes unnecessary.

@@ +889,5 @@
>      private void addBookmarkItem(ContentResolver cr, String title, String uri, long folderId) {
>          final long now = System.currentTimeMillis();
>          ContentValues values = new ContentValues();
>          if (title != null) {
> +            values.put(LegacyBrowserProvider.BookmarkColumns.TITLE, title);

This is wrong. We should be using Bookmarks.TITLE here!

@@ +992,5 @@
>      @Override
>      @RobocopTarget
>      public void updateBookmark(ContentResolver cr, int id, String uri, String title, String keyword) {
>          ContentValues values = new ContentValues();
> +        values.put(LegacyBrowserProvider.BookmarkColumns.TITLE, title);

Bookmarks.TITLE.

::: mobile/android/base/util/LegacyBrowserProvider.java
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +package org.mozilla.gecko.util;

Newline before the package, but can we just put this as a private static inner class in AndroidImport.java? It shouldn't be used anywhere else in the codebase.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #4)
> I guess the API docs haven't been updated to show deprecations in 22 yet, mm?

The web version hasn't been updated yet, but you can download the API docs for the preview version: http://developer.android.com/preview/download.html#docs

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1183069-legacy-browser-v3.patch

(In reply to Richard Newman [:rnewman] from comment #5)
> Well done for finding two bugs :)

Nice. I pay too much attention to the code. ;)

> Newline before the package, but can we just put this as a private static
> inner class in AndroidImport.java? It shouldn't be used anywhere else in the
> codebase.

Yeah, I updated the patch and moved the class inside AndroidImport.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Now the content provider is definitely going away on Android M+: https://android.googlesource.com/platform/packages/providers/BookmarkProvider/+/a87b7e2066601f09d195ea6371978fee84ec6c08

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8642926 [details] [diff] [review]
1183069-legacy-browser-v3.patch

Review of attachment 8642926 [details] [diff] [review]:
-----------------------------------------------------------------

I kinda want to conditionalize the whole importer on <M, so that if we do an M split we kill all of that code for free. But this patch is good :)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78b897906504

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/2e605777f948abaa33d6c02b68009f360ccd21f2
changeset:  2e605777f948abaa33d6c02b68009f360ccd21f2
user:       Sebastian Kaspari <[e-mail]>
date:       Mon Aug 03 17:04:27 2015 +0200
description:
Bug 1183069 - Copy removed fields from android.provider.Browser to LegacyBrowserProvider. r=rnewman

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e605777f948