Closed
Bug 1064031
Opened 10 years ago
Closed 4 years ago
Favicon support for share overlay
Categories
(Firefox for Android Graveyard :: Overlays, defect)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: ckitching, Assigned: ckitching)
References
Details
Attachments
(3 files)
28.32 KB,
patch
|
rnewman
:
review-
|
Details | Diff | Splinter Review |
21.19 KB,
patch
|
rnewman
:
review-
|
Details | Diff | Splinter Review |
23.44 KB,
patch
|
rnewman
:
review-
|
Details | Diff | Splinter Review |
We should attempt to load a favicon from the browser database and display it if we find one.
This should be pretty trivial: the favicon system already does everything we need (including URL-guessing), we just have to turn it on and display anything it gives us. SIMPLEZ.
... Just need to get the slight refactoring bug to land before this is possible.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8485495 -
Flags: review?(rnewman)
Assignee | ||
Comment 2•10 years ago
|
||
Alas, database hits are quite rare, but here it is. Telemetry will let us know just how rare such hits are...
Assignee | ||
Comment 3•10 years ago
|
||
Comment 4•10 years ago
|
||
Comment on attachment 8485495 [details] [diff] [review]
Add favicon support to the share overlay
Review of attachment 8485495 [details] [diff] [review]:
-----------------------------------------------------------------
I think this patch needs some cleanup and rebasing. Beyond that, I have some objections.
* I don't like to see Favicons inited with a UA. You're conflating scope with state.
* I don't like to see BrowserDB and LocalBrowserDB switched for convenience. BrowserDB is the static wrapper for a LocalBrowserDB for the current profile. LocalBrowserDB is a pointer to a particular profile's ContentProvider. The Favicons class shouldn't be coupled to a single instance; it uses BrowserDB as an intermediary to avoid that.
It's totally fine to use a LocalBrowserDB within the scope of an individual call. It's not OK to retain an instance.
I think most of this work is to avoid having Favicons APIs take a DB argument, right? So re-approach from that angle.
::: mobile/android/base/GeckoApp.java
@@ +22,5 @@
> import java.util.Set;
> import java.util.regex.Matcher;
> import java.util.regex.Pattern;
>
> +import android.net.http.AndroidHttpClient;
Remove.
@@ +30,5 @@
> import org.mozilla.gecko.AppConstants.Versions;
> import org.mozilla.gecko.GeckoProfileDirectories.NoMozillaDirectoryException;
> import org.mozilla.gecko.db.BrowserDB;
> import org.mozilla.gecko.favicons.Favicons;
> +import org.mozilla.gecko.favicons.LoadFaviconTask;
Remove.
::: mobile/android/base/db/BrowserDB.java
@@ +46,5 @@
> sDb = new LocalBrowserDB(profile);
> }
>
> + public static LocalBrowserDB getLocalBrowserDB() {
> + return sDb;
Don't do this. It defeats the point of having BrowserDB, no?
::: mobile/android/base/db/LocalBrowserDB.java
@@ +1196,5 @@
> + /**
> + * Get the title from the history table for a given page URL.
> + */
> + public String getTitleForURL(ContentResolver cr, String uri) {
> + Cursor c = null;
No no no
::: mobile/android/base/favicons/Favicons.java
@@ +64,5 @@
>
> // The density-adjusted maximum Favicon dimensions.
> public static int largestFaviconSize;
>
> + public static LocalBrowserDB browserDB;
This seems like a terrible idea. You just *implicitly* tied the favicon cache to a particular profile.
@@ +386,5 @@
> * Consider replacing with references to a staticly held reference to the GeckoApp object.
> *
> * @param context A reference to the GeckoApp instance.
> */
> + public static void initialiseWithContext(Context context, String userAgent, LocalBrowserDB db) throws IllegalStateException {
z, but also: why does my Favicons instance need a user agent?
@@ +391,2 @@
> // Prevent multiple-initialisation.
> if (!isInitialised.compareAndSet(false, true)) {
So a user shares a page to Firefox, we init Favicons with a null UA, and then they launch the browser. What UA does Favicons use for fetches?
::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +242,5 @@
> + * If any is found, it'll be displayed. Otherwise, the default globe icon will continue to be
> + * shown.
> + */
> + private void loadFavicon(String pageURL, LocalBrowserDB db) {
> + Favicons.initialiseWithContext(this, null, db);
That "null" is a big red flag that the interface is poorly designed.
::: mobile/android/base/util/ThreadUtils.java
@@ +83,5 @@
> }
>
> public static void setUiThread(Thread thread, Handler handler) {
> + // Prevent multiple-initialisation.
> + if (!initialised.compareAndSet(false, true)) {
Needs rebasing on top of 'z'.
Attachment #8485495 -
Flags: review?(rnewman) → review-
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #4)
> * I don't like to see Favicons inited with a UA. You're conflating scope
> with state.
The issue was that LoadFaviconTask initialises httpClient in <clinit> as follows:
static AndroidHttpClient httpClient = AndroidHttpClient.newInstance(GeckoAppShell.getGeckoInterface().getDefaultUAString());
Since getGeckoInterface() returns null as Gecko isn't running, this fails.
More generally, this means we can't seem get our useragent without Gecko, so I'm not sure what should be used as the UA.
> * I don't like to see BrowserDB and LocalBrowserDB switched for convenience.
> BrowserDB is the static wrapper for a LocalBrowserDB for the current
> profile. LocalBrowserDB is a pointer to a particular profile's
> ContentProvider. The Favicons class shouldn't be coupled to a single
> instance; it uses BrowserDB as an intermediary to avoid that.
Okay, what I have here is definitely wrong, but BrowserDB isn't the right way for it to be decoupled, either.
Fennec initialises BrowserDB with a LocalBrowserDB appropriate to the profile on startup. The overlay always uses the default profile.
If I start Fennec with a non-default profile, then use the overlay, the overlay uses BrowserDB and points it back to the default profile, then I return to Fennec, I have corrupted state...
It seems that we should be
>
> It's totally fine to use a LocalBrowserDB within the scope of an individual
> call. It's not OK to retain an instance.
We don't want to construct a new LocalBrowserDB instance every time there's a call... do we?
> I think most of this work is to avoid having Favicons APIs take a DB
> argument, right? So re-approach from that angle.
... Maybe this is suddenly the right way to go? Provide two versions of every method: One that takes a LocalBrowserDB, and one that just fetches the LocalBrowserDB from BrowserDB and passes it to the first one.
> > }
> >
> > + public static LocalBrowserDB getLocalBrowserDB() {
> > + return sDb;
>
> Don't do this. It defeats the point of having BrowserDB, no?
Okay, so apparently I've missed the point of BrowserDB.
> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +1196,5 @@
> > + /**
> > + * Get the title from the history table for a given page URL.
> > + */
> > + public String getTitleForURL(ContentResolver cr, String uri) {
> > + Cursor c = null;
>
> No no no
Ah, I appear to have munged two patches into one.
I take it from the screams and the blood dripping from the ceiling that you don't want to try to fetch a title from the DB, then?
In fairness, that seems like it will work exactly 0% of the time, so it a completely stupid thing to do.
Okay, yes, let's not do that. Wow. Such fail.
> ::: mobile/android/base/favicons/Favicons.java
> @@ +64,5 @@
> >
> > // The density-adjusted maximum Favicon dimensions.
> > public static int largestFaviconSize;
> >
> > + public static LocalBrowserDB browserDB;
>
> This seems like a terrible idea. You just *implicitly* tied the favicon
> cache to a particular profile.
True. Let's not do that.
> @@ +386,5 @@
> > * Consider replacing with references to a staticly held reference to the GeckoApp object.
> > *
> > * @param context A reference to the GeckoApp instance.
> > */
> > + public static void initialiseWithContext(Context context, String userAgent, LocalBrowserDB db) throws IllegalStateException {
>
> z, but also: why does my Favicons instance need a user agent?
See above.
> @@ +391,2 @@
> > // Prevent multiple-initialisation.
> > if (!isInitialised.compareAndSet(false, true)) {
>
> So a user shares a page to Firefox, we init Favicons with a null UA, and
> then they launch the browser. What UA does Favicons use for fetches?
How can I get the right UA from Gecko without Gecko?
Comment 6•10 years ago
|
||
(In reply to Chris Kitching [:ckitching] from comment #5)
> Since getGeckoInterface() returns null as Gecko isn't running, this fails.
No, it returns null until GeckoApp.onCreate is called.
I'm not sure why the UA method is part of this interface. If you care, you can find out.
This is part of Fennec's historical -- and wrong -- assumption that GeckoApp is the universe. It's getting better.
> We don't want to construct a new LocalBrowserDB instance every time there's
> a call... do we?
False dichotomy.
> How can I get the right UA from Gecko without Gecko?
public String getDefaultUAString() {
return HardwareUtils.isTablet() ? AppConstants.USER_AGENT_FENNEC_TABLET :
AppConstants.USER_AGENT_FENNEC_MOBILE;
}
seems like a pretty good first step.
Yes, this is code duplication, but it's much, much less coupling.
If you want to limit the duplication, then maybe move that method to GeckoApplication, make it static, and just call it from GeckoApp's getDefaultUAString.
(And mark GeckoApp's implementation of GeckoInterface methods @Override, while you're there...?)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #6)
> Yes, this is code duplication, but it's much, much less coupling.
>
> If you want to limit the duplication, then maybe move that method to
> GeckoApplication, make it static, and just call it from GeckoApp's
> getDefaultUAString.
>
> (And mark GeckoApp's implementation of GeckoInterface methods @Override,
> while you're there...?)
I also mopped up a few effectively-final, effectively-static, and effectively-abstract things while I was there.
I'm not sure why you want this in GeckoApplication instead of GeckoApp, but this is probably something to do with my not really understanding what the difference between BrowserApp, GeckoApp, and GeckoApplication are (in terms of what their defined roles are, I mean. The lack of a comment explaining this in any of these classes isn't helping....)
Still: Cleanup time!
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8488483 -
Flags: review?(rnewman)
Comment 9•10 years ago
|
||
GeckoApplication is our Application class.
GeckoApp should really be called GeckoActivity.
BrowserApp is the concrete subclass for our main browser activity.
Assignee | ||
Comment 10•10 years ago
|
||
Now with 23% less stupid
Attachment #8488828 -
Flags: review?(rnewman)
Comment 11•10 years ago
|
||
Comment on attachment 8488828 [details] [diff] [review]
Add favicon support to the share overlay
Review of attachment 8488828 [details] [diff] [review]:
-----------------------------------------------------------------
r- for sadness.
You seem to be stuck on Favicons needing to use one API for both cases; we're bouncing between two extremes. I don't think that's correct.
We need to keep serving via BrowserDB, working correctly for guest mode, but also support lookup from the share overlay. That implies two paths.
::: mobile/android/base/GeckoApp.java
@@ +30,5 @@
> import org.mozilla.gecko.AppConstants.Versions;
> import org.mozilla.gecko.GeckoProfileDirectories.NoMozillaDirectoryException;
> import org.mozilla.gecko.db.BrowserDB;
> import org.mozilla.gecko.favicons.Favicons;
> +import org.mozilla.gecko.favicons.LoadFaviconTask;
Kill both of the new imports in this file.
::: mobile/android/base/favicons/Favicons.java
@@ +260,5 @@
> return targetURL;
> }
> }
>
> + LocalBrowserDB browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);
This will be wrong when Fennec is running with a named profile, or in Guest Mode. That's part of the point of using BrowserDB.
::: mobile/android/base/favicons/LoadFaviconTask.java
@@ +75,5 @@
> + private static final AndroidHttpClient httpClient = AndroidHttpClient.newInstance(GeckoApplication.getDefaultUAString());
> +
> + public LoadFaviconTask(Context context, String pageURL, String faviconURL, int flags, OnFaviconLoadedListener listener,
> + int targetWidth, boolean onlyFromLocal) {
> + this(context, GeckoProfile.DEFAULT_PROFILE, pageURL, faviconURL, flags, listener, targetWidth, onlyFromLocal);
Get rid of this constructor. We never want to use DEFAULT_PROFILE implicitly.
@@ +90,5 @@
> this.flags = flags;
> this.targetWidth = targetWidth;
> this.onlyFromLocal = onlyFromLocal;
> +
> + browserDB = new LocalBrowserDB(profileName);
This is a bad idea. Scrolling through the history list will generate a new LocalBrowserDB for each row.
::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +105,5 @@
>
> + // Initialise ThreadUtils if it's not already been initialised by Fennec...
> + ThreadUtils.setUiThread(Thread.currentThread(), new Handler());
> +
> + // Start HardwareUtils (so we can figure out what type of device we are and hence our UA.
Closing paren.
Attachment #8488828 -
Flags: review?(rnewman) → review-
Comment 12•10 years ago
|
||
Comment on attachment 8488483 [details] [diff] [review]
Refactor GeckoApp and friends to make UA accessible
Review of attachment 8488483 [details] [diff] [review]:
-----------------------------------------------------------------
The bit you've lost here is the ability to behave differently for different GeckoInterfaces. It'll always call into GeckoApp's.
This is part of what GeckoInterface is for.
Imagine if BrowserApp, or WebAppImpl, or my own GeckoView application wanted to override getDefaultUAString, and have it affect favicon fetches etc.
We no longer can, for two reasons:
* It's static
* It's hanging off GeckoApplication, which is *not* the application class for GeckoView apps.
So: can you come up with a solution that makes the default UA string accessible for *our* use (when GeckoApplication is our app class), but still allows for Gecko-ish features hanging off GeckoApp to specify their UA?
Please file a bug for this particular part, and maybe a separate bug for the (very worthwhile) cleanup that's hiding in this patch. I'd like to get that landed sooner rather than later.
::: mobile/android/base/GeckoApp.java
@@ +184,5 @@
>
> private FullScreenHolder mFullScreenPluginContainer;
> private View mFullScreenPluginView;
>
> + private final HashMap<String, PowerManager.WakeLock> mWakeLocks = new HashMap<>();
Please move this cleanup (and others in this patch) to a Part 0.
Attachment #8488483 -
Flags: review?(rnewman) → review-
Comment 13•4 years ago
|
||
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
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
•