Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599743002:
Trigger the Snippets Scheduler when Resuming Activities (Closed)
DescriptionTrigger the Snippets Scheduler when Resuming Activities
Inform the scheduler on resuming activites, so fetching of new
snippets consider this event.
(There is a related CL that triggers a similar event for new tabs:
https://codereview.chromium.org/2592053004/)
BUG=676308
Review-Url: https://codereview.chromium.org/2599743002
Cr-Commit-Position: refs/heads/master@{#441768}
Committed: https://chromium.googlesource.com/chromium/src/+/cb5e4457d0cef43f5a3676ee30139fe9be28eaa2
Patch Set 1 #Patch Set 2 : The most important line was missing. #
Total comments: 11
Patch Set 3 : Renamed and moved the call to the bridge. #
Total comments: 8
Patch Set 4 : Used ActivityStateListener and renamed/dropped event functions. #
Total comments: 7
Patch Set 5 : Added distinction warm/cold starts. #Patch Set 6 : Rebased. #Patch Set 7 : Two separate signals instead of boolean value. #
Total comments: 4
Patch Set 8 : Explicit checks for cold/warm starts. #
Total comments: 3
Patch Set 9 : Removed Listener and introduced |isWarmOnResume|. #Patch Set 10 : Check scheduler to be valid. #
Total comments: 2
Patch Set 11 : Remove unnecessary "= false". #
Total comments: 10
Patch Set 12 : Improve variable names and comments. #Patch Set 13 : Rebase #Messages
Total messages: 61 (20 generated)
Patchset #1 (id:1) has been deleted
fhorschig@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Hi Michael, could you please take a look here, too?
The CQ bit was checked by fhorschig@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.
https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); This might not be the real browser startup. The process could have been running already due to e.g. the push notification service. It seems to be the activity startup though, so probably still what you want, but the naming is a bit confusing, I'd suggest "onActivityInitialized". Also, this will run more than once, like when opening ChromeTabbedActivity2 for split screen. Would that be handled correctly? https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:175: * Notifies the scheduler to adjust the plan due to a newly opened NTP. Please update the doc.
dgn@chromium.org changed reviewers: + dgn@chromium.org
https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); If the reference is not needed for anything else, why not make a static function taking the profile as input? or at least just making the snippetsbridge a local variable?
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > This might not be the real browser startup. The process could have been running > already due to e.g. the push notification service. It seems to be the activity > startup though, so probably still what you want, but the naming is a bit > confusing, I'd suggest "onActivityInitialized". Also, this will run more than > once, like when opening ChromeTabbedActivity2 for split screen. Would that be > handled correctly? to clarify: what we really want is to get notified when a user brings chrome in the foreground (using the app swichter, tapping the icon, etc). Triggering more often is fine as the scheduler will decide when to fetch and make sure there's reasonable time in between fetches and we don't call too often. It sounds like this place would be a good approximation. Any other ideas?
https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); On 2016/12/22 14:37:22, tschumann wrote: > On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > > This might not be the real browser startup. The process could have been > running > > already due to e.g. the push notification service. It seems to be the activity > > startup though, so probably still what you want, but the naming is a bit > > confusing, I'd suggest "onActivityInitialized". Also, this will run more than > > once, like when opening ChromeTabbedActivity2 for split screen. Would that be > > handled correctly? > > to clarify: what we really want is to get notified when a user brings chrome in > the foreground (using the app swichter, tapping the icon, etc). Triggering more > often is fine as the scheduler will decide when to fetch and make sure there's > reasonable time in between fetches and we don't call too often. > It sounds like this place would be a good approximation. Any other ideas? If you want to be notified of foregrounding, Activity#onResume() seems like a better place: https://developer.android.com/reference/android/app/Activity.html
https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); On 2016/12/22 14:35:09, dgn wrote: > If the reference is not needed for anything else, why not make a static function > taking the profile as input? or at least just making the snippetsbridge a local > variable? Made the variable local. https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > This might not be the real browser startup. The process could have been running > already due to e.g. the push notification service. It seems to be the activity > startup though, so probably still what you want, but the naming is a bit > confusing, I'd suggest "onActivityInitialized". Also, this will run more than > once, like when opening ChromeTabbedActivity2 for split screen. Would that be > handled correctly? Renamed. https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); On 2016/12/22 14:55:46, Michael van Ouwerkerk wrote: > On 2016/12/22 14:37:22, tschumann wrote: > > On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > > > This might not be the real browser startup. The process could have been > > running > > > already due to e.g. the push notification service. It seems to be the > activity > > > startup though, so probably still what you want, but the naming is a bit > > > confusing, I'd suggest "onActivityInitialized". Also, this will run more > than > > > once, like when opening ChromeTabbedActivity2 for split screen. Would that > be > > > handled correctly? > > > > to clarify: what we really want is to get notified when a user brings chrome > in > > the foreground (using the app swichter, tapping the icon, etc). Triggering > more > > often is fine as the scheduler will decide when to fetch and make sure there's > > reasonable time in between fetches and we don't call too often. > > It sounds like this place would be a good approximation. Any other ideas? > > If you want to be notified of foregrounding, Activity#onResume() seems like a > better place: https://developer.android.com/reference/android/app/Activity.html Moved. According to the lifecycle you linked, we probably want it onStart. Resume would probably triggered a little too often (e.g. after closing activities for settings?). I moved it, although this place was initially recommended by Bernhard (see bug: http://crbug.com/676308). https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:175: * Notifies the scheduler to adjust the plan due to a newly opened NTP. On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > Please update the doc. Thank you.
fhorschig@chromium.org changed reviewers: + bauerb@chromium.org, jkrcal@chromium.org
jkrcal@chromium.org: Please have a OWNER's look at the single TODO. And if the onStart is what you intended or if nativeInitialization or onResume would be a better fit. bauerb@chromium.org: Please review changes in the ChromeTabbedActivity and the snippets_bridge.*
https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); On 2016/12/22 14:55:46, Michael van Ouwerkerk wrote: > On 2016/12/22 14:37:22, tschumann wrote: > > On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > > > This might not be the real browser startup. The process could have been > > running > > > already due to e.g. the push notification service. It seems to be the > activity > > > startup though, so probably still what you want, but the naming is a bit > > > confusing, I'd suggest "onActivityInitialized". Also, this will run more > than > > > once, like when opening ChromeTabbedActivity2 for split screen. Would that > be > > > handled correctly? > > > > to clarify: what we really want is to get notified when a user brings chrome > in > > the foreground (using the app swichter, tapping the icon, etc). Triggering > more > > often is fine as the scheduler will decide when to fetch and make sure there's > > reasonable time in between fetches and we don't call too often. > > It sounds like this place would be a good approximation. Any other ideas? > > If you want to be notified of foregrounding, Activity#onResume() seems like a > better place: https://developer.android.com/reference/android/app/Activity.html To know when Chrome (= any Chrome activity) goes in the foreground, you can use ApplicationStateListener: https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... ActivityStateListener is also available for tracking a specific activity. https://codereview.chromium.org/2599743002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:474: SnippetsBridge mSnippetsBridge = new SnippetsBridge(Profile.getLastUsedProfile()); nit: no m prefix for local variables
On 2016/12/22 14:55:46, Michael van Ouwerkerk wrote: > https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java > (right): > > https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: > mSnippetsBridge.onBrowserStartup(); > On 2016/12/22 14:37:22, tschumann wrote: > > On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > > > This might not be the real browser startup. The process could have been > > running > > > already due to e.g. the push notification service. It seems to be the > activity > > > startup though, so probably still what you want, but the naming is a bit > > > confusing, I'd suggest "onActivityInitialized". Also, this will run more > than > > > once, like when opening ChromeTabbedActivity2 for split screen. Would that > be > > > handled correctly? This is weird, I've chatted with Bernhard about it, are you sure, Michael? > > to clarify: what we really want is to get notified when a user brings chrome > in > > the foreground (using the app swichter, tapping the icon, etc). Triggering > more > > often is fine as the scheduler will decide when to fetch and make sure there's > > reasonable time in between fetches and we don't call too often. > > It sounds like this place would be a good approximation. Any other ideas? > > If you want to be notified of foregrounding, Activity#onResume() seems like a > better place: https://developer.android.com/reference/android/app/Activity.html A quick comment before I forget it, I do a proper review after the holidays: To me, a cold startup of chrome is a different signal than foregrounding. There are two reasons: 1) - On a cold start, we need to be careful about doing the fetch right away so that we do not impact startup time, - I would also like to record a histogram how often we perform a fetch on a cold startup to assess the impact on startup, 2) - My feeling is that foregrounding leads in lower percentage to opening a new NTP than a cold start so I would use it as a weaker signal. - This feeling is not supported by any data, I would like to collect data how often a new NTP is opened after a given signal. We will probably not have this metric ready for M57 but anyway it is good to keep the signals separate. I think that a signal on foregrounding is a great idea, can we have it as another signal and another trigger function in Scheduler, please?
On 2016/12/22 21:08:53, jkrcal wrote: > On 2016/12/22 14:55:46, Michael van Ouwerkerk wrote: > > > https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... > > File > > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java > > (right): > > > > > https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: > > mSnippetsBridge.onBrowserStartup(); > > On 2016/12/22 14:37:22, tschumann wrote: > > > On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > > > > This might not be the real browser startup. The process could have been > > > running > > > > already due to e.g. the push notification service. It seems to be the > > activity > > > > startup though, so probably still what you want, but the naming is a bit > > > > confusing, I'd suggest "onActivityInitialized". Also, this will run more > > than > > > > once, like when opening ChromeTabbedActivity2 for split screen. Would that > > be > > > > handled correctly? > > This is weird, I've chatted with Bernhard about it, are you sure, Michael? I did add some logging and confirmed it logged repeatedly. I tried it :-) Also, when I did this CL (https://codereview.chromium.org/2554003003/) I confirmed that the process can already be running for a service. > > > > to clarify: what we really want is to get notified when a user brings chrome > > in > > > the foreground (using the app swichter, tapping the icon, etc). Triggering > > more > > > often is fine as the scheduler will decide when to fetch and make sure > there's > > > reasonable time in between fetches and we don't call too often. > > > It sounds like this place would be a good approximation. Any other ideas? > > > > If you want to be notified of foregrounding, Activity#onResume() seems like a > > better place: > https://developer.android.com/reference/android/app/Activity.html > > A quick comment before I forget it, I do a proper review after the holidays: > > To me, a cold startup of chrome is a different signal than foregrounding. There > are two reasons: > > 1) > - On a cold start, we need to be careful about doing the fetch right away so > that we do not impact startup time, > - I would also like to record a histogram how often we perform a fetch on a > cold startup to assess the impact on startup, > > 2) > - My feeling is that foregrounding leads in lower percentage to opening a new > NTP than a cold start so I would use it as a weaker signal. > - This feeling is not supported by any data, I would like to collect data how > often a new NTP is opened after a given signal. > We will probably not have this metric ready for M57 but anyway it is good to > keep the signals separate. > > I think that a signal on foregrounding is a great idea, can we have it as > another signal and another trigger function in Scheduler, please? I think you can distinguish more states: 1. Cold process start, first activity is created. This is very slow. 2. Warm activity start, the native library was already loaded for e.g. a service. 3. Creation of a second activity. 4. Foregrounding. Maybe we need to clarify what the exact goal is before continuing this implementation. Fwiw, I added a hadWarmStart() method to the activity recently, so you could pass that boolean to the scheduler if it is useful.
One more thing to inform the cold start / warm start discussion: check out the new NewTabPage.LoadType histogram. For the NTP, it looks like 1/4 of starts is warm, meaning the native library was already loaded and initialized. This has a huge impact on load time, as measured in: NewTabPage.SearchAvailableLoadTime.WarmStart and NewTabPage.SearchAvailableLoadTime.ColdStart Notably on svelte (low memory) devices, the cold start is tragically slow and we should probably avoid slowing that further. It is also more prevalent, presumably because the OS kills Chrome more often, preventing warm starts. https://codereview.chromium.org/2599743002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:475: mSnippetsBridge.onBrowserStarted(); This name seems wrong, as this is still not guaranteed to be a browser start. Like I said, the process could already be running for a service like for push notifications. Or, this method could run as a member of ChromeTabbedActivity2, for split windows. So this can run any number of times. I'd suggest renaming to something like "onActivityStarted" with explicit documentation in that method that it can be called more than once. https://codereview.chromium.org/2599743002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2599743002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:403: ->OnBrowserStartup(); Rename and document this method also. https://codereview.chromium.org/2599743002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2599743002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:141: // TODO(fhorschig): Consider that this is called twice for split screens. Not just twice, any number of times if the user split, closed, split again.
On 2016/12/23 10:56:56, Michael van Ouwerkerk wrote: > On 2016/12/22 21:08:53, jkrcal wrote: > > On 2016/12/22 14:55:46, Michael van Ouwerkerk wrote: > > > > > > https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... > > > File > > > > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: > > > mSnippetsBridge.onBrowserStartup(); > > > On 2016/12/22 14:37:22, tschumann wrote: > > > > On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > > > > > This might not be the real browser startup. The process could have been > > > > running > > > > > already due to e.g. the push notification service. It seems to be the > > > activity > > > > > startup though, so probably still what you want, but the naming is a bit > > > > > confusing, I'd suggest "onActivityInitialized". Also, this will run more > > > than > > > > > once, like when opening ChromeTabbedActivity2 for split screen. Would > that > > > be > > > > > handled correctly? > > > > This is weird, I've chatted with Bernhard about it, are you sure, Michael? > > I did add some logging and confirmed it logged repeatedly. I tried it :-) Also, > when I did this CL (https://codereview.chromium.org/2554003003/) I confirmed > that the process can already be running for a service. > > > > > > > to clarify: what we really want is to get notified when a user brings > chrome > > > in > > > > the foreground (using the app swichter, tapping the icon, etc). Triggering > > > more > > > > often is fine as the scheduler will decide when to fetch and make sure > > there's > > > > reasonable time in between fetches and we don't call too often. > > > > It sounds like this place would be a good approximation. Any other ideas? > > > > > > If you want to be notified of foregrounding, Activity#onResume() seems like > a > > > better place: > > https://developer.android.com/reference/android/app/Activity.html > > > > A quick comment before I forget it, I do a proper review after the holidays: > > > > To me, a cold startup of chrome is a different signal than foregrounding. > There > > are two reasons: > > > > 1) > > - On a cold start, we need to be careful about doing the fetch right away so > > that we do not impact startup time, > > - I would also like to record a histogram how often we perform a fetch on a > > cold startup to assess the impact on startup, > > > > 2) > > - My feeling is that foregrounding leads in lower percentage to opening a new > > NTP than a cold start so I would use it as a weaker signal. > > - This feeling is not supported by any data, I would like to collect data how > > often a new NTP is opened after a given signal. > > We will probably not have this metric ready for M57 but anyway it is good > to > > keep the signals separate. > > > > I think that a signal on foregrounding is a great idea, can we have it as > > another signal and another trigger function in Scheduler, please? > > I think you can distinguish more states: > 1. Cold process start, first activity is created. This is very slow. > 2. Warm activity start, the native library was already loaded for e.g. a > service. > 3. Creation of a second activity. > 4. Foregrounding. > > Maybe we need to clarify what the exact goal is before continuing this > implementation. Thanks for the investigation and pointing out the different states. I believe the discussion in this CL pretty much contains the main criteria, so let me summarize the requirement (@Jan, can you make sure they make it into the design document?): We want to find user-initiated trigger points to pro-actively fetch data: (1) close to the time of potential consumption (2) targeted towards active chrome users (2a) especially those interacting with the NTP [will be adjusted with Chrome Home) (3) Not negatively impact start-up time. All these signals get into the scheduler were we can experiment with weighting them and different trigger criteria (staying within budget; possibly limiting to active NTP/Zine users -- that's especially for (2a), etc.). > > Fwiw, I added a hadWarmStart() method to the activity recently, so you could > pass that boolean to the scheduler if it is useful.
Patchset #4 (id:80001) has been deleted
So in agreement with Jan, this CL was slightly reworked. In place of a browser startup, we will use the constructor and Activity#Resume (as proposed by Michael) bound to the ChromeTabbedActivity (using ActivityStateListener, as proposed by dgn@). This ensures, that we only react to user-triggered actions and not to automatic starts/wake ups of the app. For 1: Opening the NTP is closest to the consumption and handled with CL 2592053004 [1]. For 2, 2a and 3: This is an implementation detail of the scheduler. I added comments and ToDos at the places where deferred calls will be preferred over actually performing the work (i.e. for cold starts). Also: Description update incoming. [1] https://codereview.chromium.org/2592053004/ https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:414: mSnippetsBridge.onBrowserStartup(); On 2016/12/22 15:59:57, dgn wrote: > On 2016/12/22 14:55:46, Michael van Ouwerkerk wrote: > > On 2016/12/22 14:37:22, tschumann wrote: > > > On 2016/12/22 14:30:23, Michael van Ouwerkerk wrote: > > > > This might not be the real browser startup. The process could have been > > > running > > > > already due to e.g. the push notification service. It seems to be the > > activity > > > > startup though, so probably still what you want, but the naming is a bit > > > > confusing, I'd suggest "onActivityInitialized". Also, this will run more > > than > > > > once, like when opening ChromeTabbedActivity2 for split screen. Would that > > be > > > > handled correctly? > > > > > > to clarify: what we really want is to get notified when a user brings chrome > > in > > > the foreground (using the app swichter, tapping the icon, etc). Triggering > > more > > > often is fine as the scheduler will decide when to fetch and make sure > there's > > > reasonable time in between fetches and we don't call too often. > > > It sounds like this place would be a good approximation. Any other ideas? > > > > If you want to be notified of foregrounding, Activity#onResume() seems like a > > better place: > https://developer.android.com/reference/android/app/Activity.html > > To know when Chrome (= any Chrome activity) goes in the foreground, you can use > ApplicationStateListener: > https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... > > ActivityStateListener is also available for tracking a specific activity. Thank you, this is exactly what we needed. https://codereview.chromium.org/2599743002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:474: SnippetsBridge mSnippetsBridge = new SnippetsBridge(Profile.getLastUsedProfile()); On 2016/12/22 15:59:57, dgn wrote: > nit: no m prefix for local variables I now use a member to store the listener. https://codereview.chromium.org/2599743002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:475: mSnippetsBridge.onBrowserStarted(); On 2016/12/23 11:04:42, Michael van Ouwerkerk wrote: > This name seems wrong, as this is still not guaranteed to be a browser start. > Like I said, the process could already be running for a service like for push > notifications. Or, this method could run as a member of ChromeTabbedActivity2, > for split windows. So this can run any number of times. I'd suggest renaming to > something like "onActivityStarted" with explicit documentation in that method > that it can be called more than once. We figured, that we don't actually want a browser start, so this is gone. Now, the Activity#Resume event is used and everything is named accordingly. https://codereview.chromium.org/2599743002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2599743002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:403: ->OnBrowserStartup(); On 2016/12/23 11:04:42, Michael van Ouwerkerk wrote: > Rename and document this method also. Renamed to Resuming. https://codereview.chromium.org/2599743002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2599743002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:141: // TODO(fhorschig): Consider that this is called twice for split screens. On 2016/12/23 11:04:42, Michael van Ouwerkerk wrote: > Not just twice, any number of times if the user split, closed, split again. Adjusted comment to the resuming functionality.
Description was changed from ========== Trigger the Snippets Scheduler on Browser Startup Inform the scheduler on browser startup, so fetching of new snippets can consider this event. (There is a related CL that triggers a similar event for new tabs: https://codereview.chromium.org/2592053004/) BUG=676308 ========== to ========== Trigger the Snippets Scheduler when Resuming Activities Inform the scheduler on resuming activites, so fetching of new snippets consider this event. (There is a related CL that triggers a similar event for new tabs: https://codereview.chromium.org/2592053004/) BUG=676308 ==========
https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called whenever we open or return to an Activity. can we be more specific? e.g. // Called whenever the Chrome activity is resumed (i.e. when the user switches to chrome). also, if this includes cold starts of chrome, please add this. From "resumed" that's not entirely clear to me. I guess, there's no way for us to know in which situation (warm-vs-cold start) we are, hm?
https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called whenever we open or return to an Activity. On 2017/01/03 15:24:00, tschumann wrote: > can we be more specific? e.g. > // Called whenever the Chrome activity is resumed (i.e. when the user switches > to chrome). > > also, if this includes cold starts of chrome, please add this. From "resumed" > that's not entirely clear to me. > > I guess, there's no way for us to know in which situation (warm-vs-cold start) > we are, hm? You can ask the activity whether it had a warm start, see AsyncInitializationActivity#hadWarmStart(). That boolean does not change, and resume is called repeatedly, not only for starts. So to know whether you're in some kind of start, you can check whether AsyncInitializationActivity#getLastUserInteractionTime() returns 0. My thinking there is that if the user has not interacted with the activity yet, is is a start.
On 2017/01/03 15:24:01, tschumann wrote: > https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... > File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): > > https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... > components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called > whenever we open or return to an Activity. > can we be more specific? e.g. > // Called whenever the Chrome activity is resumed (i.e. when the user switches > to chrome). > > also, if this includes cold starts of chrome, please add this. From "resumed" > that's not entirely clear to me. > > I guess, there's no way for us to know in which situation (warm-vs-cold start) > we are, hm? In an offline discussion with Friedrich, we agreed that we do not need the cold-vs-warm start info. Cold start is easy to tell because the actual Scheduler is constructed. The scheduler can have a bool delay_background_fetches_, initially set to true and set it to false after a delay of ~10 seconds (and in this moment, deal with any pending request to RefetchInTheBackground()). The "resumed" signal should also include opening Chrome with a cold start (we need to distinguish loading up the library that should never directly result in fetching from the user signals that may lead to fetching). Does it make sense to you?
On 2017/01/03 17:01:18, jkrcal wrote: > On 2017/01/03 15:24:01, tschumann wrote: > > > https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... > > File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): > > > > > https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... > > components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called > > whenever we open or return to an Activity. > > can we be more specific? e.g. > > // Called whenever the Chrome activity is resumed (i.e. when the user switches > > to chrome). > > > > also, if this includes cold starts of chrome, please add this. From "resumed" > > that's not entirely clear to me. > > > > I guess, there's no way for us to know in which situation (warm-vs-cold start) > > we are, hm? > > In an offline discussion with Friedrich, we agreed that we do not need the > cold-vs-warm start info. > Cold start is easy to tell because the actual Scheduler is constructed. > The scheduler can have a bool delay_background_fetches_, initially set to true > and set it to false after a delay of ~10 seconds (and in this moment, deal with > any pending request to RefetchInTheBackground()). > > The "resumed" signal should also include opening Chrome with a cold start > (we need to distinguish loading up the library that should never directly > result in fetching from the user signals that may lead to fetching). > > Does it make sense to you? Not sure if construction of the scheduler is the right heuristic, I have no idea what the library load event encompasses; leaving this up for Michael. If that's working, I'm totally fine. I'm just concerned that we end up with sub-optimal behavior because of being afraid of the cold-start case. Bringing Chrome to the foreground is an excellent signal to immediately update our data (given it's considered old enough). Concerning the precise implementation: I'm not a fan of time-based delays. We could simply wait for the first user-interaction that triggers an event in the scheduler to unset the state.
On 2017/01/03 17:01:18, jkrcal wrote: > On 2017/01/03 15:24:01, tschumann wrote: > > > https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... > > File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): > > > > > https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... > > components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called > > whenever we open or return to an Activity. > > can we be more specific? e.g. > > // Called whenever the Chrome activity is resumed (i.e. when the user switches > > to chrome). > > > > also, if this includes cold starts of chrome, please add this. From "resumed" > > that's not entirely clear to me. > > > > I guess, there's no way for us to know in which situation (warm-vs-cold start) > > we are, hm? > > In an offline discussion with Friedrich, we agreed that we do not need the > cold-vs-warm start info. > Cold start is easy to tell because the actual Scheduler is constructed. > The scheduler can have a bool delay_background_fetches_, initially set to true > and set it to false after a delay of ~10 seconds (and in this moment, deal with > any pending request to RefetchInTheBackground()). > > The "resumed" signal should also include opening Chrome with a cold start > (we need to distinguish loading up the library that should never directly > result in fetching from the user signals that may lead to fetching). > > Does it make sense to you? Not sure if construction of the scheduler is the right heuristic, I have no idea what the library load event encompasses; leaving this up for Michael. If that's working, I'm totally fine. I'm just concerned that we end up with sub-optimal behavior because of being afraid of the cold-start case. Bringing Chrome to the foreground is an excellent signal to immediately update our data (given it's considered old enough). Concerning the precise implementation: I'm not a fan of time-based delays. We could simply wait for the first user-interaction that triggers an event in the scheduler to unset the state.
We do have the hot vs. cold start info now. And it worked for both cases. More comments inline. On 2017/01/03 17:31:21, tschumann wrote: > On 2017/01/03 17:01:18, jkrcal wrote: > > > > In an offline discussion with Friedrich, we agreed that we do not need the > > cold-vs-warm start info. We have this info now. It's cheap and we can consider using it instead of the time delay. > > Cold start is easy to tell because the actual Scheduler is constructed. > > The scheduler can have a bool delay_background_fetches_, initially set to true > > and set it to false after a delay of ~10 seconds (and in this moment, deal > with > > any pending request to RefetchInTheBackground()). > > > > The "resumed" signal should also include opening Chrome with a cold start This is happening now. > > (we need to distinguish loading up the library that should never directly > > result in fetching from the user signals that may lead to fetching). > > > > Does it make sense to you? Yes, loading the library is untied from notifying the scheduler. > > Not sure if construction of the scheduler is the right heuristic, I have no idea > what the library load event encompasses; leaving this up for The resume event is now reliably called whenever a tab appears on the users' device. > If that's working, I'm totally fine. I'm just concerned that we end up with > sub-optimal behavior because of being afraid of the cold-start case. > Bringing Chrome to the foreground is an excellent signal to immediately update > our data (given it's considered old enough). That's possible now. We should discuss if it is necessary to update snippets on cold starts. They might be updated when the new tab page is opened anyway. > > Concerning the precise implementation: I'm not a fan of time-based delays. We > could simply wait for the first user-interaction that triggers an event in the > scheduler to unset the state. Such event would either be "Resume" on warm starts or opening an NTP on cold ones. Those two cover pretty much every interaction and will never be called by services.
https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called whenever we open or return to an Activity. On 2017/01/03 16:51:08, Michael van Ouwerkerk wrote: > On 2017/01/03 15:24:00, tschumann wrote: > > can we be more specific? e.g. > > // Called whenever the Chrome activity is resumed (i.e. when the user switches > > to chrome). Only a little more specific. This comment has to apply for iOS, too. > > also, if this includes cold starts of chrome, please add this. From "resumed" > > that's not entirely clear to me. Cold starts were not included as the listener would only be triggered on warm starts. > > I guess, there's no way for us to know in which situation (warm-vs-cold start) > > we are, hm? The old distinction was: Resume called? --> warm. Otherwise use constructor. As we don't like time delays, the event triggered on resume will be triggered on cold starts as well, so there is a bool telling us whether it's warm or not. > You can ask the activity whether it had a warm start, see > AsyncInitializationActivity#hadWarmStart(). That boolean does not change, and The fact that it is stored caused me to never get another value than "false". > resume is called repeatedly, not only for starts. So to know whether you're in Resume might be called everytime, but for some reason, the listener will not get called on resume on cold starts. Not sure why and if this is WAI. > some kind of start, you can check whether > AsyncInitializationActivity#getLastUserInteractionTime() returns 0. My thinking > there is that if the user has not interacted with the activity yet, is is a > start. Thank you, this is perfect! I was so desperately looking for that. It found its way into the code (in another function and commented).
lgtm nice! focusing on the overall design (signal + naming) this looks good to me. My preference is to have a stronger focus on warm-starts only, but I'm fine either way. https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called whenever we open or return to an Activity. On 2017/01/04 10:17:04, fhorschig wrote: > On 2017/01/03 16:51:08, Michael van Ouwerkerk wrote: > > On 2017/01/03 15:24:00, tschumann wrote: > > > can we be more specific? e.g. > > > // Called whenever the Chrome activity is resumed (i.e. when the user > switches > > > to chrome). > > Only a little more specific. This comment has to apply for iOS, too. > > > > also, if this includes cold starts of chrome, please add this. From > "resumed" > > > that's not entirely clear to me. > > Cold starts were not included as the listener would only be triggered on warm > starts. > > > > I guess, there's no way for us to know in which situation (warm-vs-cold > start) > > > we are, hm? > > The old distinction was: Resume called? --> warm. Otherwise use constructor. > As we don't like time delays, the event triggered on resume will be triggered > on cold starts as well, so there is a bool telling us whether it's warm or not. > > > You can ask the activity whether it had a warm start, see > > AsyncInitializationActivity#hadWarmStart(). That boolean does not change, and > > The fact that it is stored caused me to never get another value than "false". > > > resume is called repeatedly, not only for starts. So to know whether you're in > > Resume might be called everytime, but for some reason, the listener will not > get called on resume on cold starts. Not sure why and if this is WAI. > > > some kind of start, you can check whether > > AsyncInitializationActivity#getLastUserInteractionTime() returns 0. My > thinking > > there is that if the user has not interacted with the activity yet, is is a > > start. > > Thank you, this is perfect! I was so desperately looking for that. It found its > way into the code (in another function and commented). I'm leaning towards not doing anything on cold starts, so should we just special case this signal (including the method name) to warm-starts only? [my preference].
The CQ bit was checked by fhorschig@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...
https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called whenever we open or return to an Activity. > I'm leaning towards not doing anything on cold starts, so should we just special > case this signal (including the method name) to warm-starts only? [my > preference]. Tim, do you mean - having just the warm-start signal (and throwing away part of the code of Friedrich) or - having two separate signals [my preference]?
https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called whenever we open or return to an Activity. On 2017/01/04 12:21:12, jkrcal wrote: > > I'm leaning towards not doing anything on cold starts, so should we just > special > > case this signal (including the method name) to warm-starts only? [my > > preference]. > > Tim, do you mean > - having just the warm-start signal (and throwing away part of the code of > Friedrich) or > - having two separate signals [my preference]? i'm leaning towards just the warm-start as I can't think of a reasonable way we want to do anything on a cold-start (scheduling something based on wall-time has a smell and unclear effects). But i'm fine with adding both signals.
First of all: Bernhard or Michael, could you please have a look? Added the two signals instead of one. I am very sure, that we need the cold start for an edge case we haven't thought of yet. Especially since new NTPs on first run are cold started but the call to |OnColdStart| happens after the first run experience has finished. Even if we don't it is easier to drop then. https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/100001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:16: // Called whenever we open or return to an Activity. On 2017/01/04 12:26:30, tschumann wrote: > On 2017/01/04 12:21:12, jkrcal wrote: > > > I'm leaning towards not doing anything on cold starts, so should we just > > special > > > case this signal (including the method name) to warm-starts only? [my > > > preference]. > > > > Tim, do you mean > > - having just the warm-start signal (and throwing away part of the code of > > Friedrich) or > > - having two separate signals [my preference]? > > i'm leaning towards just the warm-start as I can't think of a reasonable way we > want to do anything on a cold-start (scheduling something based on wall-time has > a smell and unclear effects). > But i'm fine with adding both signals. I added both signals as this makes it easier to drop one in case we decide that we never use it. This way, the heat of the start is pretty explicit, too.
components/ntp_snippets/ lgtm, thanks!
https://codereview.chromium.org/2599743002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:446: // Without any interaction, this start was cold and the listener will ignore it. Without user interaction, it is the start of the activity. Not guaranteed to be cold, as the native library might have been loaded already for a service or another activity. https://codereview.chromium.org/2599743002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2599743002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:192: // This event is only triggered when the start was already warm. Are you sure resume only happens for warm starts? That seems to be different from the documentation which indicates it happens as part of activity creation and startup: https://developer.android.com/reference/android/app/Activity.html
https://codereview.chromium.org/2599743002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2599743002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:446: // Without any interaction, this start was cold and the listener will ignore it. On 2017/01/04 14:32:17, Michael van Ouwerkerk wrote: > Without user interaction, it is the start of the activity. Not guaranteed to be > cold, as the native library might have been loaded already for a service or > another activity. Good lead, added a check and a warm alternative call for this case. https://codereview.chromium.org/2599743002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2599743002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:192: // This event is only triggered when the start was already warm. On 2017/01/04 14:32:17, Michael van Ouwerkerk wrote: > Are you sure resume only happens for warm starts? That seems to be different > from the documentation which indicates it happens as part of activity creation > and startup: https://developer.android.com/reference/android/app/Activity.html I was really surprised to notice this. This might be a flaw of the listener, or it is registered to late. I introduced a check for the loaded library for the case that this behavior should ever change.
https://codereview.chromium.org/2599743002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2599743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:194: if (newState == ActivityState.RESUMED && LibraryLoader.isInitialized()) { Reading LibraryLoaded.isInitialized() at this point will always return true, even if it was a cold start. That's because the state listener is registered in finishNativeInitialization which happens after native initialization. However, you can check activity.hadWarmStart() instead.
https://codereview.chromium.org/2599743002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2599743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:194: if (newState == ActivityState.RESUMED && LibraryLoader.isInitialized()) { On 2017/01/04 15:13:30, Michael van Ouwerkerk wrote: > Reading LibraryLoaded.isInitialized() at this point will always return true, > even if it was a cold start. That's because the state listener is registered in > finishNativeInitialization which happens after native initialization. However, > you can check activity.hadWarmStart() instead. This gives me always false. No matter if I run the first run experience or background chromium just for a second - it's _always_ cold. I really hope this is not correct. Is there any other way to detect warm starts?
I now check for warm onResume directly. The listener was removed as both static functions must be called in the ChromeTabbedActivity anyway and there is little reason to duplicate that code. https://codereview.chromium.org/2599743002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2599743002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:194: if (newState == ActivityState.RESUMED && LibraryLoader.isInitialized()) { On 2017/01/04 15:29:21, fhorschig wrote: > On 2017/01/04 15:13:30, Michael van Ouwerkerk wrote: > > Reading LibraryLoaded.isInitialized() at this point will always return true, > > even if it was a cold start. That's because the state listener is registered > in > > finishNativeInitialization which happens after native initialization. However, > > you can check activity.hadWarmStart() instead. > > This gives me always false. No matter if I run the first run experience or > background chromium just for a second - it's _always_ cold. > I really hope this is not correct. Is there any other way to detect warm starts? As discussed, the heat of onResume is detected where hadWarmStart() is set, too.
Michael, may I bother you again? (The recent changes prevent the crash that followed the related CL about newly opened tabs.)
lgtm https://codereview.chromium.org/2599743002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2599743002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:66: private boolean mIsWarmOnResume = false; false is the default, no need to assign that.
Thank you (also for your patience :) ) Bernhard, could you please have an OWNER's look? https://codereview.chromium.org/2599743002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2599743002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:66: private boolean mIsWarmOnResume = false; On 2017/01/05 14:36:53, Michael van Ouwerkerk wrote: > false is the default, no need to assign that. Done.
Mostly nits: https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:67: private boolean mFirstResume = true; Can you document this member? Specifically, this states whether the _next_ call to onResume() is going to be the first, not whether the most recent one was the first. https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:417: * warm. It seems this comment should go to where we calculate the value, not where we document the contract. Then can you be a bit more detailed here about what exactly warm means, and under which circumstances? Something like, "Whether this activity was warm (native library loaded and initialized) by the time it was last resumed. This is useful to distinguish between the case where an already running instance of Chrome is being brought back to the foreground from the case where Chrome is started, in order to avoid contention on browser startup". ...And now that I think of it that way, couldn't we get the value in the same way as hadWarmStart()? That is, in onResume() check whether the native library is loaded and set the flag based on that. If you want to land this CL in time for FF though, I'd be okay with trying that out in a followup. https://codereview.chromium.org/2599743002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:18: // Called whenever chrome is cold started. Could you add empty lines before comments please? https://codereview.chromium.org/2599743002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2599743002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:184: // TODO(fhorschig|jkrcal): Consider that this is called whenever we open or This syntax doesn't lend itself very well to grepping. Given that the purpose of a name in a TODO is to state the point of contact and not the person assigned to do it, can you just put a single name in there?
https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:67: private boolean mFirstResume = true; On 2017/01/05 16:32:53, Bernhard Bauer wrote: > Can you document this member? Specifically, this states whether the _next_ call > to onResume() is going to be the first, not whether the most recent one was the > first. Done. https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:417: * warm. On 2017/01/05 16:32:53, Bernhard Bauer wrote: > It seems this comment should go to where we calculate the value, not where we > document the contract. Done. > Then can you be a bit more detailed here about what exactly warm means, and > under which circumstances? Something like, "Whether this activity was warm > (native library loaded and initialized) by the time it was last resumed. This is > useful to distinguish between the case where an already running instance of > Chrome is being brought back to the foreground from the case where Chrome is > started, in order to avoid contention on browser startup". Done. Thank you for the wording idea! > ...And now that I think of it that way, couldn't we get the value in the same > way as hadWarmStart()? That is, in onResume() check whether the native library > is loaded and set the flag based on that. If you want to land this CL in time > for FF though, I'd be okay with trying that out in a followup. I think we had that and the problem was, that some cold cold starts take long enough for the library to load. So technically, it would be cold but it warmed up during the run. I can verify this as follow up nonetheless. https://codereview.chromium.org/2599743002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right): https://codereview.chromium.org/2599743002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_scheduler.h:18: // Called whenever chrome is cold started. On 2017/01/05 16:32:54, Bernhard Bauer wrote: > Could you add empty lines before comments please? Done. https://codereview.chromium.org/2599743002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2599743002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:184: // TODO(fhorschig|jkrcal): Consider that this is called whenever we open or On 2017/01/05 16:32:54, Bernhard Bauer wrote: > This syntax doesn't lend itself very well to grepping. Given that the purpose of > a name in a TODO is to state the point of contact and not the person assigned to > do it, can you just put a single name in there? Changed. Thank you for explaining, I didn't know this purpose.
LGTM https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:417: * warm. On 2017/01/05 17:11:13, fhorschig wrote: > On 2017/01/05 16:32:53, Bernhard Bauer wrote: > > It seems this comment should go to where we calculate the value, not where we > > document the contract. > > Done. > > > Then can you be a bit more detailed here about what exactly warm means, and > > under which circumstances? Something like, "Whether this activity was warm > > (native library loaded and initialized) by the time it was last resumed. This > is > > useful to distinguish between the case where an already running instance of > > Chrome is being brought back to the foreground from the case where Chrome is > > started, in order to avoid contention on browser startup". > > Done. Thank you for the wording idea! > > > > ...And now that I think of it that way, couldn't we get the value in the same > > way as hadWarmStart()? That is, in onResume() check whether the native library > > is loaded and set the flag based on that. If you want to land this CL in time > > for FF though, I'd be okay with trying that out in a followup. > > I think we had that and the problem was, that some cold cold starts take long > enough for the library to load. So technically, it would be cold but it warmed > up during the run. What do you mean by long enough? During which run? > I can verify this as follow up nonetheless. That would be great, thanks!
https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2599743002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:417: * warm. On 2017/01/05 17:19:45, Bernhard Bauer wrote: > On 2017/01/05 17:11:13, fhorschig wrote: > > On 2017/01/05 16:32:53, Bernhard Bauer wrote: > > > It seems this comment should go to where we calculate the value, not where > we > > > document the contract. > > > > Done. > > > > > Then can you be a bit more detailed here about what exactly warm means, and > > > under which circumstances? Something like, "Whether this activity was warm > > > (native library loaded and initialized) by the time it was last resumed. > This > > is > > > useful to distinguish between the case where an already running instance of > > > Chrome is being brought back to the foreground from the case where Chrome is > > > started, in order to avoid contention on browser startup". > > > > Done. Thank you for the wording idea! > > > > > > > ...And now that I think of it that way, couldn't we get the value in the > same > > > way as hadWarmStart()? That is, in onResume() check whether the native > library > > > is loaded and set the flag based on that. If you want to land this CL in > time > > > for FF though, I'd be okay with trying that out in a followup. > > > > I think we had that and the problem was, that some cold cold starts take long > > enough for the library to load. So technically, it would be cold but it warmed > > up during the run. > > What do you mean by long enough? During which run? The first run (with first run experience) for example. Should be cold, but wasn't. > > > I can verify this as follow up nonetheless. > > That would be great, thanks!
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tschumann@chromium.org, jkrcal@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2599743002/#ps260001 (title: "Improve variable names and comments.")
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
Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:
While running git apply --index -p1;
error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:312
error:
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:
patch does not apply
Patch:
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
index
4625b133e6d759f246aafcec3dd53b68e7adb4b4..3a98264f4f6ea7a61527048bb9bef5456f59bd7f
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
@@ -179,6 +179,16 @@ public class SnippetsBridge implements SuggestionsSource {
nativeOnNTPInitialized(mNativeSnippetsBridge);
}
+ public static void notifySchedulerAboutWarmResume() {
+ SnippetsBridge snippetsBridge = new
SnippetsBridge(Profile.getLastUsedProfile());
+ snippetsBridge.onActivityWarmResumed();
+ }
+
+ public static void notifySchedulerAboutColdStart() {
+ SnippetsBridge snippetsBridge = new
SnippetsBridge(Profile.getLastUsedProfile());
+ snippetsBridge.onColdStart();
+ }
+
public static void onSuggestionTargetVisited(int category, long
visitTimeMs) {
nativeOnSuggestionTargetVisited(category, visitTimeMs);
}
@@ -208,6 +218,16 @@ public class SnippetsBridge implements SuggestionsSource {
nativeFetch(mNativeSnippetsBridge, category, displayedSuggestionIds);
}
+ private void onActivityWarmResumed() {
+ assert mNativeSnippetsBridge != 0;
+ nativeOnActivityWarmResumed(mNativeSnippetsBridge);
+ }
+
+ private void onColdStart() {
+ assert mNativeSnippetsBridge != 0;
+ nativeOnColdStart(mNativeSnippetsBridge);
+ }
+
@CalledByNative
private static List<SnippetArticle> createSuggestionList() {
return new ArrayList<>();
@@ -312,6 +332,8 @@ public class SnippetsBridge implements SuggestionsSource {
long nativeNTPSnippetsBridge, int category, int position);
private native void nativeOnMoreButtonClicked(
long nativeNTPSnippetsBridge, int category, int position);
+ private native void nativeOnActivityWarmResumed(long
nativeNTPSnippetsBridge);
+ private native void nativeOnColdStart(long nativeNTPSnippetsBridge);
private static native void nativeOnSuggestionTargetVisited(int category,
long visitTimeMs);
private static native void nativeOnNTPInitialized(long
nativeNTPSnippetsBridge);
private native void nativeSetObserver(long nativeNTPSnippetsBridge,
SnippetsBridge bridge);
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, jkrcal@chromium.org, mvanouwerkerk@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2599743002/#ps280001 (title: "Rebase")
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": 280001, "attempt_start_ts": 1483650964519870,
"parent_rev": "61a02a639712b27b4d36836b0be912e4b254947b", "commit_rev":
"cb5e4457d0cef43f5a3676ee30139fe9be28eaa2"}
Message was sent while issue was closed.
Description was changed from ========== Trigger the Snippets Scheduler when Resuming Activities Inform the scheduler on resuming activites, so fetching of new snippets consider this event. (There is a related CL that triggers a similar event for new tabs: https://codereview.chromium.org/2592053004/) BUG=676308 ========== to ========== Trigger the Snippets Scheduler when Resuming Activities Inform the scheduler on resuming activites, so fetching of new snippets consider this event. (There is a related CL that triggers a similar event for new tabs: https://codereview.chromium.org/2592053004/) BUG=676308 Review-Url: https://codereview.chromium.org/2599743002 Cr-Commit-Position: refs/heads/master@{#441768} Committed: https://chromium.googlesource.com/chromium/src/+/cb5e4457d0cef43f5a3676ee3013... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001) as https://chromium.googlesource.com/chromium/src/+/cb5e4457d0cef43f5a3676ee3013... |
