Closed Bug 1064246 Opened 10 years ago Closed 10 years ago

BrowserDB.areContentProvidersDisabled is backwards.

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: ckitching, Assigned: ckitching)

Details

Attachments

(4 files)

public static boolean areContentProvidersDisabled() { return sAreContentProvidersEnabled; } No.
Static analysis of Java shows it to be dead, and it's not annotated to be kept for access from JNI... so it's both wrong and never used. Stabby stabby!
Turns out there's a bunch of dead wrappers in BrowserDB that we probably want to kill, and a bunch of dead functions in LocalBrowserDB that we *might* want to kill (though these plausibly might come in handy in the future, and Proguard'll be eating them anyway...)
Stumbled across this: it irked me.
Attachment #8485708 - Flags: review?(rnewman)
(In reply to Chris Kitching [:ckitching] from comment #4) > Created attachment 8485705 [details] [diff] [review] > Remove dead functions from LocalBrowserDB I just wanted to see why these were added: * isVisited was added in bug 824994 to support hiding a banner. Banners are now supported in JS and we do want to eventually support hiding the Marketplace banner, but we'll use a different tactic. * unpinAllSites was added in bug 783312, but we no longer support unpinning all sites. * I'm not sure why we no longer use registerHistoryObserver (added in bug 671131), but it seems to have been removed after moving the awesomescreen to a home panel. * I wonder if getReadingListCount would be useful for the new Reading List badge?
(In reply to Mark Finkle (:mfinkle) from comment #7) > * I'm not sure why we no longer use registerHistoryObserver (added in bug > 671131), but it seems to have been removed after moving the awesomescreen to > a home panel. Probably because we now regenerate top sites etc. at saner times, and everything else uses a cursorloader. > * I wonder if getReadingListCount would be useful for the new Reading List > badge? Maybe, but I think we're just going with a single 'bead' to indicate that there are unread items.
(In reply to Richard Newman [:rnewman] from comment #8) > Maybe, but I think we're just going with a single 'bead' to indicate that > there are unread items. Seems like it'd be sufficient to set a flag somewhere whenever the overlay adds something and then clear the flag whenever the reading list is opened.
Comment on attachment 8485701 [details] [diff] [review] Remove some more dead code from BrowserDB Review of attachment 8485701 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/BrowserDB.java @@ -236,5 @@ > public static void registerBookmarkObserver(ContentResolver cr, ContentObserver observer) { > sDb.registerBookmarkObserver(cr, observer); > } > > - public static void registerHistoryObserver(ContentResolver cr, ContentObserver observer) { You can remove this from LocalBrowserDB, too. @@ -257,5 @@ > sDb.unpinSite(cr, position); > } > > - public static void unpinAllSites(ContentResolver cr) { > - sDb.unpinAllSites(cr); Likewise. @@ -261,5 @@ > - sDb.unpinAllSites(cr); > - } > - > - public static Cursor getPinnedSites(ContentResolver cr, int limit) { > - return sDb.getPinnedSites(cr, limit); But not in this case. LocalBrowserDB's version is used in getTopSites.
Attachment #8485701 - Flags: review?(rnewman) → review+
Comment on attachment 8485705 [details] [diff] [review] Remove dead functions from LocalBrowserDB Review of attachment 8485705 [details] [diff] [review]: ----------------------------------------------------------------- Fine if tests still pass.
Attachment #8485705 - Flags: review?(rnewman) → review+
Attachment #8485708 - Flags: review?(rnewman) → review+
Attachment #8485698 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #10) > Comment on attachment 8485701 [details] [diff] [review] > Remove some more dead code from BrowserDB > > Review of attachment 8485701 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/db/BrowserDB.java > @@ -236,5 @@ > > public static void registerBookmarkObserver(ContentResolver cr, ContentObserver observer) { > > sDb.registerBookmarkObserver(cr, observer); > > } > > > > - public static void registerHistoryObserver(ContentResolver cr, ContentObserver observer) { > > You can remove this from LocalBrowserDB, too. > > @@ -257,5 @@ > > sDb.unpinSite(cr, position); > > } > > > > - public static void unpinAllSites(ContentResolver cr) { > > - sDb.unpinAllSites(cr); > > Likewise. > > @@ -261,5 @@ > > - sDb.unpinAllSites(cr); > > - } > > - > > - public static Cursor getPinnedSites(ContentResolver cr, int limit) { > > - return sDb.getPinnedSites(cr, limit); > > But not in this case. LocalBrowserDB's version is used in getTopSites. I think all three comments are manifest in the next patch. Looks like splitting them up did not aid clarity.
(In reply to Chris Kitching [:ckitching] from comment #9) > Seems like it'd be sufficient to set a flag somewhere whenever the overlay > adds something and then clear the flag whenever the reading list is opened. Read/unread state is a property of items in the DB.
(In reply to Richard Newman [:rnewman] from comment #14) > (In reply to Chris Kitching [:ckitching] from comment #9) > > > Seems like it'd be sufficient to set a flag somewhere whenever the overlay > > adds something and then clear the flag whenever the reading list is opened. > > Read/unread state is a property of items in the DB. True, and I guess we want to track readness in general instead of just the "minimum necessary for Yuan's pretty orange circle to work right", which would likely prove limiting when we try and do more cool stuff later.
Flags: qe-verify-
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: