Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1239923002:
webapps: add splashscreen for installed webapps (Closed)
|
Created:
5 years, 5 months ago by Lalit Maganti Modified:
5 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@manifest-title Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionwebapps: add splashscreen for installed webapps
This is the pinacle of splashscreen: it builds upon the background
color patches, the WebappDataStorage and IconDownload patches and
brings them together to display a splashscreen prior to webapps
loading.
This is a part of a group of CLs:
(1) https://codereview.chromium.org/1286973003
(2) https://codereview.chromium.org/1310223002
(3) https://codereview.chromium.org/1239923002 (this)
BUG=508627
Committed: https://crrev.com/1be62c2b7f12a4a8108d3b23b479059c706ffc09
Cr-Commit-Position: refs/heads/master@{#347625}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Update for storage class and add dynamic text color selection #Patch Set 3 : Refactor code to be less ugly #
Total comments: 1
Patch Set 4 : Just a rebase #Patch Set 5 : Fix compile and remove scaling #
Total comments: 16
Patch Set 6 : Address review comments #Patch Set 7 : Fix missed comment #
Total comments: 2
Patch Set 8 : Address review comments #Patch Set 9 : Fix test failures #Patch Set 10 : Actually fix failures #Patch Set 11 : Another test issue fix #Patch Set 12 : Fix test compile #
Messages
Total messages: 45 (8 generated)
lalitm@google.com changed reviewers: + mlamouri@chromium.org
mlamouri@chromium.org changed reviewers: + dfalcantara@chromium.org
Lalit, I assume this is more a FYI CL than something that needs to be reviewed given that we have no support for the splashscreen icon. I've added Dan as a reviewer given that he can give way better feedback than me on this.
Yes this is most certainly a WIP CL - I just threw it up here so I wouldn't lost it if I had to nuke my local chrome install for some reason. Dan: I'm not too happy about the windowDisablePreview flag hack but it's the best I could come up with to disable the white flash when the app starts up. A suggestion on how to do splashscreen without that hack would be awesome :)
Yes this is most certainly a WIP CL - I just threw it up here so I wouldn't lost it if I had to nuke my local chrome install for some reason. Dan: I'm not too happy about the windowDisablePreview flag hack but it's the best I could come up with to disable the white flash when the app starts up. A suggestion on how to do splashscreen without that hack would be awesome :)
dfalcantara@chromium.org changed reviewers: + dtrainor@chromium.org, newt@chromium.org
I'm not familiar enough with that flag hack; adding newt@ in case he knows what it does and dtrainor@ for comments on the white background thing. https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:163: // Create the splash screen all of these comments should have periods: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio...
windowDisablePreview seems like the correct tool to use here. Does it cause any problems? http://developer.android.com/reference/android/R.attr.html#windowDisablePreview https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... File chrome/android/java/res/values-v17/styles.xml (right): https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... chrome/android/java/res/values-v17/styles.xml:41: <style name="WebApp" parent="MainTheme"> nit: call this WebAppTheme to distinguish it from all the "styles" in this file. (theme != style, but they are declared in the same way :/ )
https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:272: mSplashScreen.setVisibility(View.GONE); Remove the view from the view hierarchy and set mSplashScreen to null. Don't just set its visibility to GONE, or it will continue to take up RAM and slow down the UI.
https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... File chrome/android/java/res/values-v17/styles.xml (right): https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... chrome/android/java/res/values-v17/styles.xml:42: <item name="android:windowDisablePreview">true</item> Should we be setting the window background color here? Could we set that to the light background color to hide the flash instead of the windowDisablePreview? https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (left): https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:97: removeWindowBackground(); Does not clearing this cause overdraw issues?
https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (left): https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:97: removeWindowBackground(); On 2015/07/24 20:39:19, David Trainor wrote: > Does not clearing this cause overdraw issues? +1. Since the window background isn't needed anymore (not even during activity launch), I'd recommend adding windowBackground="@null" to the WebAppTheme. Then you can safely remove this line. You can check for overdraw in Android Settings > Developer options > Debug GPU overdraw. Areas that are drawn once will be the normal color; twice -> blue; three times -> green; four or more times -> red.
https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://chromiumcodereview.appspot.com/1239923002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:167: // Create the view to hold the icon It would be much more readable to define this layout in a layout xml file, e.g. splash_screen.xml, rather than programmatically. Here, all you'd do is: ViewGroup splashScreen = LayoutInflater.from(context).inflate(R.layout.splash_screen, contentView, false); ImageView iconView = (ImageView) splashScreen.findViewById(R.id.web_app_icon); iconView.setImageBitmap(mWebappInfo.icon()); TextView appNameView = (TextView) splashScreen.findViewById(R.id.web_app_name); appNameView.setText(mWebappInfo.name()); contentView.addView(splashScreen);
Apologies for not getting back to you all sooner - I was OOO at an intern summit. Thank you for all the comments - I will attempt to fix them soon and get up an updated patch. https://codereview.chromium.org/1239923002/diff/1/chrome/android/java/res/val... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/1239923002/diff/1/chrome/android/java/res/val... chrome/android/java/res/values-v17/styles.xml:42: <item name="android:windowDisablePreview">true</item> On 2015/07/24 at 20:39:19, David Trainor wrote: > Should we be setting the window background color here? Could we set that to the light background color to hide the flash instead of the windowDisablePreview? Currently we could indeed do this. However, the idea is that in the future, the website would specify the color in its manifest at which point we couldn't hardcode the color in the XML. https://codereview.chromium.org/1239923002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (left): https://codereview.chromium.org/1239923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:97: removeWindowBackground(); On 2015/07/24 at 21:06:05, newt wrote: > On 2015/07/24 20:39:19, David Trainor wrote: > > Does not clearing this cause overdraw issues? > > +1. Since the window background isn't needed anymore (not even during activity launch), I'd recommend adding windowBackground="@null" to the WebAppTheme. Then you can safely remove this line. > > You can check for overdraw in Android Settings > Developer options > Debug GPU overdraw. Areas that are drawn once will be the normal color; twice -> blue; three times -> green; four or more times -> red. Yes it does indeed cause overdraw. I will attempt to work out a flow which gets rid of this. https://codereview.chromium.org/1239923002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1239923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:167: // Create the view to hold the icon On 2015/07/24 at 21:20:57, newt wrote: > It would be much more readable to define this layout in a layout xml file, e.g. splash_screen.xml, rather than programmatically. > > Here, all you'd do is: > > ViewGroup splashScreen = LayoutInflater.from(context).inflate(R.layout.splash_screen, contentView, false); > > ImageView iconView = (ImageView) splashScreen.findViewById(R.id.web_app_icon); > iconView.setImageBitmap(mWebappInfo.icon()); > > TextView appNameView = (TextView) splashScreen.findViewById(R.id.web_app_name); > appNameView.setText(mWebappInfo.name()); > > contentView.addView(splashScreen); Agreed. Since this is a WIP CL I threw everything into Java but was planning to move it into XML in the future. https://codereview.chromium.org/1239923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:272: mSplashScreen.setVisibility(View.GONE); On 2015/07/24 at 19:24:50, newt wrote: > Remove the view from the view hierarchy and set mSplashScreen to null. Don't just set its visibility to GONE, or it will continue to take up RAM and slow down the UI. Huh I never thought of that - thanks for pointing that out :)
Lalit, how much of this CL do you want to keep? Would it make sense to update it or maybe close it?
On 2015/08/24 09:54:05, Mounir Lamouri wrote: > Lalit, how much of this CL do you want to keep? Would it make sense to update it > or maybe close it? Most of the Java code was included the in BG color patch so is now redundant. Some of the XML is still relevant though so I'll probably update it rather than creating a new one. I need to make this use the webapp storage class as well.
Although the code looks ugly ATM (I'll fix this by refactoring into a different method soon), functionally this now complete.
On 2015/08/25 16:08:01, Lalit Maganti wrote: > Although the code looks ugly ATM (I'll fix this by refactoring into a different > method soon), functionally this now complete. Are we supposed to be reviewing this now or afterward?
On 2015/08/25 16:12:17, dfalcantara wrote: > On 2015/08/25 16:08:01, Lalit Maganti wrote: > > Although the code looks ugly ATM (I'll fix this by refactoring into a > different > > method soon), functionally this now complete. > > Are we supposed to be reviewing this now or afterward? Just refactored so should be good for review now :)
Because this is dependent on CLs that haven't landed, you should link to all of them in the CL description. Dave was confused about why he couldn't find the WebappDataStorage class anywhere.
https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:179: private void addSplashscreen() { Does it make sense to just add the splash screen at an earlier stage than this? Do we need to wait for postInflationStartup? Also, just curious, do you see any flickers before we set the background color (even if we're just setting it to webapp_default_bg)?
On 2015/08/26 at 01:09:45, dfalcantara wrote: > Because this is dependent on CLs that haven't landed, you should link to all of them in the CL description. Dave was confused about why he couldn't find the WebappDataStorage class anywhere. Have added to all three in series. On 2015/08/26 at 05:56:27, dtrainor wrote: > https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... > File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): > > https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:179: private void addSplashscreen() { > Does it make sense to just add the splash screen at an earlier stage than this? Do we need to wait for postInflationStartup? > > Also, just curious, do you see any flickers before we set the background color (even if we're just setting it to webapp_default_bg)? Yes you're right there is a flicker on Nexus 6 running M if the webapp is resumed from the background (i.e. the activity is killed but the Chrome process/library is still loaded) It makes a lot of sense to add it earlier than this but I wasn't sure if that would end up with the splashscreen behind the browser views rather than infront.
On 2015/08/26 13:16:30, Lalit Maganti wrote: > On 2015/08/26 at 01:09:45, dfalcantara wrote: > > Because this is dependent on CLs that haven't landed, you should link to all > of them in the CL description. Dave was confused about why he couldn't find the > WebappDataStorage class anywhere. > > Have added to all three in series. > > On 2015/08/26 at 05:56:27, dtrainor wrote: > > > https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... > > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java > (right): > > > > > https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:179: > private void addSplashscreen() { > > Does it make sense to just add the splash screen at an earlier stage than > this? Do we need to wait for postInflationStartup? > > > > Also, just curious, do you see any flickers before we set the background color > (even if we're just setting it to webapp_default_bg)? > > Yes you're right there is a flicker on Nexus 6 running M if the webapp is > resumed from the background (i.e. the activity is killed but the Chrome > process/library is still loaded) > > It makes a lot of sense to add it earlier than this but I wasn't sure if that > would end up with the splashscreen behind the browser views rather than infront. Could you override setContentView() and add the view after calling super.setContentView()?
On 2015/08/26 at 20:27:40, dtrainor wrote: > On 2015/08/26 13:16:30, Lalit Maganti wrote: > > On 2015/08/26 at 01:09:45, dfalcantara wrote: > > > Because this is dependent on CLs that haven't landed, you should link to all > > of them in the CL description. Dave was confused about why he couldn't find the > > WebappDataStorage class anywhere. > > > > Have added to all three in series. > > > > On 2015/08/26 at 05:56:27, dtrainor wrote: > > > > > https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... > > > File > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java > > (right): > > > > > > > > https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:179: > > private void addSplashscreen() { > > > Does it make sense to just add the splash screen at an earlier stage than > > this? Do we need to wait for postInflationStartup? > > > > > > Also, just curious, do you see any flickers before we set the background color > > (even if we're just setting it to webapp_default_bg)? > > > > Yes you're right there is a flicker on Nexus 6 running M if the webapp is > > resumed from the background (i.e. the activity is killed but the Chrome > > process/library is still loaded) > > > > It makes a lot of sense to add it earlier than this but I wasn't sure if that > > would end up with the splashscreen behind the browser views rather than infront. > > Could you override setContentView() and add the view after calling super.setContentView()? Yes if I could do that I definitely would but in ChromeActivity, setContentView() is declared as being final. I'm not sure if I would be OK to change that given how widely ChromeActivity is used.
On 2015/08/26 20:34:15, Lalit Maganti wrote: > On 2015/08/26 at 20:27:40, dtrainor wrote: > > On 2015/08/26 13:16:30, Lalit Maganti wrote: > > > On 2015/08/26 at 01:09:45, dfalcantara wrote: > > > > Because this is dependent on CLs that haven't landed, you should link to > all > > > of them in the CL description. Dave was confused about why he couldn't find > the > > > WebappDataStorage class anywhere. > > > > > > Have added to all three in series. > > > > > > On 2015/08/26 at 05:56:27, dtrainor wrote: > > > > > > > > https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... > > > > File > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java > > > (right): > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1239923002/diff/40001/chrome/android/j... > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:179: > > > private void addSplashscreen() { > > > > Does it make sense to just add the splash screen at an earlier stage than > > > this? Do we need to wait for postInflationStartup? > > > > > > > > Also, just curious, do you see any flickers before we set the background > color > > > (even if we're just setting it to webapp_default_bg)? > > > > > > Yes you're right there is a flicker on Nexus 6 running M if the webapp is > > > resumed from the background (i.e. the activity is killed but the Chrome > > > process/library is still loaded) > > > > > > It makes a lot of sense to add it earlier than this but I wasn't sure if > that > > > would end up with the splashscreen behind the browser views rather than > infront. > > > > Could you override setContentView() and add the view after calling > super.setContentView()? > > Yes if I could do that I definitely would but in ChromeActivity, > setContentView() is declared as being final. I'm not sure if I would be OK to > change that given how widely ChromeActivity is used. Huh I missed that. Oddly the comment talks about requiring a call to super.setContentView(). Can you trace through that call chain to find a good hook right after that point? Ideally you could try putting it in preInflate instead of post :).
David: yeah the comment in ChromeActivity is definitely confusing. I'm guessing someone added the final keyword and then forgot to get rid of the comment. Also looks like setContentView and onPostInflate are called immediately after each other on the UI thread but the native library async load is kicked off inbetween. I guess this is causing the flicker in some way. Unfortunately, preInflate would not work because the view would end up behind the inflated ones. However, I'm investigating playing around with transparent values and seeing where I get with the flicker.
On 2015/08/28 13:42:01, Lalit Maganti wrote: > David: yeah the comment in ChromeActivity is definitely confusing. I'm guessing > someone added the final keyword and then forgot to get rid of the comment. > > Also looks like setContentView and onPostInflate are called immediately after > each other on the UI thread but the native library async load is kicked off > inbetween. I guess this is causing the flicker in some way. > > Unfortunately, preInflate would not work because the view would end up behind > the inflated ones. However, I'm investigating playing around with transparent > values and seeing where I get with the flicker. Yeah sadly it sounds like there's no good way to do this cleanly. Ideally we could be setting our content view splash screen immediately, then when we get our proper hook to add real content we just insert it where we want in the view hierarchy. If that investigation/change is outside the scope of this that's probably okay... We can finish up and land something like this first, then investigate the fixes required to make it work without any flickers/flashes.
On 2015/08/28 20:09:42, David Trainor (OOO) wrote: > On 2015/08/28 13:42:01, Lalit Maganti wrote: > > David: yeah the comment in ChromeActivity is definitely confusing. I'm > guessing > > someone added the final keyword and then forgot to get rid of the comment. > > > > Also looks like setContentView and onPostInflate are called immediately after > > each other on the UI thread but the native library async load is kicked off > > inbetween. I guess this is causing the flicker in some way. > > > > Unfortunately, preInflate would not work because the view would end up behind > > the inflated ones. However, I'm investigating playing around with transparent > > values and seeing where I get with the flicker. > > Yeah sadly it sounds like there's no good way to do this cleanly. Ideally we > could be setting our content view splash screen immediately, then when we get > our proper hook to add real content we just insert it where we want in the view > hierarchy. If that investigation/change is outside the scope of this that's > probably okay... We can finish up and land something like this first, then > investigate the fixes required to make it work without any flickers/flashes. I'm out of the office for the next week and a half. Dan can you take over reviewing this? Sorry!
Dan: in which case if you could review this patch so we can get it landed soon, that would be awesome.
On 2015/09/04 18:08:36, Lalit Maganti wrote: > Dan: in which case if you could review this patch so we can get it landed soon, > that would be awesome. newt@: Dan said you had comments too so please take a look too.
lgtm to land so that it's easier to iterate on. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/webapp_splashscreen.xml (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/webapp_splashscreen.xml:24: android:textAppearance="@style/WebAppSplashScreenTextAppearanceTitle" nit: group the layout_ attributes together instead of putting textAppearance in the middle https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:181: private void addSplashscreen() { nit: initializeSplashscreen? You're doing more than just adding it to the hierarchy. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:206: private void addSplashscreenIconAndText(Bitmap splashIcon, int backgroundColor) { nit: setSplashscreenIconAndText?
https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/webapp_splashscreen.xml (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/webapp_splashscreen.xml:24: android:textAppearance="@style/WebAppSplashScreenTextAppearanceTitle" Since you're only using this style in one place, I wouldn't use a style at all. Just add android:fontFamily, android:textSize, and android:textColor attributes directly to this TextView. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... chrome/android/java/res/values/colors.xml:139: <color name="webapp_splash_title">#333333</color> Reuse existing colors where possible. e.g. use default_text_color instead of adding webapp_splash_title. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:199: <dimen name="webapp_splash_title_bottom_margin">64dp</dimen> Likewise, I'd just inline these values rather than defining them in dimens.xml. Since they're not going to be used anywhere else, putting them here just adds unnecessary indirection. (i.e. it makes the code harder to read, and provides no benefit) https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:181: private void addSplashscreen() { On 2015/09/04 18:25:56, dfalcantara wrote: > nit: initializeSplashscreen? You're doing more than just adding it to the > hierarchy. Be consistent with whether you treat splash screen as one or two words. Is it "mSplashScreen" or "mSplashscreen"? "splash_screen.xml" or splashscreen.xml"? I'd vote for the former options since splash screen is two separate words. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:194: contentView.addView(mSplashScreen); Where / when does the splash screen get removed from the content view? https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:207: TextView appNameView = (TextView) mSplashScreen.findViewById(R.id Don't wrap lines in the middle of a constant (R.id.webapp_splashscreen_name); that defeats grep. Wrap before "R". Likewise on the next line and on line 215.
I rebased on the latest since there have been some changes to WebappActivity. Changes which aren't mine are pretty clear though. newt@: PTAL https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/webapp_splashscreen.xml (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/webapp_splashscreen.xml:24: android:textAppearance="@style/WebAppSplashScreenTextAppearanceTitle" On 2015/09/04 18:45:48, newt wrote: > Since you're only using this style in one place, I wouldn't use a style at all. > Just add android:fontFamily, android:textSize, and android:textColor attributes > directly to this TextView. Done both. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... chrome/android/java/res/values/colors.xml:139: <color name="webapp_splash_title">#333333</color> On 2015/09/04 18:45:48, newt wrote: > Reuse existing colors where possible. e.g. use default_text_color instead of > adding webapp_splash_title. Done. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:199: <dimen name="webapp_splash_title_bottom_margin">64dp</dimen> On 2015/09/04 18:45:48, newt wrote: > Likewise, I'd just inline these values rather than defining them in dimens.xml. > Since they're not going to be used anywhere else, putting them here just adds > unnecessary indirection. (i.e. it makes the code harder to read, and provides no > benefit) Done. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:181: private void addSplashscreen() { On 2015/09/04 18:45:49, newt wrote: > On 2015/09/04 18:25:56, dfalcantara wrote: > > nit: initializeSplashscreen? You're doing more than just adding it to the > > hierarchy. > > Be consistent with whether you treat splash screen as one or two words. Is it > "mSplashScreen" or "mSplashscreen"? "splash_screen.xml" or splashscreen.xml"? > I'd vote for the former options since splash screen is two separate words. Done and gone with SplashScreen because it's what I did in earlier CLs as well. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:194: contentView.addView(mSplashScreen); On 2015/09/04 18:45:49, newt wrote: > Where / when does the splash screen get removed from the content view? On first paint. The code to do this is already present from my background CL change which landed earlier on. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:206: private void addSplashscreenIconAndText(Bitmap splashIcon, int backgroundColor) { On 2015/09/04 18:25:56, dfalcantara wrote: > nit: setSplashscreenIconAndText? Done. https://codereview.chromium.org/1239923002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:207: TextView appNameView = (TextView) mSplashScreen.findViewById(R.id On 2015/09/04 18:45:49, newt wrote: > Don't wrap lines in the middle of a constant (R.id.webapp_splashscreen_name); > that defeats grep. Wrap before "R". Likewise on the next line and on line 215. Done.
lgtm
lgtm with shortName changed to name. https://codereview.chromium.org/1239923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1239923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:212: appNameView.setText(mWebappInfo.shortName()); I'm pretty sure we wanted to show the name here, could you change to: appNameView.setText(mWebappInfo.name());
https://codereview.chromium.org/1239923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1239923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:212: appNameView.setText(mWebappInfo.shortName()); On 2015/09/05 15:05:05, Mounir Lamouri wrote: > I'm pretty sure we wanted to show the name here, could you change to: > appNameView.setText(mWebappInfo.name()); I misremembered - you're right. It's name for the splash screen and shortName for the launcher shortcut.
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, mlamouri@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1239923002/#ps140001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239923002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239923002/140001
The CQ bit was unchecked by lalitm@google.com
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, dfalcantara@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1239923002/#ps210001 (title: "Fix test compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239923002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239923002/210001
Message was sent while issue was closed.
Committed patchset #12 (id:210001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/1be62c2b7f12a4a8108d3b23b479059c706ffc09 Cr-Commit-Position: refs/heads/master@{#347625} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
