1286203 - Read icons from PartnerBookmarksProvider

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android partner distribution, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

We support reading and displaying bookmarks from the partner bookmarks provider, but we do not load the icons yet.

I've a patch to import the icons into our database but it would be nicer to actually read them from the  partner provider directly, like we do with the bookmarks.

http://androidxref.com/6.0.0_r1/xref/packages/providers/PartnerBookmarksProvider/src/com/android/providers/partnerbookmarks/PartnerBookmarksProvider.java

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1286203 - Introduce proxy for partner bookmarks and load icons.

This patch does multiple things:
* Introduce a proxy content provider for the partner bookmarks provider: This allows
  us to hide the id transformation in the proxy and will later be used to filter
  "deleted" bookmarks from the actual content provider (bug 1286794).
* Modifies LoadFaviconTask to support loading icons from a content provider.
* Introduces a new flag to not download icons from guessed default favicon URLs.

Review commit: https://reviewboard.mozilla.org/r/64220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64220/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8770893 [details]
Bug 1286203 - Introduce proxy for partner bookmarks and load icons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64220/diff/1-2/

Comment 3

[Andrzej Hunt :ahunt]

Comment on attachment 8770893 [details]
Bug 1286203 - Introduce proxy for partner bookmarks and load icons.

https://reviewboard.mozilla.org/r/64220/#review61958

Comment 4

[:Grisha Kruglov]

Comment on attachment 8770893 [details]
Bug 1286203 - Introduce proxy for partner bookmarks and load icons.

https://reviewboard.mozilla.org/r/64220/#review62320

Generally this seem fine, but I think there's a bit of room for improvement, specifically around the ContentProvider proxy. Your call on tests and the smaller nits!

::: mobile/android/base/java/org/mozilla/gecko/distribution/PartnerBookmarksProviderProxy.java:29
(Diff revision 2)
> + * Bookmarks in folder:
> + *   content://{PACKAGE_ID}.partnerbookmarks/bookmarks/{folderId}
> + * Icon of bookmark:
> + *   content://{PACKAGE_ID}.partnerbookmarks/icons/{bookmarkId}
> + */
> +public class PartnerBookmarksProviderProxy extends ContentProvider {

I'd love to see unit tests for this. We already have some unit tests for the BrowserProvider which are easy to write and work well enough. Can we use a similar approach here?

See https://dxr.mozilla.org/mozilla-central/search?q=path%3ABrowserProvider*Test*.java&redirect=false

::: mobile/android/base/java/org/mozilla/gecko/distribution/PartnerBookmarksProviderProxy.java:91
(Diff revision 2)
> +
> +        return true;
> +    }
> +
> +    @Override
> +    public Cursor query(@NonNull Uri uri, String[] projection, String selection, String[] selectionArgs, String sortOrder) {

You're kind of breaking the contract here between a contentprovider and its consumers; the expectation is that consumers are able to pass in their projections, selection and sorting args, but they're ignored here entirely;

and in the fetchContentProviderFavicon you actually do pass in a projection, which doesn't actually do anything.

::: mobile/android/base/java/org/mozilla/gecko/distribution/PartnerBookmarksProviderProxy.java:98
(Diff revision 2)
> +
> +        final ContentResolver contentResolver = assertAndGetContext().getContentResolver();
> +
> +        switch (match) {
> +            case URI_MATCH_BOOKMARKS:
> +                return getBookmarksInFolder(contentResolver, ContentUris.parseId(uri));

.parseId will return -1 if ID isn't found in the last segment of the URI. You probably don't want to pass that down the road into specific getters.

::: mobile/android/base/java/org/mozilla/gecko/distribution/PartnerBookmarksProviderProxy.java:131
(Diff revision 2)
> +                        // Use the ID of the entry as GUID
> +                        PartnerContract.ID + " as " + BrowserContract.Bookmarks.GUID
> +                },
> +                PartnerContract.PARENT + " = ?"
> +                        // Only select entries with valid type
> +                        + " AND (" + BrowserContract.Bookmarks.TYPE + " = 1 OR " + BrowserContract.Bookmarks.TYPE + " = 2)"

Maybe use TYPE_BOOKMARK and TYPE_FOLDER constants?

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:214
(Diff revision 2)
>      }
>  
> +    /**
> +     * Fetch icon from a content provider following the partner bookmarks provider contract.
> +     */
> +    private Bitmap fetchContentProviderFavicon(String uri) {

This will return null in all of these cases:
- passed-in uri is not good
- cursor is null
- no records found in the cursor
- records found, but favicon data for both touchicon and favicon is null

To me this seems a little ambiquous, and at least I would change uri checks to be assertions

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:225
(Diff revision 2)
> +            return null;
> +        }
> +
> +        Cursor cursor = context.getContentResolver().query(
> +                Uri.parse(uri),
> +                new String[] {

this projection does nothing, right?

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:244
(Diff revision 2)
> +            if (!cursor.moveToFirst()) {
> +                return null;
> +            }
> +
> +            final int touchIconIndex = cursor.getColumnIndex(PartnerBookmarksProviderProxy.PartnerContract.TOUCHICON);
> +            if (touchIconIndex != -1 && !cursor.isNull(touchIconIndex)) {

nit:
Are we being particularly paranoid here about the column index because we don't trust our own ContentProvider proxy and want to avoid throwing? (getColumnIndexOrThrow)

I get the isNull check, but I'd prefer to use getColumnIndexOrThrow as we do elsewhere

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:246
(Diff revision 2)
> +            }
> +
> +            final int touchIconIndex = cursor.getColumnIndex(PartnerBookmarksProviderProxy.PartnerContract.TOUCHICON);
> +            if (touchIconIndex != -1 && !cursor.isNull(touchIconIndex)) {
> +                byte[] data = cursor.getBlob(touchIconIndex);
> +                Bitmap bitmap = BitmapFactory.decodeByteArray(data, 0, data.length);

decodeByArray call seems like the only bit of icon data validation we're doing - and it's on the consumer side of the proxy not in the proxy itself (so, easy to forget to do, depending on what consumer will be doing with the data).

Can you clarify that this is enough, and don't see problems arising from this in the future? I'm not entirely certain either way.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:255
(Diff revision 2)
> +            }
> +
> +            final int faviconIndex = cursor.getColumnIndex(PartnerBookmarksProviderProxy.PartnerContract.FAVICON);
> +            if (faviconIndex != -1 && !cursor.isNull(faviconIndex)) {
> +                byte[] data = cursor.getBlob(faviconIndex);
> +                return BitmapFactory.decodeByteArray(data, 0, data.length);

nit: with future refactoring/changes in mind, i think it's worth following the same null-checking pattern for both types of icons.

this bit of code is relying on the fact that it's the last in a sequence of checks (first try touchicon, then try favicon...), which is true now but probably won't always be.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:540
(Diff revision 2)
>  
>                  return iconMap.get(bestSize);
>              }
>          }
>  
> +        if ((FLAG_NO_DOWNLOAD_FROM_GUESSED_DEFAULT_URL & flags) == FLAG_NO_DOWNLOAD_FROM_GUESSED_DEFAULT_URL) {

Can you leave a comment on why this is needed?

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:277
(Diff revision 2)
>  
>      protected void update(String title, String url, long bookmarkId, boolean hasReaderCacheItem) {
>          if (mShowIcons) {
>              // The bookmark id will be 0 (null in database) when the url
> -            // is not a bookmark.
> -            final boolean isBookmark = bookmarkId != 0;
> +            // is not a bookmark and negative for 'fake' bookmarks.
> +            final boolean isBookmark = bookmarkId > 0;

So we specifically don't want to show a blue icon for 'fake' (partner) bookmarks?

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/64220/#review62320

> I'd love to see unit tests for this. We already have some unit tests for the BrowserProvider which are easy to write and work well enough. Can we use a similar approach here?
> 
> See https://dxr.mozilla.org/mozilla-central/search?q=path%3ABrowserProvider*Test*.java&redirect=false

The problem here is that we do not have a partner provider that we can proxy during test though.

> You're kind of breaking the contract here between a contentprovider and its consumers; the expectation is that consumers are able to pass in their projections, selection and sorting args, but they're ignored here entirely;
> 
> and in the fetchContentProviderFavicon you actually do pass in a projection, which doesn't actually do anything.

Yeah, kind of. The content provider is not exported and therefore not accessible outside of our app. I mostly wanted to keep it simple and not handling all parameters even though we do not need dynamic projection and selection right now.

For fetchContentProviderFavicon: The reason for that is that with the change privileged code can load icons from content providers using Favicons.xxx(). This is in theory not limited to our proxy provider and you could load icons from abritrary content providers. That's why I specified explicit columns to look for.

> This will return null in all of these cases:
> - passed-in uri is not good
> - cursor is null
> - no records found in the cursor
> - records found, but favicon data for both touchicon and favicon is null
> 
> To me this seems a little ambiquous, and at least I would change uri checks to be assertions

This is following the structure of the other methods. This method is called in a specific flow and there are early returns in case this code should not continue. Compare with fetchJARFavicon(). I agree this is confusing but I do not intent to change this with this patch. However as part of bug 1265712 I would like to restructure this.

> this projection does nothing, right?

Depends on the content provider. For the proxy: Yep!

> nit:
> Are we being particularly paranoid here about the column index because we don't trust our own ContentProvider proxy and want to avoid throwing? (getColumnIndexOrThrow)
> 
> I get the isNull check, but I'd prefer to use getColumnIndexOrThrow as we do elsewhere

I don't trust the partner provider behind the proxy: I don't know if any of the requested columns will be in the result. I'm not using getColumnIndexOrThrow() because I want to continue if the column doesn't exist for whatever reason: If the TOUCHICON column does not exist but the FAVICON column does then I use the FAVICON.

> decodeByArray call seems like the only bit of icon data validation we're doing - and it's on the consumer side of the proxy not in the proxy itself (so, easy to forget to do, depending on what consumer will be doing with the data).
> 
> Can you clarify that this is enough, and don't see problems arising from this in the future? I'm not entirely certain either way.

I'll switch to FaviconDecoder.decodeFavicon(). This will use the same code as we use for downloading favicons from the web. And this is able to decode ICO too.

> So we specifically don't want to show a blue icon for 'fake' (partner) bookmarks?

Yep, partner bookmarks only show up in the bookmarks panel and we do not show the indicator there. But actually I'm wondering how this works for normal bookmarks. I'll investiage a bit more.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8770893 [details]
Bug 1286203 - Introduce proxy for partner bookmarks and load icons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64220/diff/2-3/

Comment 7

[:Grisha Kruglov]

https://reviewboard.mozilla.org/r/64220/#review62320

> The problem here is that we do not have a partner provider that we can proxy during test though.

Fair enough; I suppose everything could be mocked with enough determination, but now is probably not the time.

Comment 8

[:Grisha Kruglov]

Thanks for clarifications, Sebastian!

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/f70cf3b330110df340514a3dfef0696c852a3ee3
Bug 1286203 - Introduce proxy for partner bookmarks and load icons. r=ahunt,grisha

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8770893 [details]
Bug 1286203 - Introduce proxy for partner bookmarks and load icons.

Approval Request Comment

[Feature/regressing bug #]: Support for the partner bookmarks provider was introduced in bug 1283025 and uplifted to 48.0. (Partnership deal with 48.0 build)

[User impact if declined]: Without this patch favicons served from the partner bookmarks provider are not loaded and displayed in the UI. Instead we show our generic fallback icons.

[Describe test coverage new/current, TreeHerder]: Local testing.

[Risks and why]: Low. This change only affects the partner code which is behind a pref and should not affect all other users. Only small changes to the favicon loader code.

[String/UUID change made/needed]: -

Comment 11

[Sylvestre Ledru [:Sylvestre]]

same as in bug 1286794

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f70cf3b33011

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8770893 [details]
Bug 1286203 - Introduce proxy for partner bookmarks and load icons.

According to comment #10 and #11 in bug 1286794 and it's too late for 48 beta, let's let it ride the train on 49.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e68714c0861a