Closed
Bug 1064246
Opened 10 years ago
Closed 10 years ago
BrowserDB.areContentProvidersDisabled is backwards.
Categories
(Firefox for Android Graveyard :: Data Providers, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: ckitching, Assigned: ckitching)
Details
Attachments
(4 files)
1.10 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
1.99 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
10.79 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
3.83 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
public static boolean areContentProvidersDisabled() {
return sAreContentProvidersEnabled;
}
No.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8485698 -
Flags: review?(rnewman)
Assignee | ||
Comment 2•10 years ago
|
||
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!
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8485701 -
Flags: review?(rnewman)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8485705 -
Flags: review?(rnewman)
Assignee | ||
Comment 5•10 years ago
|
||
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...)
Assignee | ||
Comment 6•10 years ago
|
||
Stumbled across this: it irked me.
Attachment #8485708 -
Flags: review?(rnewman)
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
(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•10 years ago
|
||
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 11•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8485708 -
Flags: review?(rnewman) → review+
Updated•10 years ago
|
Attachment #8485698 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•