1064246 - BrowserDB.areContentProvidersDisabled is backwards.

Status: RESOLVED FIXED

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

Description

[Chris Kitching [:ckitching]]

public static boolean areContentProvidersDisabled() {
    return sAreContentProvidersEnabled;
}


No.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Remove nonsensical dead getter from BrowserDB

Comment 2

[Chris Kitching [:ckitching]]

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!

Comment 3

[Chris Kitching [:ckitching]]

Attachment: Remove some more dead code from BrowserDB

Comment 4

[Chris Kitching [:ckitching]]

Attachment: Remove dead functions from LocalBrowserDB

Comment 5

[Chris Kitching [:ckitching]]

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...)

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Cleanup SpecialFoldersCursorWrapper

Stumbled across this: it irked me.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(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?

Comment 8

[Richard Newman [:rnewman]]

(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.

Comment 9

[Chris Kitching [:ckitching]]

(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 10

[Richard Newman [:rnewman]]

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.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8485705 [details] [diff] [review]
Remove dead functions from LocalBrowserDB

Review of attachment 8485705 [details] [diff] [review]:
-----------------------------------------------------------------

Fine if tests still pass.

Comment 12

[Chris Kitching [:ckitching]]

(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.

Comment 13

[Chris Kitching [:ckitching]]

https://tbpl.mozilla.org/?tree=Try&rev=3e4569dffebe

Comment 14

[Richard Newman [:rnewman]]

(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.

Comment 15

[Chris Kitching [:ckitching]]

(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.

Comment 16

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/e1a0ef9321a5
https://hg.mozilla.org/integration/fx-team/rev/6b06287cae0a
https://hg.mozilla.org/integration/fx-team/rev/1653427f5ee9
https://hg.mozilla.org/integration/fx-team/rev/0e258994bc0b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1a0ef9321a5
https://hg.mozilla.org/mozilla-central/rev/6b06287cae0a
https://hg.mozilla.org/mozilla-central/rev/1653427f5ee9
https://hg.mozilla.org/mozilla-central/rev/0e258994bc0b