Read icons from PartnerBookmarksProvider

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
Distributions
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

unspecified
Firefox 50
All
Android
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 fixed, firefox50 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Component: General → Distributions
(Assignee)

Comment 1

2 years ago
Created attachment 8770893 [details]
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/
Attachment #8770893 - Flags: review?(gkruglov)
Attachment #8770893 - Flags: review?(ahunt)
(Assignee)

Comment 2

2 years ago
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

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

https://reviewboard.mozilla.org/r/64220/#review61958
Attachment #8770893 - Flags: review?(ahunt) → review+
Attachment #8770893 - Flags: review?(gkruglov) → review+
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?
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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/
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.
Thanks for clarifications, Sebastian!
(Assignee)

Comment 9

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

Comment 10

2 years ago
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]: -
Attachment #8770893 - Flags: approval-mozilla-beta?
Attachment #8770893 - Flags: approval-mozilla-aurora?
same as in bug 1286794
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f70cf3b33011
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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.
Attachment #8770893 - Flags: approval-mozilla-beta?
Attachment #8770893 - Flags: approval-mozilla-beta-
Attachment #8770893 - Flags: approval-mozilla-aurora?
Attachment #8770893 - Flags: approval-mozilla-aurora+

Updated

2 years ago
status-firefox48: affected → wontfix

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e68714c0861a
status-firefox49: affected → fixed
You need to log in before you can comment on or make changes to this bug.