Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
364793005:
Implementation of GetFavicon for current tab. (Closed)
|
Created:
6 years, 5 months ago by Jitu( very slow this week) Modified:
6 years, 4 months ago CC:
chromium-reviews, avayvod+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionImplementation of GetFavicon for current tab.
This patch returns the bitmap of the current tab favicon.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289275
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixed the review comments #
Total comments: 1
Patch Set 3 : Rename function #
Total comments: 4
Patch Set 4 : Fixed the review comment #
Total comments: 6
Patch Set 5 : Scaled favicon image to 16x16 size #
Total comments: 1
Patch Set 6 : Cache the favicon #
Total comments: 6
Patch Set 7 : Resize to 16X16 dp #
Total comments: 7
Patch Set 8 : Fixed review comments #
Total comments: 1
Patch Set 9 : Fix the review comments and rebase the patch #
Total comments: 3
Patch Set 10 : Changed as per suggested #
Messages
Total messages: 30 (0 generated)
PTAL
Nice, thanks! https://codereview.chromium.org/364793005/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://codereview.chromium.org/364793005/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:886: if (mNativeTabAndroid != 0) Nit: I would probably invert this and bail out early if |mNativeTabAndroid| is null. https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.cc:537: return bitmap; The downstream version of this code will try to get a favicon with the right density for the device, and scale it otherwise. I think it would make sense to upstream that eventually, but for now I think this is fine. https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.cc:538: } Nit: empty line afterwards. https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... File chrome/browser/android/tab_android.h (right): https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.h:150: // Return current tab favicon Nit: This comment doesn't say anything that the method name doesn't already say.
Fixed the review comment. PTAL https://codereview.chromium.org/364793005/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://codereview.chromium.org/364793005/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:886: if (mNativeTabAndroid != 0) On 2014/07/02 13:32:28, Bernhard Bauer wrote: > Nit: I would probably invert this and bail out early if |mNativeTabAndroid| is > null. Done. https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.cc:538: } On 2014/07/02 13:32:28, Bernhard Bauer wrote: > Nit: empty line afterwards. Done. https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... File chrome/browser/android/tab_android.h (right): https://codereview.chromium.org/364793005/diff/1/chrome/browser/android/tab_a... chrome/browser/android/tab_android.h:150: // Return current tab favicon On 2014/07/02 13:32:28, Bernhard Bauer wrote: > Nit: This comment doesn't say anything that the method name doesn't already say. Done.
PTAL
https://codereview.chromium.org/364793005/diff/20001/chrome/browser/android/t... File chrome/browser/android/tab_android.h (right): https://codereview.chromium.org/364793005/diff/20001/chrome/browser/android/t... chrome/browser/android/tab_android.h:153: jobject obj); I think I would rename this back. The "current tab" doesn't make sense for a method on a tab object. It's always going to be this tab's favicon (and that is implicit).
PTAL
LGTM, but you might want to wait for David and/or Yaron to take a look as well.
https://codereview.chromium.org/364793005/diff/30001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://codereview.chromium.org/364793005/diff/30001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:883: * TODO(bauerb): Upstream implementation. At a minimum this needs to be updated. We should probably change to something like "reconcile with the downstream" version. https://codereview.chromium.org/364793005/diff/30001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1013: protected void onFaviconUpdated() { Does this get called with your change?
Fixed the review comments. PTAL https://codereview.chromium.org/364793005/diff/30001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://codereview.chromium.org/364793005/diff/30001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:883: * TODO(bauerb): Upstream implementation. On 2014/07/02 17:43:08, Yaron wrote: > At a minimum this needs to be updated. We should probably change to something > like "reconcile with the downstream" version. Done. https://codereview.chromium.org/364793005/diff/30001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1013: protected void onFaviconUpdated() { On 2014/07/02 17:43:08, Yaron wrote: > Does this get called with your change? Yes.
https://chromiumcodereview.appspot.com/364793005/diff/50001/chrome/android/ja... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://chromiumcodereview.appspot.com/364793005/diff/50001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:884: public Bitmap getFavicon() { We need to make sure this bitmap is scaled to 16x16dp here (given the javadoc). Can we Check the width and height of the returned bitmap and if they aren't 16x16 dp we scale them to that size? We could also do this in native with the other scaling. Also, it might be nice to cache the favicon in this class (as well as the URL we were on when we last grabbed the favicon). If the cached URL doesn't match the current URL here we could invalidate the cached value. We could also invalidate it in onFaviconUpdated as well. https://chromiumcodereview.appspot.com/364793005/diff/50001/chrome/browser/an... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/364793005/diff/50001/chrome/browser/an... chrome/browser/android/tab_android.cc:53: using gfx::ConvertToJavaBitmap; Can we just use gfx::ConvertToJavaBitmap explicitly? https://chromiumcodereview.appspot.com/364793005/diff/50001/chrome/browser/an... chrome/browser/android/tab_android.cc:533: const SkBitmap& favicon = favicon_tab_helper->GetFavicon().AsBitmap(); Should we be scaling this based on the device density? Maybe see if the following gives you a non-empty bitmap, and if not then use the AsBitmap() method. tab_helper->GetFavicon().AsImageSkia().GetRepresentation(ResourceBundle::GetSharedInstance::GetMaxScaleFactor()).sk_bitmap();
@David, PTAL to this patch. Also i have some queries. Thanks https://codereview.chromium.org/364793005/diff/50001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://codereview.chromium.org/364793005/diff/50001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:884: public Bitmap getFavicon() { I have changed the bitmap to 16x16 in native side. As per the current implementation once we get the callback onFaviconUpdated() application can call GetFavicon() to get the icon for current tab. Regarding your 2nd comment for caching the favicon and the URL, I have one queries What if the favicon image changed in for the website, still we holds the old favicon icon as current Tab cached URL matches the current URL. Plz correct me if i am wrong. https://codereview.chromium.org/364793005/diff/50001/chrome/browser/android/t... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/364793005/diff/50001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:53: using gfx::ConvertToJavaBitmap; On 2014/07/07 17:59:04, David Trainor wrote: > Can we just use gfx::ConvertToJavaBitmap explicitly? Done. https://codereview.chromium.org/364793005/diff/50001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:533: const SkBitmap& favicon = favicon_tab_helper->GetFavicon().AsBitmap(); As per description GetRepresentation(float scale) returns the image whose density best matches to scale else it returns the null image. Currently here i have done changes for resizing the bitmap if size is not 16x16. Please let me know your comments any thing else i will take care here.
On 2014/07/09 11:07:12, Jitu wrote: > @David, > > PTAL to this patch. > > Also i have some queries. > > Thanks > > https://codereview.chromium.org/364793005/diff/50001/chrome/android/java/src/... > File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): > > https://codereview.chromium.org/364793005/diff/50001/chrome/android/java/src/... > chrome/android/java/src/org/chromium/chrome/browser/Tab.java:884: public Bitmap > getFavicon() { > I have changed the bitmap to 16x16 in native side. We need 16 dp not 16 px, so I think you need to use GetRepresentation then scale properly based on the device display density. > > As per the current implementation once we get the callback onFaviconUpdated() > application can call GetFavicon() to get the icon for current tab. > > Regarding your 2nd comment for caching the favicon and the URL, > I have one queries > > What if the favicon image changed in for the website, still we holds the old > favicon icon as current Tab cached URL matches the current URL. > > Plz correct me if i am wrong. You're correct :). But as I mentioned above, you could invalidate the cached favicon/URL inside onFaviconUpdated(). That way the next call to getFavicon() will grab the correct image. > > https://codereview.chromium.org/364793005/diff/50001/chrome/browser/android/t... > File chrome/browser/android/tab_android.cc (right): > > https://codereview.chromium.org/364793005/diff/50001/chrome/browser/android/t... > chrome/browser/android/tab_android.cc:53: using gfx::ConvertToJavaBitmap; > On 2014/07/07 17:59:04, David Trainor wrote: > > Can we just use gfx::ConvertToJavaBitmap explicitly? > > Done. > > https://codereview.chromium.org/364793005/diff/50001/chrome/browser/android/t... > chrome/browser/android/tab_android.cc:533: const SkBitmap& favicon = > favicon_tab_helper->GetFavicon().AsBitmap(); > > As per description GetRepresentation(float scale) returns the image whose > density best matches to scale > else it returns the null image. > > Currently here i have done changes for resizing the bitmap if size is not 16x16. > > Please let me know your comments any thing else i will take care here.
https://chromiumcodereview.appspot.com/364793005/diff/70001/chrome/browser/an... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/364793005/diff/70001/chrome/browser/an... chrome/browser/android/tab_android.cc:543: bitmap = gfx::ConvertToJavaBitmap(&favicon); I think this is still incorrect. It looks like you're not accounting for dpi. We want 16dp not 16px. That's why you should use GetRepresentation. We don't want to scale the 16x16 favicon up to 48x48 on xxhdpi it would look awful.
@David, Please let me know weather this you want. or anything else which i missed.
https://chromiumcodereview.appspot.com/364793005/diff/90001/chrome/android/ja... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://chromiumcodereview.appspot.com/364793005/diff/90001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1021: if ((mFaviconUrl == null) || (!mFaviconUrl.equals(mContentViewCore.getUrl()))) { Could this change on a page refresh (even if the URL stays the same)? Also if nobody cares about the favicon we shouldn't bother building it. Maybe we should do something like: onFaviconUpdated: mFavicon = null; mFaviconUrl = null; getFavicon: if (mFavicon == null || !cvcurl.equals(faviconurl)) buildFavicon; return mFavicon; https://chromiumcodereview.appspot.com/364793005/diff/90001/chrome/browser/an... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/364793005/diff/90001/chrome/browser/an... chrome/browser/android/tab_android.cc:536: if (!favicon.isNull()) { If this is null use the default gfx::Image::AsBitmap() call. https://chromiumcodereview.appspot.com/364793005/diff/90001/chrome/browser/an... chrome/browser/android/tab_android.cc:538: favicon.height() != gfx::kFaviconSize) This scaling is still wrong it'll still scale to 16px not 16dp right? We need to make sure we're at 16dp.
@David, I couldn't found any API which is scale to 16dp. Can you please help me in this. Or else you want me to create a canvas with destination rect ( by calculating dip) and set the destination bitmap to canvas and paint using the src bitmap.
On 2014/07/23 12:33:08, Jitu wrote: > @David, > > I couldn't found any API which is scale to 16dp. > Can you please help me in this. > > Or else you want me to create a canvas with destination rect ( by calculating > dip) > and set the destination bitmap to canvas and paint using the src bitmap. You just need the density of the device so you can multiply it by 16. An example would be to use gfx::DeviceDisplayInfo().GetDIPScale() * 16. Worst case you could also pass the density down through your nativeGetFavicon() call, but using the DeviceDisplayInfo is probably better. Just check that it works: on mdpi devices GetDIPScale should be 1.0, on hdpi it should be 1.5, on xhdpi 2.0, and xxhdpi 3.0. Then your resize function can go from there.
PTAL... https://codereview.chromium.org/364793005/diff/90001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://codereview.chromium.org/364793005/diff/90001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1021: if ((mFaviconUrl == null) || (!mFaviconUrl.equals(mContentViewCore.getUrl()))) { Changes done as per suggestion https://codereview.chromium.org/364793005/diff/90001/chrome/browser/android/t... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/364793005/diff/90001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:536: if (!favicon.isNull()) { On 2014/07/21 18:18:17, David Trainor wrote: > If this is null use the default gfx::Image::AsBitmap() call. Done. https://codereview.chromium.org/364793005/diff/90001/chrome/browser/android/t... chrome/browser/android/tab_android.cc:538: favicon.height() != gfx::kFaviconSize) On 2014/07/21 18:18:17, David Trainor wrote: > This scaling is still wrong it'll still scale to 16px not 16dp right? We need > to make sure we're at 16dp. Done.
PTAL
https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:897: return null; put on line above or add {} (java style) https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/browser/a... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/browser/a... chrome/browser/android/tab_android.cc:537: if (!favicon.isNull()) { I think IsEmpty() might be better here Also, f favicon is empty we should fall back to icon.AsBitmap(). If one of those isn't empty then we should perform the appropriate scaling and return it... this will never fall back to a default favicon if we don't have the correct density. https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/browser/a... chrome/browser/android/tab_android.cc:542: favicon.height() != target_size_dip) Use {} this seems somewhat hard to read.
@David, Fixed the comments. I have one doubt in one comment. Please clarify Thanks https://codereview.chromium.org/364793005/diff/110001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://codereview.chromium.org/364793005/diff/110001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:897: return null; On 2014/07/28 17:50:04, David Trainor wrote: > put on line above or add {} (java style) Done. https://codereview.chromium.org/364793005/diff/110001/chrome/browser/android/... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/364793005/diff/110001/chrome/browser/android/... chrome/browser/android/tab_android.cc:537: if (!favicon.isNull()) { We can't do check IsEmpty() here as favicon is type SkBitmap. Sorry Are you talking about only IsEmpty() check or anything else which i can't interpreted. Or do you want i have to changes like gfx::Image favicon = favicon_tab_helper->GetFavicon().image; And then check if favicon is not empty then do the scaling. Please let me know in case any misunderstood. https://codereview.chromium.org/364793005/diff/110001/chrome/browser/android/... chrome/browser/android/tab_android.cc:542: favicon.height() != target_size_dip) On 2014/07/28 17:50:04, David Trainor wrote: > Use {} this seems somewhat hard to read. Done.
https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/browser/a... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/browser/a... chrome/browser/android/tab_android.cc:537: if (!favicon.isNull()) { Ah sorry! I meant SkBitmap.empty() not IsEmpty()... On 2014/07/30 11:33:56, Jitu wrote: > We can't do check IsEmpty() here as favicon is type SkBitmap. > > Sorry > Are you talking about only IsEmpty() check or anything else which i can't > interpreted. > > Or do you want i have to changes like > > gfx::Image favicon = favicon_tab_helper->GetFavicon().image; > > And then check if favicon is not empty then do the scaling. > > Please let me know in case any misunderstood. https://chromiumcodereview.appspot.com/364793005/diff/130001/chrome/browser/a... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/364793005/diff/130001/chrome/browser/a... chrome/browser/android/tab_android.cc:552: favicon = image.AsBitmap(); This is still an empty bitmap :(. We should be doing something like: 1. Try to grab correct scale from gfx::Image 2. If SkBitmap.empty(), grab the default (gfx::Image::AsBitmap()) 3. If the result of 1 & 2 is not empty, make sure 16dp 4. Convert to jobject
On 2014/07/30 17:51:24, David Trainor wrote: > https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/browser/a... > File chrome/browser/android/tab_android.cc (right): > > https://chromiumcodereview.appspot.com/364793005/diff/110001/chrome/browser/a... > chrome/browser/android/tab_android.cc:537: if (!favicon.isNull()) { > Ah sorry! I meant SkBitmap.empty() not IsEmpty()... > > On 2014/07/30 11:33:56, Jitu wrote: > > We can't do check IsEmpty() here as favicon is type SkBitmap. > > > > Sorry > > Are you talking about only IsEmpty() check or anything else which i can't > > interpreted. > > > > Or do you want i have to changes like > > > > gfx::Image favicon = favicon_tab_helper->GetFavicon().image; > > > > And then check if favicon is not empty then do the scaling. > > > > Please let me know in case any misunderstood. > > https://chromiumcodereview.appspot.com/364793005/diff/130001/chrome/browser/a... > File chrome/browser/android/tab_android.cc (right): > > https://chromiumcodereview.appspot.com/364793005/diff/130001/chrome/browser/a... > chrome/browser/android/tab_android.cc:552: favicon = image.AsBitmap(); > This is still an empty bitmap :(. We should be doing something like: > > 1. Try to grab correct scale from gfx::Image > 2. If SkBitmap.empty(), grab the default (gfx::Image::AsBitmap()) > 3. If the result of 1 & 2 is not empty, make sure 16dp > 4. Convert to jobject @David, Due to some prob in my machine. I will be pushing the changes tomorrow. Thanks
Changes done as per suggested. PTAL... https://codereview.chromium.org/364793005/diff/150001/chrome/browser/android/... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/364793005/diff/150001/chrome/browser/android/... chrome/browser/android/tab_android.cc:600: if (favicon.empty()) { If favicon is empty get the default favicon. https://codereview.chromium.org/364793005/diff/150001/chrome/browser/android/... chrome/browser/android/tab_android.cc:607: if (!favicon.empty()) { If both default and favicon is not empty then scale it.
https://chromiumcodereview.appspot.com/364793005/diff/150001/chrome/browser/a... File chrome/browser/android/tab_android.cc (right): https://chromiumcodereview.appspot.com/364793005/diff/150001/chrome/browser/a... chrome/browser/android/tab_android.cc:600: if (favicon.empty()) { On 2014/08/01 11:35:50, Jitu wrote: > If favicon is empty get the default favicon. Sorry for the communication issues. I meant, "Grab the default scale for the current favicon." I don't know if that actually does anything given the current gfx::Image code, but our internal code looks like it will grab the default bitmap (favicon_tab_helper->GetFavicon().AsBitmap()) if the above is empty.
PTAL
lgtm
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jitendra.ks@samsung.com/364793005/170001
Message was sent while issue was closed.
Change committed as 289275 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
