Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1343913002:
Introduce Animated Logo to Chrome on Android (Closed)
|
Created:
5 years, 3 months ago by Ian Wen Modified:
4 years, 8 months ago Reviewers:
sky, newt (away), sevenfold1999AC, Martin Barbella, sevenfold1999ac, klobag.chromium, justincohen, mmenke CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce Animated Logo to Chrome on Android
This CL enables chrome on Android to show GIF-powered doogle in NTP. To
accomplish this goal, this CL:
1. Ports GifPlayer library from googlesource to chromium third_party
folder.
2. Creates AnimatedLogoTracker to download the gif url specified by the
parsed logo.
3. Copies the downloaded gif as byte array to java side, and in NTP,
GifPlayer is used to decode the raw byte array and draw it as an Android
drawable.
BUG=414528
Committed: https://crrev.com/70232143bad79b5dcfe8c1c828e6bde7a4b72b70
Cr-Commit-Position: refs/heads/master@{#350907}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 59
Patch Set 3 : respond to newt's comments #
Total comments: 57
Patch Set 4 : lots of renamings #
Total comments: 3
Patch Set 5 : make findbug and license.py happy #Patch Set 6 : Fix GN #Patch Set 7 : #Depends on Patchset: Messages
Total messages: 60 (19 generated)
ianwen@chromium.org changed reviewers: + newt@chromium.org
ianwen@chromium.org changed reviewers: + mbarbella@chromium.org
Hi Martin, I wonder if you could do a security review for my third_party change? I was told to approach to an ISE in the email thread with chrome-security@. If you are not the right person for this type of work, could you add the right person as the reviewer? Thanks!
mbarbella@google.com changed reviewers: + mbarbella@google.com
Have you also reached out to open source for this? You'll need them to sign off as well. Most of the initial questions I have were already answered on the email thread you sent to chrome-security. Has anyone from ISE reviewed this code already? The owners of it would likely know. If so, that would make this pretty easy. If not, I can probably find another reviewer or take a look if not. https://codereview.chromium.org/1343913002/diff/1/third_party/gif_player/READ... File third_party/gif_player/README.chromium (right): https://codereview.chromium.org/1343913002/diff/1/third_party/gif_player/READ... third_party/gif_player/README.chromium:6: Security Critical: no Since this is shipping to users, this is security critical.
Thank you Martin for such quick response! I am not super familiar with our security review process. Could you please help me find an ISE to review it (I thought you were the ISE that Chris mentioned in the email thread)? Are there any other things I can do to expedite the process? https://codereview.chromium.org/1343913002/diff/1/third_party/gif_player/READ... File third_party/gif_player/README.chromium (right): https://codereview.chromium.org/1343913002/diff/1/third_party/gif_player/READ... third_party/gif_player/README.chromium:6: Security Critical: no On 2015/09/17 02:44:34, mbarbella1 wrote: > Since this is shipping to users, this is security critical. Done.
In terms of open sourcing, I talked to the owner of the library in Google3 and he said "it was okay". Also this library is _already_ open sourced, and distributed in APACHE v2. Anyone can see the library using this link: http://android-gifview.googlecode.com/svn/!svn/bc/8/trunk/GifPlayer/ All I did was to move it from googlecode.com to chromium.org. Is there an "open source" team in chromium that I need to add to the reviewer list?
On 2015/09/17 21:14:17, Ian Wen wrote: > In terms of open sourcing, I talked to the owner of the library in Google3 and > he said "it was okay". > > Also this library is _already_ open sourced, and distributed in APACHE v2. > Anyone can see the library using this link: > http://android-gifview.googlecode.com/svn/!svn/bc/8/trunk/GifPlayer/ All I did > was to move it from http://googlecode.com to http://chromium.org. > > Is there an "open source" team in chromium that I need to add to the reviewer > list? Send a separate email with a link to this CL to open-source-third-party-reviews@google.com (see https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review)
On 2015/09/17 21:10:55, Ian Wen wrote: > Thank you Martin for such quick response! > > I am not super familiar with our security review process. Could you please help > me find an ISE to review it (I thought you were the ISE that Chris mentioned in > the email thread)? I don't think I was specifically mentioned in the email thread, but I don't mind looking into it or finding someone else if it's necessary (I know nothing about java). In this context, I meant ISE team (a security team within google). If they've already looked at this code, I'd rather not duplicate the effort. Could you reach out to the owners of the code to see if it has been reviewed before? > > Are there any other things I can do to expedite the process? > > https://codereview.chromium.org/1343913002/diff/1/third_party/gif_player/READ... > File third_party/gif_player/README.chromium (right): > > https://codereview.chromium.org/1343913002/diff/1/third_party/gif_player/READ... > third_party/gif_player/README.chromium:6: Security Critical: no > On 2015/09/17 02:44:34, mbarbella1 wrote: > > Since this is shipping to users, this is security critical. > > Done.
mbarbella@chromium.org changed reviewers: - mbarbella@google.com
This actually seems fairly straightforward, so don't worry about trying to track down any previous reviews. LGTM from security for third_party/gif_player. You may still wish to fill out a security survey for the feature itself, as I only looked over the third party code (not usage). I wouldn't expect any trouble here, as most of the security team's questions about the feature itself have already been answered.
Ian, this is a great change overall! Thanks for putting this together :)
I have a million comments, of course, so... here goes:
---------------
What if the user taps the logo several times while the animated logo is being
downloaded? We need to handle this case better so that it doesn't result in
aborted and restarted downloads.
I'd delete animated_logo_tracker.cc and move the downloading logic straight into
logo_bridge.cc. Bouncing through those additional layers doesn't gain anything
AFAICT. Plus, the LogoBridge's lifetime and the URLFetcher's lifetime should be
tied together, so putting the downloading in logo_bridge.cc simplifies lifetime
management.
While I suggested that we shouldn't *optimize* for the case where multiple
animated logos are being downloaded simultaneously, we still need to support it
non-buggily. Moving the downloading logic into LogoBridge satisfies this, if we
add one additional constraint: only one outstanding call to
LogoBridge.getAnimatedLogo() is allowed (add this to the javadoc and add a
DCHECK for it in C++, and fix the current behavior which calls getAnimatedLogo()
every time the user taps on the LogoView).
Then, the C++ LogoBridge can look like this:
class LogoBridge : public net::URLFetcherDelegate {
...
scoped_ptr<URLFetcher> animated_logo_fetcher_;
ScopedJavaGlobalRef<jobject> animated_logo_callback_;
}
void LogoBridge::GetAnimatedLogo(...) {
DCHECK(!animated_logo_fetcher_)
...
// set animated_logo_fetcher_ to a new URLFetcher and start the download
}
void LogoBridge::OnURLFetchComplete(const net::URLFetcher* source) {
...
// At this point, you know that LogoBridge.onDestroy() hasn't been called,
// (otherwise this object would have been destructed so the URLFetcher would
// have been destructed, so OnURLFetchComplete wouldn't have been called)
// convert response to byte array and pass to animated_logo_callback_, which
// we also know is still valid.
}
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java
(right):
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:40:
public final String gifUrl;
animatedLogoUrl
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:70: }
else if (!image.sameAs(other.image)) {
Why sameAs()? This compares pixel config, but not the actual data. Would "==" be
acceptable in this case?
Even better would be to fix the underlying issue (multiple taps triggers
multiple animated logo downloads) so that we don't need to do equality
comparisons on logos.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:95:
interface GifListener {
Call this AnimatedLogoCallback or AnimatedLogoObserver. "Callback" is most
appropriate since whoever implements this is not merely "observing" that the
animated logo was downloaded, but is the sole actor that's handling the
post-download actions.
"Observer" or "Listener" would imply that some other code is handling the
completed download, and that we're just interested in some side action that
should also happen when the download is complete.
(Admittedly, LogoObserver doesn't exactly follow this pattern. It's a bit weird
because multiple requests are coalesced, so there could be multiple
LogoObservers that handle the downloading of a single logo.)
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:100: *
@param bytes The byte array representing the raw data for the GIF.
"animated GIF"
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:139: *
Asynchronously gets the GIF animation.
"Downloads an animated GIF logo."
(It's pretty obvious that downloading is asynchronous :)
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:143:
void getGif(GifListener listener, String gifUrl) {
"getAnimatedLogo"
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:154:
private native void nativeGetAnimatedLogo(long nativeLogoBridge, GifListener
listenr,
"listenr" typo, but it should be changed to "callback" anyway.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java
(right):
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:35: *
available.
update javadoc
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:46:
private Bitmap mLogo;
Be sure to set mLogo and mNewLogo to null once the animated gif is shown to
avoid holding on to unneeded image bytes
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:129: if
(mCurrentLogo != null && mCurrentLogo.equals(logo)) return;
Why is this needed? To prevent the non-animated logo from fading in over the
animated logo if logo revalidation happens after the animated doodle is already
playing?
If so, it would be nice to push this logic down towards the LogoBridge or the
LogoTracker. Also, this might indicate a bug if we're decoding the logo image
twice.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:185: void
updateGif(BaseGifImage gifImage) {
Make this public as with the method below. Let's try to keep this class's API
consistent, and it's currently a public API. (You could reasonably change the
class's visibility to package private, but technically, Android wants all View
subclasses to be public so they can be instantiated via reflection)
I'd probably move these two methods above the private updateLogo() method to
keep all the public methods together.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:195:
boolean isGifPlaying() {
What does this return after the gif stops animating? I would guess that
mGifDrawable.isRunning() returns false at that point, so this would also return
false... but that would cause broken behavior: onDraw() wouldn't draw the
animated gif, and NewTabPageManager.openLogoLink() would do the wrong thing.
Instead, I think this method should be called "isAnimatedLogoShowing()", and
should always return true after the animated logo starts playing.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:196:
return mGifDrawable == null ? false : mGifDrawable.isRunning() &&
mGifDrawable.isValid();
Simpler: "return mGifDrawable != null && mGifDrawable.isRunning() &&
mGifDrawable.isValid();"
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:230:
protected boolean verifyDrawable(Drawable who) {
Move this down next to invalidateDrawable(). In general, keep related overrides
together.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:247: *
@see ImageView#invalidateDrawable(Drawable)
Remove this javadoc comment and instead add an implementation comment inside the
method. I'd briefly explain why we need to call invalidate() and refer to
ImageView#invalidateDrawable(Drawable) for more information.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:258: //
Gif drawable is present, draw animation instead.
At this point we should set mLogo and mNewLogo to null and cancel mAnimation if
it happens to be running. In the normal case, mLogo will be the special logo,
and mNewLogo and mAnimation will be null, but this depends on code outside this
class, so I'd recommend handling the case where an animation is running rather
than assuming that it can't happen.
if (mAnimation != null) mAnimation.cancel();
if (mLogo != null) mLogo = null;
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:260:
canvas.concat(mLogoMatrix);
Rather than piggy backing on mLogoMatrix, I'd use a separate Matrix for the
animated logo. This would allow us to set mLogo to null and still get matrix
updates when the user rotates their device (see onSizeChanged() below). This
also makes the animated logo a bit more independent, so that if it happens to
have a different size than the custom logo (which shouldn't happen, but might),
the animated logo will still be displayed centered and sized correctly.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:93:
private String mGifUrl;
Call this "mAnimatedLogoUrl". Yes, it's a gif, but that's not its most
descriptive aspect (it's sort of like naming a variable "integer". Yes, it's an
integer, *but* what's it used for?)
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:359: if
(mOnLogoClickUrl == null) return;
This check should be moved inside the if clause below. Otherwise, we'll never be
able to show animated logos if mOnLogoClickUrl is null.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:361: if
(TextUtils.isEmpty(mGifUrl) || mNewTabPageView.isGifPlaying()) {
I'd rename this method to onLogoClicked() since it has multiple behaviors now.
I'd also add a parameter "isAnimatedLogoShowing" instead of querying
mNewTabPageView.isGifPlaying().
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:366:
public void onGifDownloaded(byte[] bytes) {
add "if (mIsDestroyed) return;" or "assert !mIsDestroyed" is you're feeling
overly confident that this can't be called after onDestroy().
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:545:
void updateLogoGif(BaseGifImage gifImage) {
how about "playAnimatedLogo()"?
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:552:
boolean isGifPlaying() {
"isAnimatedLogoPlaying()"
https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/logo_bridge.h (right):
https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/...
chrome/browser/android/logo_bridge.h:31: LogoService* logo_service_;
why public?? member variables should almost never be public, except in data-only
types.
https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/...
chrome/browser/android/logo_bridge.h:36: JavaObjectWeakGlobalRef
j_listener_weak_ref;
Weak Java refs are a code smell, especially when used as listeners. If you
follow the advice in my overview comment, then you can change this to a
ScopedJavaGlobalRef<jobject>, which is much nicer.
https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/logo_service.cc (right):
https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/...
chrome/browser/android/logo_service.cc:129: true);
as a later step, we can remove this parameter from SetServerAPI since all
platforms now support animated logos :)
https://codereview.chromium.org/1343913002/diff/20001/components/search_provi...
File components/search_provider_logos/animated_logo_tracker.cc (right):
https://codereview.chromium.org/1343913002/diff/20001/components/search_provi...
components/search_provider_logos/animated_logo_tracker.cc:24: fetcher_ =
resetting fetcher_ will abort any currently in-progress download. probably not
what you want.
https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/...
File third_party/gif_player/BUILD.gn (right):
https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/...
third_party/gif_player/BUILD.gn:9: "//base:base_java",
Why do these depend on base_java? The gyp target doesn't depend on base_java.
https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/...
File third_party/gif_player/README.chromium (right):
https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/...
third_party/gif_player/README.chromium:1: Name: GifPlayer Library for Showing
Gif on Android
Is this an established name taken from somewhere else? If not how about
"GifPlayer Animated GIF Library" or simply "GifPlayer"? Everything in
third_party is a "library" of sorts, so it's fine to remove "library" from the
name.
https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/...
third_party/gif_player/README.chromium:10: The GifDrawable library is a partial
library built from the GifDecoder of
The first sentence should mention the purpose of this library (and use the right
name): "GifPlayer allows animated GIFs to be displayed in Android's native view
system."
Then explain where it's from.
https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java (right): https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:40: public final String gifUrl; On 2015/09/18 20:46:01, newt wrote: > animatedLogoUrl Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:70: } else if (!image.sameAs(other.image)) { On 2015/09/18 20:46:01, newt wrote: > Why sameAs()? This compares pixel config, but not the actual data. Would "==" be > acceptable in this case? > > Even better would be to fix the underlying issue (multiple taps triggers > multiple animated logo downloads) so that we don't need to do equality > comparisons on logos. Removed. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:95: interface GifListener { On 2015/09/18 20:46:01, newt wrote: > Call this AnimatedLogoCallback or AnimatedLogoObserver. "Callback" is most > appropriate since whoever implements this is not merely "observing" that the > animated logo was downloaded, but is the sole actor that's handling the > post-download actions. > > "Observer" or "Listener" would imply that some other code is handling the > completed download, and that we're just interested in some side action that > should also happen when the download is complete. > > (Admittedly, LogoObserver doesn't exactly follow this pattern. It's a bit weird > because multiple requests are coalesced, so there could be multiple > LogoObservers that handle the downloading of a single logo.) Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:100: * @param bytes The byte array representing the raw data for the GIF. On 2015/09/18 20:46:01, newt wrote: > "animated GIF" Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:139: * Asynchronously gets the GIF animation. On 2015/09/18 20:46:01, newt wrote: > "Downloads an animated GIF logo." > > (It's pretty obvious that downloading is asynchronous :) Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:143: void getGif(GifListener listener, String gifUrl) { On 2015/09/18 20:46:01, newt wrote: > "getAnimatedLogo" Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:154: private native void nativeGetAnimatedLogo(long nativeLogoBridge, GifListener listenr, On 2015/09/18 20:46:01, newt wrote: > "listenr" typo, but it should be changed to "callback" anyway. Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java (right): https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:35: * available. On 2015/09/18 20:46:02, newt wrote: > update javadoc Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:46: private Bitmap mLogo; On 2015/09/18 20:46:02, newt wrote: > Be sure to set mLogo and mNewLogo to null once the animated gif is shown to > avoid holding on to unneeded image bytes Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:129: if (mCurrentLogo != null && mCurrentLogo.equals(logo)) return; On 2015/09/18 20:46:02, newt wrote: > Why is this needed? To prevent the non-animated logo from fading in over the > animated logo if logo revalidation happens after the animated doodle is already > playing? > > If so, it would be nice to push this logic down towards the LogoBridge or the > LogoTracker. Also, this might indicate a bug if we're decoding the logo image > twice. Talked offline. This is trying to fix an imaginary issue that only happens with my fake server. Reverted this line because double-fading-in is unlikely to happen for real doodle server. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:185: void updateGif(BaseGifImage gifImage) { On 2015/09/18 20:46:02, newt wrote: > Make this public as with the method below. Let's try to keep this class's API > consistent, and it's currently a public API. (You could reasonably change the > class's visibility to package private, but technically, Android wants all View > subclasses to be public so they can be instantiated via reflection) > > I'd probably move these two methods above the private updateLogo() method to > keep all the public methods together. Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:195: boolean isGifPlaying() { On 2015/09/18 20:46:02, newt wrote: > What does this return after the gif stops animating? I would guess that > mGifDrawable.isRunning() returns false at that point, so this would also return > false... but that would cause broken behavior: onDraw() wouldn't draw the > animated gif, and NewTabPageManager.openLogoLink() would do the wrong thing. > > Instead, I think this method should be called "isAnimatedLogoShowing()", and > should always return true after the animated logo starts playing. BaseGifDrawable will always be running unless we call drawable#stop(). However I still think your naming makes more sense. Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:196: return mGifDrawable == null ? false : mGifDrawable.isRunning() && mGifDrawable.isValid(); On 2015/09/18 20:46:02, newt wrote: > Simpler: "return mGifDrawable != null && mGifDrawable.isRunning() && > mGifDrawable.isValid();" Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:230: protected boolean verifyDrawable(Drawable who) { On 2015/09/18 20:46:01, newt wrote: > Move this down next to invalidateDrawable(). In general, keep related overrides > together. Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:247: * @see ImageView#invalidateDrawable(Drawable) On 2015/09/18 20:46:02, newt wrote: > Remove this javadoc comment and instead add an implementation comment inside the > method. I'd briefly explain why we need to call invalidate() and refer to > ImageView#invalidateDrawable(Drawable) for more information. Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:258: // Gif drawable is present, draw animation instead. On 2015/09/18 20:46:02, newt wrote: > At this point we should set mLogo and mNewLogo to null and cancel mAnimation if > it happens to be running. In the normal case, mLogo will be the special logo, > and mNewLogo and mAnimation will be null, but this depends on code outside this > class, so I'd recommend handling the case where an animation is running rather > than assuming that it can't happen. > > if (mAnimation != null) mAnimation.cancel(); > if (mLogo != null) mLogo = null; > Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:260: canvas.concat(mLogoMatrix); On 2015/09/18 20:46:01, newt wrote: > Rather than piggy backing on mLogoMatrix, I'd use a separate Matrix for the > animated logo. This would allow us to set mLogo to null and still get matrix > updates when the user rotates their device (see onSizeChanged() below). This > also makes the animated logo a bit more independent, so that if it happens to > have a different size than the custom logo (which shouldn't happen, but might), > the animated logo will still be displayed centered and sized correctly. Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:93: private String mGifUrl; On 2015/09/18 20:46:02, newt wrote: > Call this "mAnimatedLogoUrl". Yes, it's a gif, but that's not its most > descriptive aspect (it's sort of like naming a variable "integer". Yes, it's an > integer, *but* what's it used for?) Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:359: if (mOnLogoClickUrl == null) return; On 2015/09/18 20:46:02, newt wrote: > This check should be moved inside the if clause below. Otherwise, we'll never be > able to show animated logos if mOnLogoClickUrl is null. Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:361: if (TextUtils.isEmpty(mGifUrl) || mNewTabPageView.isGifPlaying()) { On 2015/09/18 20:46:02, newt wrote: > I'd rename this method to onLogoClicked() since it has multiple behaviors now. > I'd also add a parameter "isAnimatedLogoShowing" instead of querying > mNewTabPageView.isGifPlaying(). Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:366: public void onGifDownloaded(byte[] bytes) { On 2015/09/18 20:46:02, newt wrote: > add "if (mIsDestroyed) return;" or "assert !mIsDestroyed" is you're feeling > overly confident that this can't be called after onDestroy(). Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:545: void updateLogoGif(BaseGifImage gifImage) { On 2015/09/18 20:46:03, newt wrote: > how about "playAnimatedLogo()"? Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:552: boolean isGifPlaying() { On 2015/09/18 20:46:03, newt wrote: > "isAnimatedLogoPlaying()" Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/... chrome/browser/android/logo_bridge.h:31: LogoService* logo_service_; On 2015/09/18 20:46:03, newt wrote: > why public?? member variables should almost never be public, except in data-only > types. This was for testing and I forgot to clean it up. Reverted this line. https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/... chrome/browser/android/logo_bridge.h:36: JavaObjectWeakGlobalRef j_listener_weak_ref; On 2015/09/18 20:46:03, newt wrote: > Weak Java refs are a code smell, especially when used as listeners. If you > follow the advice in my overview comment, then you can change this to a > ScopedJavaGlobalRef<jobject>, which is much nicer. Done. https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/... File chrome/browser/android/logo_service.cc (right): https://codereview.chromium.org/1343913002/diff/20001/chrome/browser/android/... chrome/browser/android/logo_service.cc:129: true); On 2015/09/18 20:46:03, newt wrote: > as a later step, we can remove this parameter from SetServerAPI since all > platforms now support animated logos :) I added a todo to clean this up. https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/... File third_party/gif_player/BUILD.gn (right): https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/... third_party/gif_player/BUILD.gn:9: "//base:base_java", On 2015/09/18 20:46:03, newt wrote: > Why do these depend on base_java? The gyp target doesn't depend on base_java. Removed the dependency. https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/... File third_party/gif_player/README.chromium (right): https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/... third_party/gif_player/README.chromium:1: Name: GifPlayer Library for Showing Gif on Android On 2015/09/18 20:46:03, newt wrote: > Is this an established name taken from somewhere else? If not how about > "GifPlayer Animated GIF Library" or simply "GifPlayer"? Everything in > third_party is a "library" of sorts, so it's fine to remove "library" from the > name. Done. https://codereview.chromium.org/1343913002/diff/20001/third_party/gif_player/... third_party/gif_player/README.chromium:10: The GifDrawable library is a partial library built from the GifDecoder of On 2015/09/18 20:46:03, newt wrote: > The first sentence should mention the purpose of this library (and use the right > name): "GifPlayer allows animated GIFs to be displayed in Android's native view > system." > > Then explain where it's from. Done.
Nice. 100 lines fewer in the current patch set :) Basically looks good, just a bunch of nits. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:37: * The URL to download the animated GIF. If null, there is no GIF to download. "animated GIF logo" https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:39: public final String animagedLogoUrl; typo https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:41: Logo(Bitmap image, String onClickUrl, String altText, String gifUrl) { animatedLogoUrl https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:65: * A callback that is called when the GIF file is successfully downloaded. s/GIF file/animated logo https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:70: * Called when the animated GIF is successfully downloaded. Let's use "animated GIF logo" consistently in the Javadocs, and "animatedLogo" in the method/variable names (instead of a haphazard mix of "animated logo" "gif" "animated gif logo", etc). https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:75: void onGifDownloaded(byte[] bytes); "void onAnimatedLogoAvailable(byte[] imageData);" https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:111: * Downloads an animated GIF logo. Explain the subtleties about what happens if this method is called multiple times. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:116: nativeGetAnimatedLogo(mNativeLogoBridge, callback, gifUrl); nativeGetAnimatedLogo returns a boolean, but we drop the return value here. Why? https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:48: private ObjectAnimator mAnimation; It's probably good to rename this to mFadeAnimation, since there are now multiple "animations" going on. (Also update endFadingAnimation() to endFadeAnimation()) https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:50: private BaseGifDrawable mGifDrawable; I'd call this mAnimatedLogoDrawable for clarity and consistency. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:51: private Matrix mAnimatedLogoMatrix; nit: put this with the other Matrix objects https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:130: * Updates the GIF contained in this View and starts playing it. "animated GIF logo" https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:212: private void setMatrix(Bitmap image, Matrix matrix, boolean preventUpscaling) { you could just remove this method and update callers to use the new setMatrix() below https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:218: private void setMatrix(int imageWidth, int imageHeight, Matrix matrix, boolean preventUpscale) { s/preventUpscale/preventUpscaling/ preventUpscale doesn't make sense and having slightly different variable names that mean the same thing is just confusing. Also, wrapping is totally fine :) https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:251: // Drawable inside of the view does not have complete information about its boundary, making How about: "mGifDrawable doesn't actually know its bounds, so super.invalidateDrawable() doesn't invalidate the right area. Instead invalidate the entire view; the drawable takes up most of the view anyway so this is just as efficient. See ImageView..." https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:264: mLogo = mNewLogo = null; I'd avoid fancy tricks here. Use two separate statements. mLogo = null; mNewLogo = null; https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:293: if (mNewLogo != null) setMatrix(mNewLogo, mNewLogoMatrix, mNewLogoIsDefault); you need to update mAnimatedLogoMatrix here. Otherwise, it'll get wonky when you rotate the screen. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:361: && !TextUtils.isEmpty(mAnimatedLogoUrl); I'd prefer to continue using simple null checks, rather than TextUtils.isEmpty() here (and on line 370). TextUtils.isEmpty() is a bit viral: once you start using it, you have to use it everywhere. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:367: mNewTabPageView.playAnimatedLogo(new BaseGifImage(bytes)); Currently, we construct the BaseGifImage here, but the BaseGifDrawable inside NewTabPageView. Splitting this apart is a bit confusing. I'd do all the BaseGifImage/BaseGifDrawable work in the same place: either here or in NewTabPageView (or maybe LogoBridge). https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:546: void playAnimatedLogo(BaseGifImage gifImage) { Let's use the same name for this method as for the method that it calls in LogoView. They could both be playAnimatedLogo() or showAnimatedLogo(). https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... File chrome/browser/android/logo_bridge.cc (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.cc:107: ClearFetcher(); this isn't technically needed, but I think it's OK to keep for consistency https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.cc:127: jobject obj, indentation https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.cc:135: if (fetcher_ && fetcher_->GetOriginalURL() == url) { nit: don't use curly braces for single-line if statements in C++ https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.cc:153: std::string raw_response; Why "raw"? I'd probably just call this "response" to be consistent with URLFetcher's terminology (e.g. GetResponseAsString()) https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.h:31: jobject obj, indentation https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.h:46: // The URLFetcher currently fetching the logo. NULL when not fetching. s/logo/animated logo/ https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.h:49: // The URLRequestContextGetter used for network requests. "... used to download the animated logo." https://codereview.chromium.org/1343913002/diff/40001/components/search_provi... File components/search_provider_logos/logo_tracker.h (right): https://codereview.chromium.org/1343913002/diff/40001/components/search_provi... components/search_provider_logos/logo_tracker.h:75: // TODO(ianwen): remove wants_cta from parameter. I approve of this TODO, but it's in the wrong place. https://codereview.chromium.org/1343913002/diff/40001/third_party/gif_player/... File third_party/gif_player/README.chromium (right): https://codereview.chromium.org/1343913002/diff/40001/third_party/gif_player/... third_party/gif_player/README.chromium:2: Short Name: GifPlayer Library "GifPlayer". It's cleaner ;)
ianwen@chromium.org changed reviewers: + mmenke@chromium.org
A question for the network team: mmenke@, are we utilizing the net layer cache, if we download the gif file using UrlFetcher? https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:37: * The URL to download the animated GIF. If null, there is no GIF to download. On 2015/09/23 20:38:42, newt wrote: > "animated GIF logo" Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:39: public final String animagedLogoUrl; On 2015/09/23 20:38:43, newt wrote: > typo Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:41: Logo(Bitmap image, String onClickUrl, String altText, String gifUrl) { On 2015/09/23 20:38:42, newt wrote: > animatedLogoUrl Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:65: * A callback that is called when the GIF file is successfully downloaded. On 2015/09/23 20:38:43, newt wrote: > s/GIF file/animated logo Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:70: * Called when the animated GIF is successfully downloaded. On 2015/09/23 20:38:43, newt wrote: > Let's use "animated GIF logo" consistently in the Javadocs, and "animatedLogo" > in the method/variable names (instead of a haphazard mix of "animated logo" > "gif" "animated gif logo", etc). Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:75: void onGifDownloaded(byte[] bytes); On 2015/09/23 20:38:42, newt wrote: > "void onAnimatedLogoAvailable(byte[] imageData);" Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:111: * Downloads an animated GIF logo. On 2015/09/23 20:38:42, newt wrote: > Explain the subtleties about what happens if this method is called multiple > times. Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java:116: nativeGetAnimatedLogo(mNativeLogoBridge, callback, gifUrl); On 2015/09/23 20:38:43, newt wrote: > nativeGetAnimatedLogo returns a boolean, but we drop the return value here. Why? Removed. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:48: private ObjectAnimator mAnimation; On 2015/09/23 20:38:43, newt wrote: > It's probably good to rename this to mFadeAnimation, since there are now > multiple "animations" going on. (Also update endFadingAnimation() to > endFadeAnimation()) Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:50: private BaseGifDrawable mGifDrawable; On 2015/09/23 20:38:43, newt wrote: > I'd call this mAnimatedLogoDrawable for clarity and consistency. Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:51: private Matrix mAnimatedLogoMatrix; On 2015/09/23 20:38:43, newt wrote: > nit: put this with the other Matrix objects Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:130: * Updates the GIF contained in this View and starts playing it. On 2015/09/23 20:38:43, newt wrote: > "animated GIF logo" Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:212: private void setMatrix(Bitmap image, Matrix matrix, boolean preventUpscaling) { On 2015/09/23 20:38:43, newt wrote: > you could just remove this method and update callers to use the new setMatrix() > below Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:218: private void setMatrix(int imageWidth, int imageHeight, Matrix matrix, boolean preventUpscale) { On 2015/09/23 20:38:43, newt wrote: > s/preventUpscale/preventUpscaling/ > > preventUpscale doesn't make sense and having slightly different variable names > that mean the same thing is just confusing. Also, wrapping is totally fine :) Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:251: // Drawable inside of the view does not have complete information about its boundary, making On 2015/09/23 20:38:43, newt wrote: > How about: "mGifDrawable doesn't actually know its bounds, so > super.invalidateDrawable() doesn't invalidate the right area. Instead invalidate > the entire view; the drawable takes up most of the view anyway so this is just > as efficient. See ImageView..." Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:264: mLogo = mNewLogo = null; On 2015/09/23 20:38:43, newt wrote: > I'd avoid fancy tricks here. Use two separate statements. > > mLogo = null; > mNewLogo = null; Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:293: if (mNewLogo != null) setMatrix(mNewLogo, mNewLogoMatrix, mNewLogoIsDefault); On 2015/09/23 20:38:43, newt wrote: > you need to update mAnimatedLogoMatrix here. Otherwise, it'll get wonky when you > rotate the screen. Done locally already. https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:361: && !TextUtils.isEmpty(mAnimatedLogoUrl); On 2015/09/23 20:38:43, newt wrote: > I'd prefer to continue using simple null checks, rather than TextUtils.isEmpty() > here (and on line 370). TextUtils.isEmpty() is a bit viral: once you start using > it, you have to use it everywhere. (≧σ≦) https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:367: mNewTabPageView.playAnimatedLogo(new BaseGifImage(bytes)); On 2015/09/23 20:38:43, newt wrote: > Currently, we construct the BaseGifImage here, but the BaseGifDrawable inside > NewTabPageView. Splitting this apart is a bit confusing. I'd do all the > BaseGifImage/BaseGifDrawable work in the same place: either here or in > NewTabPageView (or maybe LogoBridge). Done. Moved it to bridge. I dislike passing bytes to a view... https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:546: void playAnimatedLogo(BaseGifImage gifImage) { On 2015/09/23 20:38:43, newt wrote: > Let's use the same name for this method as for the method that it calls in > LogoView. They could both be playAnimatedLogo() or showAnimatedLogo(). Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... File chrome/browser/android/logo_bridge.cc (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.cc:127: jobject obj, On 2015/09/23 20:38:43, newt wrote: > indentation Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.cc:135: if (fetcher_ && fetcher_->GetOriginalURL() == url) { On 2015/09/23 20:38:43, newt wrote: > nit: don't use curly braces for single-line if statements in C++ Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.cc:153: std::string raw_response; On 2015/09/23 20:38:43, newt wrote: > Why "raw"? I'd probably just call this "response" to be consistent with > URLFetcher's terminology (e.g. GetResponseAsString()) Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... File chrome/browser/android/logo_bridge.h (right): https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.h:31: jobject obj, On 2015/09/23 20:38:44, newt wrote: > indentation Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.h:46: // The URLFetcher currently fetching the logo. NULL when not fetching. On 2015/09/23 20:38:44, newt wrote: > s/logo/animated logo/ Done. https://codereview.chromium.org/1343913002/diff/40001/chrome/browser/android/... chrome/browser/android/logo_bridge.h:49: // The URLRequestContextGetter used for network requests. On 2015/09/23 20:38:43, newt wrote: > "... used to download the animated logo." Done. https://codereview.chromium.org/1343913002/diff/40001/components/search_provi... File components/search_provider_logos/logo_tracker.h (right): https://codereview.chromium.org/1343913002/diff/40001/components/search_provi... components/search_provider_logos/logo_tracker.h:75: // TODO(ianwen): remove wants_cta from parameter. On 2015/09/23 20:38:44, newt wrote: > I approve of this TODO, but it's in the wrong place. Done. https://codereview.chromium.org/1343913002/diff/40001/third_party/gif_player/... File third_party/gif_player/README.chromium (right): https://codereview.chromium.org/1343913002/diff/40001/third_party/gif_player/... third_party/gif_player/README.chromium:2: Short Name: GifPlayer Library On 2015/09/23 20:38:44, newt wrote: > "GifPlayer". It's cleaner ;) Done.
On 2015/09/23 21:59:52, Ian Wen wrote: > A question for the network team: > > mmenke@, are we utilizing the net layer cache, if we download the gif file using > UrlFetcher? As long as you're using the Profile's main URLRequestContext you are. The system URLRequestContext doesn't have a cache.
On 2015/09/23 22:01:26, mmenke wrote: > On 2015/09/23 21:59:52, Ian Wen wrote: > > A question for the network team: > > > > mmenke@, are we utilizing the net layer cache, if we download the gif file > using > > UrlFetcher? > > As long as you're using the Profile's main URLRequestContext you are. The > system URLRequestContext doesn't have a cache. (And yes, looks like you're using the right context)
lgtm! Future work: - To loop or not to loop - Progress spinner when loading takes a while - Ensure animation stops when LogoView is detached from window https://codereview.chromium.org/1343913002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java (right): https://codereview.chromium.org/1343913002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:246: // mGifDrawable doesn't actually know its bounds, so super.invalidateDrawable() doesn't "mAnimatedLogoDrawable"
ianwen@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, Could you please review the change in third_party folder? More context for you: Gifplayer is a simple java library that display GIFs using Android native views. It is a piece of open source code that is already open sourced and can be found http://android-gifview.googlecode.com/svn/!svn/bc/8/trunk/ on internet. This particular CL has: 1. passed chrome security review. 2. passed Google ISE review. (https://groups.google.com/a/google.com/forum/#!msg/ise-tps/AZR3D3v8OlY/iQBMm6...) 3. already been opensourced with Apache v2, so there is no open sourcing issue as well. More details can be found in design doc: https://docs.google.com/a/google.com/document/d/1aundmTW7jhE6y6H9GYSLUnCDMPaJ...
justincohen@chromium.org changed reviewers: + justincohen@chromium.org
https://codereview.chromium.org/1343913002/diff/60001/components/search_provi... File components/search_provider_logos/logo_tracker.h (right): https://codereview.chromium.org/1343913002/diff/60001/components/search_provi... components/search_provider_logos/logo_tracker.h:123: // TODO(ianwen): remove wants_cta from parameter. Does this mean GoogleAppendQueryparamsToLogoURL will remove wants_cta and default the functionality as if wants_cta == true?
On 2015/09/24 20:14:00, Ian Wen wrote: > Hi Scott, > > Could you please review the change in third_party folder? More context for you: > > Gifplayer is a simple java library that display GIFs using Android native views. > It is a piece of open source code that is already open sourced and can be found > http://android-gifview.googlecode.com/svn/!svn/bc/8/trunk/ on internet. > > This particular CL has: > 1. passed chrome security review. Quick clarification: my review wasn't a security review for the CL, but rather for the addition of this library to third_party. You should still fill out a security survey for the feature itself, as this is how security reviews are tracked (see https://chrome-security-survey.appspot.com/). > 2. passed Google ISE review. > (https://groups.google.com/a/google.com/forum/#!msg/ise-tps/AZR3D3v8OlY/iQBMm6...) > 3. already been opensourced with Apache v2, so there is no open sourcing issue > as well. > > More details can be found in design doc: > https://docs.google.com/a/google.com/document/d/1aundmTW7jhE6y6H9GYSLUnCDMPaJ...
https://codereview.chromium.org/1343913002/diff/60001/components/search_provi... File components/search_provider_logos/logo_tracker.h (right): https://codereview.chromium.org/1343913002/diff/60001/components/search_provi... components/search_provider_logos/logo_tracker.h:123: // TODO(ianwen): remove wants_cta from parameter. On 2015/09/24 20:20:48, justincohen wrote: > Does this mean GoogleAppendQueryparamsToLogoURL will remove wants_cta and > default the functionality as if wants_cta == true? Yes, that's the plan. Sound good?
After an offline talk with Martin, we agreed this is a tiny feature and all questions from the security team has already been answered in this email thread 2 weeks ago (search animiated doodle in g/chrome-security to see). A security survey has been created under the name of "animated logo", but since this feature is too tiny and it won't be discussed in launch review anyway. Scott: Please take a look. :)
sky@chromium.org changed reviewers: + klobag@chromium.org - sky@chromium.org
Grace is an owner of third_party and better to review this code anyway. So, sky->klobag . If you need to review chrome changes, feel free to add me back.
On 2015/09/24 23:56:17, sky wrote: > Grace is an owner of third_party and better to review this code anyway. So, > sky->klobag . If you need to review chrome changes, feel free to add me back. As you have got the security blessing, the code is already open source, I am OK to include it in the third party.
lgtm
lgtm
ianwen@chromium.org changed reviewers: + sky@chromium.org
Scott, could you take a look at the one line change in chrom.gyp? Thank you! :)
The CQ bit was checked by ianwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1343913002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1343913002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Don't you need to update DEPS?
The CQ bit was checked by ianwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1343913002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1343913002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
Since this is too simple we did not set up a repository for gif_player, instead it is checked out with third_party folder in trunk. We do similar things to several other folders in third_party as well, such as sqlite, jinja2, fuzzymatch, ashmem, etc.
My question wasn't about DEPS in the root, rather DEPS in the places that are including the new gif_player, eg chrome/android/java/src/org/chromium/chrome/browser/ntp. But maybe DEPS doesn't work for java files, so it doesn't matter? -Scott On Fri, Sep 25, 2015 at 12:12 AM, <ianwen@chromium.org> wrote: > Since this is too simple we did not set up a repository for gif_player, > instead > it is checked out with third_party folder in trunk. > > We do similar things to several other folders in third_party as well, such > as > sqlite, jinja2, fuzzymatch, ashmem, etc. > > https://codereview.chromium.org/1343913002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/09/25 16:07:11, sky wrote: > My question wasn't about DEPS in the root, rather DEPS in the places > that are including the new gif_player, eg > chrome/android/java/src/org/chromium/chrome/browser/ntp. But maybe > DEPS doesn't work for java files, so it doesn't matter? chrome/android/java/DEPS does exist and should be updated. I'm not sure if DEPS is actually enforced for java files though.
LGTM
The CQ bit was checked by ianwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mbarbella@chromium.org, newt@chromium.org, klobag@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1343913002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1343913002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1343913002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1343913002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1343913002/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/70232143bad79b5dcfe8c1c828e6bde7a4b72b70 Cr-Commit-Position: refs/heads/master@{#350907}
Message was sent while issue was closed.
sevenfold1999ac@gmail.com changed reviewers: + sevenfold1999AC@gmail.com
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
sevenfold1999ac@gmail.com changed reviewers: + sevenfold1999ac@gmail.com
Message was sent while issue was closed.
lgtm |
