Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
866713003:
Show favicons in Site Settings lists. (Closed)
|
Created:
5 years, 11 months ago by Finnur Modified:
5 years, 10 months ago Reviewers:
newt (away) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow favicons in Site Settings lists.
BUG=451374
Committed: https://crrev.com/dddc005051f4e5eb0a5b7e86de0685ee0991c391
Cr-Commit-Position: refs/heads/master@{#313685}
Patch Set 1 #Patch Set 2 : #
Total comments: 19
Patch Set 3 : Review comments #
Total comments: 10
Patch Set 4 : Address comments #Patch Set 5 : Destroy all the instances! #
Total comments: 3
Patch Set 6 : Address review comments #
Total comments: 4
Patch Set 7 : Better cleanup routine #
Messages
Total messages: 17 (2 generated)
finnur@chromium.org changed reviewers: + newt@chromium.org
PTAL. I'm not quite satisfied with how far the icons appear to the left in the list, but my attempts at changing padding/margins have been completely unsuccessful (see note in code).
Could you upload a screenshot of the current icon situation? https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/res/... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/res/... chrome/android/java/res/values/dimens.xml:102: <dimen name="pref_list_favicon">16dp</dimen> I'd group this with the section above. With too many headers, the headers aren't much help anymore. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:47: private int mDesiredSize = 0; how about: mDesiredFaviconSize or just mFaviconSize https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:52: private static final int FAVICON_BACKGROUND_COLOR = It's fine to write colors in hex (0xff969696), and I personally find that easier to read, since it matches what's in our colors.xml files. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:63: // Start the favicon fetching. Will respond in onFaviconAvailable. It would be nice to be smarter about favicon fetching here. If the user has 1000 websites with cookies (which is quite possible), this will incur a huge overhead (at least 10MB of RAM) when opening the cookies or all sites pages. One option: fetch favicons only once the preference is actually shown (e.g. when onBindView() is called). https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:65: int dimension = 100 character limit. No need to wrap here. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:68: (int) (dimension / resources.getDisplayMetrics().density); rather than dividing by the density, I'd store the desired size in DP as a constant in this file, then multiple by the density to get dimension: private static final int FAVICON_SIZE_DP = 16; ... mFaviconSize = Math.round(FAVICON_SIZE_DP * resources.getDisplayMetrics().density); https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:84: if (image == null || image.getHeight() == 0) { does image.getHeight() == 0 happen in practice? https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:90: image = faviconGenerator.generateIconForUrl(faviconUrl()); Can you just use iconUrl here? Calling faviconUrl() again seems wasteful. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:112: // Both icon functions (the one that fetches the favicon and the one How about using android.net.Uri.Builder here? That seems at least better than constructing URLs, parsing them, and catching parse exceptions. E.g.: Uri uri = Uri.parse(mSite.getAddress().getOrigin()); if (uri.getPort() != null) { // Remove port. uri = uri.buildUpon().authority(uri.host()).build(); } return uri.toString(); https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:185: The code below doesn't work (does not affect margins/paddings at all). The icon and title view already have padding and margins of 0, so I'm not sure what you're trying to achieve here. See Android's preference_holo.xml and preference_material.xml. If you're trying to get rid of the spacing between the icon and the title, that's going to be tricky with the default preference layout since the icon is wrapped in a parent view that has a min width. You *could* remove the parent's min width, but then we're relying on implementation details in Android (maybe OK here, since preference_holo and preference_material won't change, but preference_fancy_new_future_android_ui might have a very different structure). Alternatively, you could use a custom preference layout.
How about this? https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/res/... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/res/... chrome/android/java/res/values/dimens.xml:102: <dimen name="pref_list_favicon">16dp</dimen> On 2015/01/22 17:17:23, newt wrote: > I'd group this with the section above. With too many headers, the headers aren't > much help anymore. Done. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:47: private int mDesiredSize = 0; On 2015/01/22 17:17:23, newt wrote: > how about: mDesiredFaviconSize or just mFaviconSize Obsolete. Was able to do away with this var. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:52: private static final int FAVICON_BACKGROUND_COLOR = On 2015/01/22 17:17:24, newt wrote: > It's fine to write colors in hex (0xff969696), and I personally find that easier > to read, since it matches what's in our colors.xml files. Done. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:63: // Start the favicon fetching. Will respond in onFaviconAvailable. Good suggestion. Done. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:65: int dimension = On 2015/01/22 17:17:23, newt wrote: > 100 character limit. No need to wrap here. Done. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:68: (int) (dimension / resources.getDisplayMetrics().density); Done. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:84: if (image == null || image.getHeight() == 0) { No, I had some problems with icons not appearing at all at first, thinking I was getting invalid icons. This code was testing, but forgot to delete. Thanks for catching. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:90: image = faviconGenerator.generateIconForUrl(faviconUrl()); No, we need to create a generic image because the icon couldn't be retrieved and iconUrl is therefore blank. https://codereview.chromium.org/866713003/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:112: // Both icon functions (the one that fetches the favicon and the one Ooh, I like that.
https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:42: private FaviconHelper mFaviconHelper; mFaviconHelper needs to be destroyed when you're done using it. Otherwise its native side will be leaked. https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:67: public void onFaviconAvailable(Bitmap image, String iconUrl) { nit: I'd put the favicon related methods next to each other, e.g. move site() above onFaviconAvailable() https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:90: private String faviconUrl() { I think you can further simplify this: String origin = mSite.getAddress().getOrigin(); if (origin == null) { return "http://" + mSite.getAddress().getHost(); } Uri uri = Uri.parse(origin); if (uri.getPort() != -1) { // Remove the port. uri = uri.buildUpon().authority(uri.host()).build(); } return uri.toString(); Also, I noticed that getPort() returns -1 in the case where there's no port. https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:173: LinearLayout parentLayout = (LinearLayout) icon.getParent(); lines 173-176 need fixing: 1) Don't cast to LinearLayout before checking if it is a LinearLayout 2) Don't use integer constants for min width and padding.
PTAL https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:42: private FaviconHelper mFaviconHelper; On 2015/01/23 17:10:39, newt wrote: > mFaviconHelper needs to be destroyed when you're done using it. Otherwise its > native side will be leaked. Done. Do we need to worry about this class being destroyed while the request is live? Should I null out mFaviconHelper (when destroying in the callback), add a finalizer and destroy it there also (if not null already)? https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:67: public void onFaviconAvailable(Bitmap image, String iconUrl) { On 2015/01/23 17:10:39, newt wrote: > nit: I'd put the favicon related methods next to each other, e.g. move site() > above onFaviconAvailable() Done. https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:90: private String faviconUrl() { Done. https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:173: LinearLayout parentLayout = (LinearLayout) icon.getParent(); On 2015/01/23 17:10:39, newt wrote: > lines 173-176 need fixing: > 1) Don't cast to LinearLayout before checking if it is a LinearLayout > 2) Don't use integer constants for min width and padding. 1) Done. 2) Both those functions take an int, so I presume you mean to define this as const at the tops (as opposed to changing the type)? If so, done.
https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:42: private FaviconHelper mFaviconHelper; On 2015/01/26 11:31:10, Finnur wrote: > On 2015/01/23 17:10:39, newt wrote: > > mFaviconHelper needs to be destroyed when you're done using it. Otherwise its > > native side will be leaked. > > Done. Do we need to worry about this class being destroyed while the request is > live? Should I null out mFaviconHelper (when destroying in the callback), add a > finalizer and destroy it there also (if not null already)? No. FaviconHelper can be destroyed at any time. Callbacks won't be received after it's destroyed. (FaviconHelper uses a CancelableTaskTracker when requesting favicons, and in FaviconHelper's destructor, the CancelableTaskTracker is destructed, which cancels the favicon requests) https://codereview.chromium.org/866713003/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:52: private static final int FAVICON_PARENT_MINWIDTH = 110; These should be specified in DP and multiplied by the current density. https://codereview.chromium.org/866713003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:165: mFaviconHelper.destroy(); I'd set mFaviconHelper to null after destroying it to prevent accidental use after destruction. Same above.
Done. PTAL. https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:42: private FaviconHelper mFaviconHelper; I'm not sure I made myself clear. Or I might be misreading your reply. Here's the scenario I was thinking of: 1) A site is scrolled into view, causing the WebsitePreference to be created and a request fired off to fetch the icon. 2) User scrolls the list before the request is satisfied. 3) As you point out, the request gets cancelled, which means onFavIconAvailable isn't called and therefore destroy() is not explicitly called on the FaviconHelper. Are you suggesting we don't need to worry about this scenario because the FaviconHelper gets destroyed anyway (in some other way)? https://codereview.chromium.org/866713003/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:165: mFaviconHelper.destroy(); Good point.
On 2015/01/27 11:28:14, Finnur wrote: > I'm not sure I made myself clear. Or I might be misreading your reply. Here's > the scenario I was thinking of: > > 1) A site is scrolled into view, causing the WebsitePreference to be created and > a request fired off to fetch the icon. > 2) User scrolls the list before the request is satisfied. > 3) As you point out, the request gets cancelled, which means onFavIconAvailable > isn't called and therefore destroy() is not explicitly called on the > FaviconHelper. > > Are you suggesting we don't need to worry about this scenario because the > FaviconHelper gets destroyed anyway (in some other way)? This scenario can't happen. The favicon request is canceled only if you call destroy() on the FaviconHelper. So, "the request gets canceled" in step 3 won't happen just because you scrolled the WebsitePreference off the screen. Here's how I'd think about it: The request is guaranteed to result in onFaviconAvailable() being called at some point. At that point you can destroy the FaviconHelper and all will be good. The only think you need to ensure is that onFaviconAvailable() is safe to call after the WebsitePreference object is no longer being used, e.g. after the activity has finished. In this case, I think it is, but it'd be reasonable to test it out locally by adding an artificial delay before onFaviconAvailable() is called.
https://codereview.chromium.org/866713003/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:165: mFaviconHelper.destroy(); Actually, you should call onFaviconAvailable(null, null) here. That will both destroy() the FaviconHelper and generated the placeholder favicon. https://codereview.chromium.org/866713003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:175: if (parentLayout != null && parentLayout instanceof LinearLayout) { This line isn't needed. You already know that parent is a non-null LinearLayout thanks to the instanceof check on line 173.
PTAL https://codereview.chromium.org/866713003/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java (right): https://codereview.chromium.org/866713003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:165: mFaviconHelper.destroy(); Ooh, I like that idea. https://codereview.chromium.org/866713003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreference.java:175: if (parentLayout != null && parentLayout instanceof LinearLayout) { Duh.
Almost forgot. Also added a delay to the history backend service to delay the favicon response by a few seconds and it works as expected. I can scroll a view out of um... view (no pun intended) and it works just fine.
lgtm ship it!
The CQ bit was checked by finnur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866713003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/dddc005051f4e5eb0a5b7e86de0685ee0991c391 Cr-Commit-Position: refs/heads/master@{#313685} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
