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.
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.
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.
Component: General → Data Providers
Hardware: ARM → All
Created attachment 8485364 [details] [diff] [review] Fix getFlagsForItem implementation in BrowserDB
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...
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.
Status: NEW → ASSIGNED
(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.
Created attachment 8486601 [details] [diff] [review] Kill getItemFlags in BrowserDB All the best patches are >90% red.
Attachment #8486601 - Flags: review?(rnewman)
Attachment #8486601 - Flags: review?(rnewman) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.