1063887 - BrowserProvider.getItemFlags does not correctly return results for reading list items.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Chris Kitching [:ckitching]]

See:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java#662

This is incorrect now reading list items have their own table, meaning that, according to this method, nothing is ever a reading list item.

Comment 1

[Chris Kitching [:ckitching]]

Thankfully, static analysis shows us that mReadingListItem in Tab (which is set using this method by the only place that uses it) is never actually used, so this produces no observable symptoms... until someone tries to use the attractive-looking function in BrowserProvider, that is.

Comment 2

[Richard Newman [:rnewman]]

Without digging in: it's entirely possible that the need to look up flags was only needed for the bookmarks DB. Might be possible to rescope/rename the method instead of augmenting it.

Comment 3

[Chris Kitching [:ckitching]]

Attachment: Fix getFlagsForItem implementation in BrowserDB

Comment 4

[Chris Kitching [:ckitching]]

For now, I've fixed up the query.

However, this function is used only here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#432
At that point, it is used to update the write-only field mReadingListItem, so it sort of pointless and could equivalently be replaced with isBookmark().

So, yes, the overlays patch from Bug 1061721 is the only remaining legitimate use of the flags utility, and it could equivalently use two queries. Is the performance worth it? Probably not, but I suspect we'll find other places where the flag utility will come in handy...

Comment 5

[Richard Newman [:rnewman]]

Let's remove mIsReadingListItem (and so on up the chain), and also the flags usage in Tabs, as a Part 1.

Now that we have the separate reading list provider, and we use different mechanisms for showing the reader icon in BrowserApp, this is both violating encapsulation and not buying us anything.

It's up to you whether you want to remove getItemFlags altogether, or keep it (with notes about encapsulation!) to speed up the share overlay.

Let me know what patch I should review.

Comment 6

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #5)
> Let's remove mIsReadingListItem (and so on up the chain), and also the flags
> usage in Tabs, as a Part 1.
> 
> Now that we have the separate reading list provider, and we use different
> mechanisms for showing the reader icon in BrowserApp, this is both violating
> encapsulation and not buying us anything.
> 
> It's up to you whether you want to remove getItemFlags altogether, or keep
> it (with notes about encapsulation!) to speed up the share overlay.
> 
> Let me know what patch I should review.

My general preference is to remove the evil hacky database code if it's no longer pointful. The performance of two queries is still extremely unlikely to exceed our 500ms quota before the user can possibly observe anything strange (the duration of the animation).
So, yes, I'll get a mop.

Comment 7

[Chris Kitching [:ckitching]]

Attachment: Kill getItemFlags in BrowserDB

All the best patches are >90% red.

Comment 8

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/4468f6d5d0c5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4468f6d5d0c5