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

RESOLVED FIXED in Firefox 35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ckitching, Assigned: ckitching)

Tracking

unspecified
Firefox 35
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Comment 3

4 years ago
Created attachment 8485364 [details] [diff] [review]
Fix getFlagsForItem implementation in BrowserDB
Attachment #8485364 - Flags: review?(rnewman)
(Assignee)

Comment 4

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

Updated

4 years ago
Blocks: 1061721
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
Flags: needinfo?(chriskitching)
(Assignee)

Comment 6

4 years ago
(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.
Flags: needinfo?(chriskitching)
(Assignee)

Comment 7

4 years ago
Created attachment 8486601 [details] [diff] [review]
Kill getItemFlags in BrowserDB

All the best patches are >90% red.
Attachment #8486601 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Attachment #8485364 - Attachment is obsolete: true
Attachment #8485364 - Flags: review?(rnewman)
Attachment #8486601 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/4468f6d5d0c5
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.