Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715433008:
🏡 Add a ChromeHomeNewTabPage (Closed)
Description[Home] Add a ChromeHomeNewTabPage
Adds a simple NTP that displays the search provider logo and a
close button for use in Chrome Home.
BUG=695973
Review-Url: https://codereview.chromium.org/2715433008
Cr-Commit-Position: refs/heads/master@{#453662}
Committed: https://chromium.googlesource.com/chromium/src/+/5c88a0f3700057690cd3e644e346e5fef84ae666
Patch Set 1 #Patch Set 2 : [Home] Add a ChromeHomeNewTabPage #Patch Set 3 : [Home] Add a ChromeHomeNewTabPage #Patch Set 4 : Rebase #
Total comments: 4
Patch Set 5 : Changes from mdjones@ review #
Total comments: 26
Patch Set 6 : Changes from dgn@/mvanouwerkerk@ review #Patch Set 7 : Add LogoDelegateImpl #
Total comments: 6
Patch Set 8 : Nits #
Total comments: 2
Patch Set 9 : Cancel inprogress animation when FBV disabled #Messages
Total messages: 55 (42 generated)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Home] Add a ChromeHomeNewTabPage Adds a simple NTP that displays the search provider logo and a close button for use in Chrome Home. BUG=695973 ========== to ========== [Home] Add a ChromeHomeNewTabPage Adds a simple NTP that displays the search provider logo and a close button for use in Chrome Home. BUG=695973 ==========
twellington@chromium.org changed reviewers: + mdjones@chromium.org
mdjones@ - will you please take a look from a Chrome Home perspective before I send this to the NTP folks?
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2715433008/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java (right): https://codereview.chromium.org/2715433008/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:157: mTab.getActivity().getFadingBackgroundView().setAlpha(0.f); setViewAlpha please. That class should probably be backed by a view rather than extending one. https://codereview.chromium.org/2715433008/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java (right): https://codereview.chromium.org/2715433008/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java:42: if (tab.isIncognito()) { nit: else if?
twellington@chromium.org changed reviewers: + dgn@chromium.org
+dgn@ - ptal https://codereview.chromium.org/2715433008/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java (right): https://codereview.chromium.org/2715433008/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:157: mTab.getActivity().getFadingBackgroundView().setAlpha(0.f); On 2017/02/27 19:50:25, mdjones wrote: > setViewAlpha please. That class should probably be backed by a view rather than > extending one. Done. https://codereview.chromium.org/2715433008/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java (right): https://codereview.chromium.org/2715433008/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java:42: if (tab.isIncognito()) { On 2017/02/27 19:50:25, mdjones wrote: > nit: else if? Done.
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mvanouwerkerk@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:36: private final LogoManagerImpl mLogoManager; I wanted to say: just use the interface type LogoManager, but destroy() is not in the base interface, so you need to type your variables to LogoManagerImpl. I guess that's ok for now. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:150: boolean newTabPageSelected = mTabModelSelector.getCurrentTab().getUrl().equals(getUrl()) This newTabPageSelected variable is unused... what is it for? Looks like a leftover that can be deleted. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:178: return tab.getUrl().equals(getUrl()) && !tab.isIncognito(); nit: use only one space after return https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoManagerImpl.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoManagerImpl.java:63: RecordHistogram.recordSparseSlowlyHistogram(LOGO_CLICK_UMA_NAME, CTA_IMAGE_CLICKED); I think we should separate old-style NTP histograms from Chrome Home histograms. If we don't, any changes will be difficult to attribute as NTP and Chrome Home might co-exist for some time while we ramp up usage of Chrome Home. What do you think? https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:93: public interface LogoManager { Can we rename this to Delegate and refer to LogoView.Delegate everywhere? I think "delegate" expresses what this does a bit more accurately than "manager" and we avoid the duplication in LogoView.LogoManager. It's what we've been moving to when breaking pieces out of the NewTabPageManager: https://cs.chromium.org/search/?q=f:ntp%7Csuggestions+interface.*Delegate+lan... https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:495: mSearchProviderLogoView.setMananger(manager); Hmn, maybe rename setMananger to setManager now that you're here :-)
Nice to see NTPManager getting dismantled further! https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/chrome_home_new_tab_page.xml (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/chrome_home_new_tab_page.xml:11: android:focusable="true" why make it focusable? is that related to accessibility? https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:32: public class ChromeHomeNewTabPage implements NativePage, TemplateUrlServiceObserver { is "Chrome" needed in the class name? https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:150: boolean newTabPageSelected = mTabModelSelector.getCurrentTab().getUrl().equals(getUrl()) On 2017/02/28 13:36:15, Michael van Ouwerkerk wrote: > This newTabPageSelected variable is unused... what is it for? Looks like a > leftover that can be deleted. Looks like it got replaced by the isTabChromeHomeNewTabPage() call on the line below :) https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:164: mTab.getActivity().getFadingBackgroundView().setViewAlpha(0.f); nit: 0f? I see a mix of 0f, 0.0f, 0.f, .0f is used in our codebase, but why not just the simplest one if they are equivalent? https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:490: final LogoManagerImpl manager = mLogoManager; why the local variable here? https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java:68: if (!isEnabled() || MathUtils.areFloatsEqual(alpha, getAlpha())) return; Document the class to say what enabling or disabling it does? Clicks will still close the bottom sheet right? Alternatively, override setEnabled to force alpha to 0f and document the behaviour at the same time?
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/chrome_home_new_tab_page.xml (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/chrome_home_new_tab_page.xml:11: android:focusable="true" On 2017/02/28 14:09:56, dgn wrote: > why make it focusable? is that related to accessibility? If it's not focusable then the toolbar/omnibox gets focus when an NTP is open. It also allows the full NTP to gain focus when TalkBack is on. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:32: public class ChromeHomeNewTabPage implements NativePage, TemplateUrlServiceObserver { On 2017/02/28 14:09:56, dgn wrote: > is "Chrome" needed in the class name? Yes, I think it is. "Chrome Home" is the name of the feature. "HomeNewTabPage" doesn't really make sense. I could change this to "SimpleNewTabPage" or something like that instead if that's preferable. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:36: private final LogoManagerImpl mLogoManager; On 2017/02/28 13:36:15, Michael van Ouwerkerk wrote: > I wanted to say: just use the interface type LogoManager, but destroy() is not > in the base interface, so you need to type your variables to LogoManagerImpl. I > guess that's ok for now. Thank you for the feedback. I made destroy() an interface method. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:150: boolean newTabPageSelected = mTabModelSelector.getCurrentTab().getUrl().equals(getUrl()) On 2017/02/28 14:09:56, dgn wrote: > On 2017/02/28 13:36:15, Michael van Ouwerkerk wrote: > > This newTabPageSelected variable is unused... what is it for? Looks like a > > leftover that can be deleted. > > Looks like it got replaced by the isTabChromeHomeNewTabPage() call on the line > below :) That's correct -- it's no longer needed since it was replaced with a call to isTabChromeHomeNewTabPage(). https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:164: mTab.getActivity().getFadingBackgroundView().setViewAlpha(0.f); On 2017/02/28 14:09:56, dgn wrote: > nit: 0f? I see a mix of 0f, 0.0f, 0.f, .0f is used in our codebase, but why not > just the simplest one if they are equivalent? Done (in FadingBackgroundView). https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPage.java:178: return tab.getUrl().equals(getUrl()) && !tab.isIncognito(); On 2017/02/28 13:36:15, Michael van Ouwerkerk wrote: > nit: use only one space after return Done. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoManagerImpl.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoManagerImpl.java:63: RecordHistogram.recordSparseSlowlyHistogram(LOGO_CLICK_UMA_NAME, CTA_IMAGE_CLICKED); On 2017/02/28 13:36:15, Michael van Ouwerkerk wrote: > I think we should separate old-style NTP histograms from Chrome Home histograms. > If we don't, any changes will be difficult to attribute as NTP and Chrome Home > might co-exist for some time while we ramp up usage of Chrome Home. What do you > think? I would prefer to use the same histograms because then we can rely on our dashboards easily visualize differences between the Chrome Home and default experiment groups. We can set these histograms as important in the variations associated data configuration files so that we get alerts if there are statistically significant differences. If we use separate histograms then we would have to do that comparison/analysis manually. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:93: public interface LogoManager { On 2017/02/28 13:36:15, Michael van Ouwerkerk wrote: > Can we rename this to Delegate and refer to LogoView.Delegate everywhere? I > think "delegate" expresses what this does a bit more accurately than "manager" > and we avoid the duplication in LogoView.LogoManager. It's what we've been > moving to when breaking pieces out of the NewTabPageManager: > https://cs.chromium.org/search/?q=f:ntp%7Csuggestions+interface.*Delegate+lan... Done. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:490: final LogoManagerImpl manager = mLogoManager; On 2017/02/28 14:09:56, dgn wrote: > why the local variable here? The variable is referenced in onLogoAvailable() below so it needs to be final. mLogoManager isn't initialized in the constructor so it can't be final. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:495: mSearchProviderLogoView.setMananger(manager); On 2017/02/28 13:36:15, Michael van Ouwerkerk wrote: > Hmn, maybe rename setMananger to setManager now that you're here :-) I renamed it to setDelegate(). https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java:68: if (!isEnabled() || MathUtils.areFloatsEqual(alpha, getAlpha())) return; On 2017/02/28 14:09:56, dgn wrote: > Document the class to say what enabling or disabling it does? Clicks will still > close the bottom sheet right? No, clicks don't still close the bottom sheet. The view won't receive touch events if it's disabled. > Alternatively, override setEnabled to force alpha to 0f and document the > behaviour at the same time? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
lgtm if Michael is happy https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:490: final LogoManagerImpl manager = mLogoManager; On 2017/02/28 17:02:23, Theresa wrote: > On 2017/02/28 14:09:56, dgn wrote: > > why the local variable here? > > The variable is referenced in onLogoAvailable() below so it needs to be final. > mLogoManager isn't initialized in the constructor so it can't be final. but member variables don't need to be final to be used in callbacks, right? For example NewTabPage#mIsDestroyed being used in the NewTabPageManager implementation. https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java:68: if (!isEnabled() || MathUtils.areFloatsEqual(alpha, getAlpha())) return; On 2017/02/28 17:02:23, Theresa wrote: > On 2017/02/28 14:09:56, dgn wrote: > > Document the class to say what enabling or disabling it does? Clicks will > still > > close the bottom sheet right? > > No, clicks don't still close the bottom sheet. The view won't receive touch > events if it's disabled. > Ah thanks, missed that. > > Alternatively, override setEnabled to force alpha to 0f and document the > > behaviour at the same time? > > Done.
lgtm with nits Thanks! https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:40: * Construct a new LogoManagerImpl. nit: "Construct a new {@link LogoDelegateImpl}." https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java (right): https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:142: public void setDelegate(Delegate manager) { nit: rename param to delegate https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:96: private LogoDelegateImpl mLogoDelegate; So now this can be of type LogoView.Delegate ?
twellington@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc@ for ChromeActivity and FadingBackgroundView OWNERS https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2715433008/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:490: final LogoManagerImpl manager = mLogoManager; On 2017/02/28 17:21:23, dgn wrote: > On 2017/02/28 17:02:23, Theresa wrote: > > On 2017/02/28 14:09:56, dgn wrote: > > > why the local variable here? > > > > The variable is referenced in onLogoAvailable() below so it needs to be final. > > mLogoManager isn't initialized in the constructor so it can't be final. > > but member variables don't need to be final to be used in callbacks, right? For > example NewTabPage#mIsDestroyed being used in the NewTabPageManager > implementation. You're right. Done. https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java (right): https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java:40: * Construct a new LogoManagerImpl. On 2017/02/28 17:24:32, Michael van Ouwerkerk wrote: > nit: "Construct a new {@link LogoDelegateImpl}." Done. https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java (right): https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoView.java:142: public void setDelegate(Delegate manager) { On 2017/02/28 17:24:32, Michael van Ouwerkerk wrote: > nit: rename param to delegate Done. https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2715433008/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:96: private LogoDelegateImpl mLogoDelegate; On 2017/02/28 17:24:32, Michael van Ouwerkerk wrote: > So now this can be of type LogoView.Delegate ? Done.
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2715433008/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java (right): https://codereview.chromium.org/2715433008/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java:80: if (!isEnabled) setAlpha(0f); Should this be canceling the mOverlayAnimator as well?
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
Thank you all for the reviews. https://codereview.chromium.org/2715433008/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java (right): https://codereview.chromium.org/2715433008/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java:80: if (!isEnabled) setAlpha(0f); On 2017/02/28 17:56:33, Ted C wrote: > Should this be canceling the mOverlayAnimator as well? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by twellington@chromium.org
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org, dgn@chromium.org, mvanouwerkerk@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2715433008/#ps140001 (title: "Cancel inprogress animation when FBV disabled")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488309206555300,
"parent_rev": "c1d9ece59f24058d02965208c692c4204dcef799", "commit_rev":
"5c88a0f3700057690cd3e644e346e5fef84ae666"}
Message was sent while issue was closed.
Description was changed from ========== [Home] Add a ChromeHomeNewTabPage Adds a simple NTP that displays the search provider logo and a close button for use in Chrome Home. BUG=695973 ========== to ========== [Home] Add a ChromeHomeNewTabPage Adds a simple NTP that displays the search provider logo and a close button for use in Chrome Home. BUG=695973 Review-Url: https://codereview.chromium.org/2715433008 Cr-Commit-Position: refs/heads/master@{#453662} Committed: https://chromium.googlesource.com/chromium/src/+/5c88a0f3700057690cd3e644e346... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5c88a0f3700057690cd3e644e346... |
