Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
949293002:
Implement a poor man's PostAfterStartupTask() function. (Closed)
|
Created:
5 years, 10 months ago by michaeln Modified:
5 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+watch, nhiroki, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement a poor man's PostAfterStartupTask() function. The initial impl claims complete after the first page loads.
BUG=460265
Committed: https://crrev.com/96f887e253b8583cb5ea9dccda2aa796cdce2b98
Cr-Commit-Position: refs/heads/master@{#324950}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 3
Patch Set 3 : AfterStartupTaskPoster #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 1
Patch Set 7 : #
Total comments: 1
Patch Set 8 : #Patch Set 9 : #
Total comments: 29
Patch Set 10 : #Patch Set 11 : #
Total comments: 1
Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #
Total comments: 10
Patch Set 17 : #Patch Set 18 : #
Total comments: 2
Patch Set 19 : #
Total comments: 11
Patch Set 20 : #Patch Set 21 : #Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #Patch Set 25 : #Patch Set 26 : #
Total comments: 10
Patch Set 27 : #
Total comments: 22
Patch Set 28 : #Patch Set 29 : #Patch Set 30 : #
Total comments: 9
Patch Set 31 : #Patch Set 32 : #Patch Set 33 : #Patch Set 34 : #
Total comments: 2
Patch Set 35 : #
Total comments: 10
Patch Set 36 : #
Total comments: 4
Patch Set 37 : #Patch Set 38 : #Messages
Total messages: 77 (10 generated)
https://codereview.chromium.org/949293002/diff/1/content/browser/browser_main... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/949293002/diff/1/content/browser/browser_main... content/browser/browser_main_runner.cc:292: bool IsBrowserStartingUp() { The startup metrics component tracks some stats like this, but it's interpretation of 'starting up' is just how long till the main msg loop begins pumping msgs.
michaeln@chromium.org changed reviewers: + gab@chromium.org
Hi gab, ptal The criteria for 'is starting up' is not what it should be, I'm looking for some feedback on how to make that better.
On 2015/02/25 01:02:50, michaeln wrote: > Hi gab, ptal > > The criteria for 'is starting up' is not what it should be, I'm looking for some > feedback on how to make that better. Hey Michael, thanks for taking this on. IMO a good first 'is startup done' heuristic would be whether the first tab (or even ideally the foreground tab) has been loaded (we have metrics like SessionRestore.ForegroundTabLoaded which make me assume there are hooks in place to know when this occurred already). I like the idea of posting and re-posting with random delays, naturally spreading out the tasks. Instead of adding this inside each implementation though, perhaps it makes sense to introduce some kind of PostNonImportantPostStartupTask() which does this random post-startup delay magic for all callers that want this property (or could even just accumulate a pool of tasks and release them when notified via some registered callback that "startup" is done). Also, curious, which thread do these tasks run on at the moment? I would essentially eventually like everything to converge towards the BlockingPool which could then be made smarter with task types (IO, etc.), priorities (UI blocking, etc.), and phases (e.g. startup vs not). Would it be worth running an in-place experiment (similar to what you have now) to see how much there is to gain with instrumenting just a few spots and help justify a more advanced framework for post-startup tasks? Cheers, Gab
Thnx for looking. > Hey Michael, thanks for taking this on. > > IMO a good first 'is startup done' heuristic would be whether the first tab (or > even ideally the foreground tab) has been loaded (we have metrics like > SessionRestore.ForegroundTabLoaded which make me assume there are hooks in place > to know when this occurred already). SessionRestore.ForegroundTabLoaded, i'll look into that, sounds good for the predominant case. I think we have some some other cases where that may not work, those for which a foreground tab isn't loaded (think starting chrome for the sole purpose of spinning up and dispatching an event to a serviceworker). > I like the idea of posting and re-posting with random delays, naturally > spreading out the tasks. That was to address the concern peter mentioned about "at time T after startup my system bogs down". > Instead of adding this inside each implementation though, perhaps it makes sense > to introduce some kind of PostNonImportantPostStartupTask() which does this > random post-startup delay magic for all callers that want this property (or > could even just accumulate a pool of tasks and release them when notified via > some registered callback that "startup" is done). I was starting with the most basic primitive i could think of but i like the next step up the food chain PostAfterStartupTask idea to codify the pattern. So two functions bool IsBrowserStartingUp() and void PostAfterStartupTask(). > Also, curious, which thread do these tasks run on at the moment? I would The core logic for sw and appcache run directly on the IO thread, those system dish anything with blocking file IO off to other threads. The shiny new sw system uses the SequencedWorkerPool (SWP) for that, the crufty old appcache system is still using the FILE_USER_BLOCKING_THREAD (although it wouldn't be difficult to migrate over). DOMStorage (overhauled circa 2012) is using the SWP. I haven't retrofitted the quota system with this yet, there's at least one place in there to defer writing to its database. > essentially eventually like everything to converge towards the BlockingPool > which could then be made smarter with task types (IO, etc.), priorities (UI > blocking, etc.), and phases (e.g. startup vs not). +1 make greater use of the SWP in general and making that facility smarter. > Would it be worth running an in-place experiment (similar to what you have now) > to see how much there is to gain with instrumenting just a few spots and help > justify a more advanced framework for post-startup tasks? Experiments how? With many changes going into canary/dev to help speed up startup, isolating which changes are having how much impact might be difficult. > Cheers, > Gab
On 2015/02/25 20:15:10, michaeln wrote: > Thnx for looking. > > > Hey Michael, thanks for taking this on. > > > > IMO a good first 'is startup done' heuristic would be whether the first tab > (or > > even ideally the foreground tab) has been loaded (we have metrics like > > SessionRestore.ForegroundTabLoaded which make me assume there are hooks in > place > > to know when this occurred already). > > SessionRestore.ForegroundTabLoaded, i'll look into that, sounds good for the > predominant case. I think we have some some other cases where that may not work, > those for which a foreground tab isn't loaded (think starting chrome for the > sole purpose of spinning up and dispatching an event to a serviceworker). > > > I like the idea of posting and re-posting with random delays, naturally > > spreading out the tasks. > > That was to address the concern peter mentioned about "at time T after startup > my system bogs down". > > > Instead of adding this inside each implementation though, perhaps it makes > sense > > to introduce some kind of PostNonImportantPostStartupTask() which does this > > random post-startup delay magic for all callers that want this property (or > > could even just accumulate a pool of tasks and release them when notified via > > some registered callback that "startup" is done). > > I was starting with the most basic primitive i could think of but i like the > next step up the food chain PostAfterStartupTask idea to codify the pattern. So > two functions bool IsBrowserStartingUp() and void PostAfterStartupTask(). Okay, basic primitive makes sense to me wrapped in an experiment, otherwise it's essentially getting lost in the noise as you mentioned below. > > > Also, curious, which thread do these tasks run on at the moment? I would > > The core logic for sw and appcache run directly on the IO thread, those system > dish anything with blocking file IO off to other threads. The shiny new sw > system uses the SequencedWorkerPool (SWP) for that, the crufty old appcache > system is still using the FILE_USER_BLOCKING_THREAD (although it wouldn't be > difficult to migrate over). DOMStorage (overhauled circa 2012) is using the SWP. > I haven't retrofitted the quota system with this yet, there's at least one place > in there to defer writing to its database. > > > essentially eventually like everything to converge towards the BlockingPool > > which could then be made smarter with task types (IO, etc.), priorities (UI > > blocking, etc.), and phases (e.g. startup vs not). > > +1 make greater use of the SWP in general and making that facility smarter. > > > Would it be worth running an in-place experiment (similar to what you have > now) > > to see how much there is to gain with instrumenting just a few spots and help > > justify a more advanced framework for post-startup tasks? > > Experiments how? With many changes going into canary/dev to help speed up > startup, isolating which changes are having how much impact might be difficult. We have the LightSpeed experiment for that (see Alexei's recent post). The idea is to use Finch + the variations dashboard which give you A/B stats for an even split of the population on a given release (and even if other experiments are running it's okay because your A/B groups will get an even distribution of on/off for the other experiments). > > > Cheers, > > Gab
> We have the LightSpeed experiment for that (see Alexei's recent post). Perfecto! 598 // TODO(asvitkine): Experimental code for measuring start up impact of SB. 599 // Remove when experimentation is complete. http://crbug.com/450037 600 if (!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty()) 601 enable = false; I'll update the cl with 1) 'isstartingup' being based on ForegroundTabLoaded (+ a long'ish timeout) 2) provide a bool getter and a posttask wrapper 3) only turn on the delays via the lightspeed experiment ?) Will need to do something about tests. Test harnesses in general probably need to insist we'restartedupalreadydammit. Some individual tests will probably need to fiddle with that.
On 2015/02/25 21:22:33, michaeln wrote: > > We have the LightSpeed experiment for that (see Alexei's recent post). > > Perfecto! > 598 // TODO(asvitkine): Experimental code for measuring start up impact of > SB. > 599 // Remove when experimentation is complete. http://crbug.com/450037 > 600 if (!variations::GetVariationParamValue("LightSpeed", > "DisableSB").empty()) > 601 enable = false; > > I'll update the cl with > 1) 'isstartingup' being based on ForegroundTabLoaded (+ a long'ish timeout) > 2) provide a bool getter and a posttask wrapper > 3) only turn on the delays via the lightspeed experiment SGTM, do we even need the bool getter? i.e. if all we have is the post task wrapper, it could be smart enough to run the task right away if startup is done already. > > ?) Will need to do something about tests. Test harnesses in general probably > need to insist we'restartedupalreadydammit. Some individual tests will probably > need to fiddle with that. Good point. Not sure exactly what this means just yet, but it will probably become clear once you begin implementing and see what fails.
> SGTM, do we even need the bool getter? i.e. if all we have is the post task > wrapper, it could be smart enough to run the task right away if startup is done > already. At all my current callsites, i could live w/o the bool getter. It might be useful for stat gathering purposes?
On 2015/02/26 22:04:39, michaeln wrote: > > SGTM, do we even need the bool getter? i.e. if all we have is the post task > > wrapper, it could be smart enough to run the task right away if startup is > done > > already. > > At all my current callsites, i could live w/o the bool getter. It might be > useful for stat gathering purposes? Can you add a blank line as the second line of your description for Git compatibility?
cmumford@chromium.org changed reviewers: + cmumford@chromium.org
https://codereview.chromium.org/949293002/diff/20001/content/browser/appcache... File content/browser/appcache/appcache_update_job.cc (right): https://codereview.chromium.org/949293002/diff/20001/content/browser/appcache... content/browser/appcache/appcache_update_job.cc:464: FROM_HERE, base::TimeDelta::FromSeconds(base::RandInt(5, 15)), Constants for 5 & 15? Also (I don't know RandInt well), but do we need to see the random number generator to avoid getting the same value at startup? https://codereview.chromium.org/949293002/diff/20001/content/browser/browser_... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/949293002/diff/20001/content/browser/browser_... content/browser/browser_main_runner.cc:295: base::TimeDelta::FromSeconds(15); Assuming the same 15 from above? https://codereview.chromium.org/949293002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/949293002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:200: DCHECK(phase_ == START) << phase_; DCHECK_EQ. Also, couldn't you get here before START if startup was slow?
On 2015/02/26 22:04:39, michaeln wrote: > > SGTM, do we even need the bool getter? i.e. if all we have is the post task > > wrapper, it could be smart enough to run the task right away if startup is > done > > already. > > At all my current callsites, i could live w/o the bool getter. It might be > useful for stat gathering purposes? Hmmm, maybe if you can find stats you really care about that depend on this value in the callsite (and even then I'd argue that the RunTaskAfterStartup() should still handle everything and perhaps return true/false based on whether the task was handled synchronously). As an aside though I think stats wise it may makes sense to track how often "a" task was deferred, but probably not "which" task, i.e. whether it hits for a given task is kind of irrelevant IMO if the developer made the decision that it should never run on startup, using this helper makes it explicit (protecting against future changes as desired) and whether it hits or not in practice doesn't add overhead and doesn't change the justification for it being around.
Declaring when startup is complete has to be a function of chrome, the content
lib doesn't have enough context to make that call (ala session restore). The
public content api needs to support this in some way. Important things about
that interface:
1. should be optional so not all content lib embedders need to change to support
it
2. should be usable from any thread
#2 means it shouldn't be a new ContentBrowserClient virtual method with a
default impl of posting immediately.
Here's one option:
Define a new StartupTaskPoster abstract iterface in the public content api. Set
a global in the content lib to an instance early on. Implement a static
PostAfterStartupTask() method in the BrowserThread. Have an additional
BrowserThread method to SetStartupTaskPoster(...). If the embedder hasn't set
one by a well define point in the startup process, one that posts immediately is
used.
[yup] class StartupTaskPoster {
virtual void PostAfterStartupTask(...) = 0
};
class BrowserThread {
[nah] static void SetStartupTaskPoster(poster) { if (!global) global = poster;
}
[yup] static PostAfterStartupTask(...) { if (global)
global->PostAfterStartupTask() else runner->PostTask(); }
};
Another options:
[yup] class ContentBrowserClient {
virtual StartupTaskPoster& GetStartupTaskPoster() { return an impl that runs
tasks immediately by default; }
}
Use that early on during init to set the global. Prior to the global being set
with the browser client's poster, BrowserThread::PostAfterStartupTask() defers.
Another option:
[nah] Provide a static Get/SetIsBrowserStartingUp methods on BrowserThread, the
initial value is kMaybe. To defer tasks, the embedder should set it to kYes
early on otherwise the content lib will switch it on at some well defined point.
--------------------
Here's the additions i think i like to the content public api:
class BrowserThread {
static PostAfterStartupTask(...);
// also some way to short circuit for testing (maybe CBC method is enough?)
};
class StartupTaskPoster {
virtual void PostAfterStartupTask(...) = 0;
};
class ContentBrowserClient {
virtual StartupTaskPoster& GetStartupTaskPoster() { return an impl that runs
tasks immediately by default; }
};
BrowserMainLoop::PreCreateThreads() looks like a reasonable place get the poster from the client and set it for the content lib.
ptal, here's a cut at an interface and default no-delay impl along the lines of my previous comments. Once the content/public aspects are ok'd, I'll dig into a chromium impl that actually delays. Beyond that, I'll be splitting this up into multiple CLs for landing... 1. content public api w default impl and hooking into browser main 2. chrome/ specific impl that really does delay 3. individual CLs to use it for different cases and getting things to work in our test harnesses https://codereview.chromium.org/949293002/diff/100001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/100001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:806: // todo: write me I'll flesh this out after the content public api is ok'd
Question (disclaimer I'm no content vs chrome expert): why does chrome even need to be involved? Isn't content all about multi-tasking/rendering (i.e. it should have all the information to figure out how to post tasks and know when startup is complete (as long as that's defined in terms of abstract concepts like "foreground tab loaded" and not Chrome concepts like "session restore"). Otherwise regarding your proposal (if we really need to get chrome involved), content already interacts with Chrome over a bunch of delegates, could we provide a delegate with a default impl which may be overridden by a chrome specific one? Could we maybe even add that to an existing delegate rather than provide a new one? (just thoughts, I lack expertise to tell the best way of bridging content and chrome for this specific use case). Thanks! Gab
On 2015/03/03 13:50:53, gab wrote: > Question (disclaimer I'm no content vs chrome expert): why does chrome even need > to be involved? Isn't content all about multi-tasking/rendering (i.e. it should > have all the information to figure out how to post tasks and know when startup > is complete (as long as that's defined in terms of abstract concepts like > "foreground tab loaded" and not Chrome concepts like "session restore"). I think it belongs in chrome. Ideally, it should be in terms of things like "session restore", many (all?) of the metrics we gather now related to when we're done starting are gathered up in chrome. Stuff to look at... ChromeBrowserMainParts::PostBrowserStart() FirstWebContentsProfiler::DocumentOnLoadCompletedInMainFrame() TabLoader::Observe() [session_restore.cc] TabLoader::HandleTabClosedOrLoaded() [session_restore.cc] The content lib has no context like this. It's great at understanding what's happening to an individual WebContent, but not so much on the bigger picture. > Otherwise regarding your proposal (if we really need to get chrome involved), > content already interacts with Chrome over a bunch of delegates, could we > provide a delegate with a default impl which may be overridden by a chrome > specific one? Could we maybe even add that to an existing delegate rather than > provide a new one? (just thoughts, I lack expertise to tell the best way of > bridging content and chrome for this specific use case). I think I've done something close to that. The existing delegate that's been extended is ContentBrowserClient. The difference is that I didn't put the new task posting methods directly on that interface, instead it provides a pointer to a separate class with the new methods. I haven't gotten to testing yet, but i think the separate class will make it easier to inject a task poster suitable for particular tests, and to compose unittests w/o having to create a ContentBrowserClient at all. The ContentBrowserClient doc comments say // The methods are assumed to be called on the UI // thread unless otherwise specified. Use this "escape hatch" sparingly I'll ping jam about the details of the chrome/content interface.
On 2015/03/03 19:52:45, michaeln wrote: > On 2015/03/03 13:50:53, gab wrote: > > Question (disclaimer I'm no content vs chrome expert): why does chrome even > need > > to be involved? Isn't content all about multi-tasking/rendering (i.e. it > should > > have all the information to figure out how to post tasks and know when startup > > is complete (as long as that's defined in terms of abstract concepts like > > "foreground tab loaded" and not Chrome concepts like "session restore"). > > I think it belongs in chrome. Ideally, it should be in terms of things like > "session restore", many (all?) of the metrics we gather now related to when > we're done starting are gathered up in chrome. Stuff to look at... > > ChromeBrowserMainParts::PostBrowserStart() > FirstWebContentsProfiler::DocumentOnLoadCompletedInMainFrame() > TabLoader::Observe() [session_restore.cc] > TabLoader::HandleTabClosedOrLoaded() [session_restore.cc] > > The content lib has no context like this. It's great at understanding what's > happening to an individual WebContent, but not so much on the bigger picture. > > > Otherwise regarding your proposal (if we really need to get chrome involved), > > content already interacts with Chrome over a bunch of delegates, could we > > provide a delegate with a default impl which may be overridden by a chrome > > specific one? Could we maybe even add that to an existing delegate rather than > > provide a new one? (just thoughts, I lack expertise to tell the best way of > > bridging content and chrome for this specific use case). > > I think I've done something close to that. The existing delegate that's been > extended is ContentBrowserClient. The difference is that I didn't put the new > task posting methods directly on that interface, instead it provides a pointer > to a separate class with the new methods. I haven't gotten to testing yet, but i > think the separate class will make it easier to inject a task poster suitable > for particular tests, and to compose unittests w/o having to create a > ContentBrowserClient at all. > > The ContentBrowserClient doc comments say > // The methods are assumed to be called on the UI > // thread unless otherwise specified. Use this "escape hatch" sparingly > > I'll ping jam about the details of the chrome/content interface. Okay SG, let me know what the OWNERS think and I'll take another look then.
> > I'll ping jam about the details of the chrome/content interface. > > Okay SG, let me know what the OWNERS think and I'll take another look then. I'll try paring the interface down to just ContentBrowserClient::PostAfterStartupTask() for now, it can always be pulled out later if that would help. I'm still not sure what to use a the signal for being done starting up? I'm thinking something like this: a. If no tabs are loading at PostBrowserStart() time, claim started right then. b. If SessionRestore is happening, wait till <m> tabs are loaded, where me counts the first few tabs of a multi-tab restore. c. If SessionRestore is not happening, wait till FirstWebContents.MainFrameLoad. f. As a fallback, start an <n> minute timer in ChromeBrowserMainParts::PostBrowserStart() in case the above never produce a signal.
On 2015/03/03 20:26:48, michaeln wrote: > > > I'll ping jam about the details of the chrome/content interface. > > > > Okay SG, let me know what the OWNERS think and I'll take another look then. > > I'll try paring the interface down to just > ContentBrowserClient::PostAfterStartupTask() for now, it can always be pulled > out later if that would help. SGTM > > I'm still not sure what to use a the signal for being done starting up? I'm > thinking something like this: > > a. If no tabs are loading at PostBrowserStart() time, claim started right then. > > b. If SessionRestore is happening, wait till <m> tabs are loaded, where me > counts the first few tabs of a multi-tab restore. (or only the foreground tabs -- some tabs may never be loaded depending on resource constraints, depends on the platform) > > c. If SessionRestore is not happening, wait till FirstWebContents.MainFrameLoad. > If we just have a way to know when the foreground most tab is loaded, I'd be happy with that for now. > f. As a fallback, start an <n> minute timer in > ChromeBrowserMainParts::PostBrowserStart() in case the above never produce a > signal. Yea that will probably be required for things like background mode (I like a long timer, e.g. 5 minutes, to avoid kicking this off as the user opens Chrome right after it auto-launched in background mode on OS login).
michaeln@chromium.org changed reviewers: + jam@chromium.org
@jam, can you take a look at the content public api?
https://codereview.chromium.org/949293002/diff/120001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/120001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:817: // TODO(michaeln): write me It would be clearer while reviewing this cl if this was implemented.
> It would be clearer while reviewing this cl if this was implemented. Ok, ptal, i've stripped this cl down to implementing the PostAfterStartupTask. The initial impl watches for the first page load and does not take session restore into account. I'll fixup the cl title and description. Also I'll make seperate cls for using this at different callsites once this lands.
I'll let jam weigh in on the interface, looks reasonable in general IMO, a few thoughts below. https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:229: using StartupCompleteFlag = base::CancellationFlag; Feels weird to use "CancellationFlag" here although it does exactly what we need... I guess the next closest thing would be a WaitableEvent(true, false); which would be slightly more heavy weight. Perhaps CancellationFlag should be repurposed to be non-"Cancellation" specific (not in this CL, but just a thought for the Chrome owners to weigh). https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1140: base::TimeDelta::FromMinutes(3)); Is PostBrowserStart() called in background mode launches (i.e. before an explicit Chrome launch)? I would guess not. I assume PostMainMessageLoopStart() would be called though even if in background mode, this would thus be a better place for this code IMO. https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:838: PostAfterStartupTaskImpl(from_here, task_runner, task); Why does this need an explicit impl? i.e. why not handle the call right here? https://codereview.chromium.org/949293002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:292: chrome::SetBrowserStartupIsComplete(); Doesn't feel like ExtraPartsMetrics is the right place for this as it doesn't have to do with metrics.
thnx for looking https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:229: using StartupCompleteFlag = base::CancellationFlag; > Perhaps CancellationFlag should be repurposed to be non-"Cancellation" specific > (not in this CL, but just a thought for the Chrome owners to weigh). ^^^ this https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1131: base::Bind(&WebRtcLogUtil::DeleteOldWebRtcLogFilesForAllProfiles), This might be a good candidate for PostAfterStartupTask(). https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1140: base::TimeDelta::FromMinutes(3)); On 2015/03/10 15:00:32, gab wrote: > Is PostBrowserStart() called in background mode launches (i.e. before an > explicit Chrome launch)? I would guess not. > > I assume PostMainMessageLoopStart() would be called though even if in background > mode, this would thus be a better place for this code IMO. I'm really not sure about which of these methods gets called when and on which platforms? The NOTREACHED in PostMainMessageLoopStart tells me its not used on android. Any guidance i can get about where to best place the failsafe and where to signal 'complete' would be great. Maybe the failsafe timer should start earlier in the startup process, as soon as BrowserThread::PostDelayedTask(UI...) can be called? https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:838: PostAfterStartupTaskImpl(from_here, task_runner, task); On 2015/03/10 15:00:32, gab wrote: > Why does this need an explicit impl? i.e. why not handle the call right here? good question, this method can be called on anythread and in the called before startup case the bind closure wants a weakptr to 'this', but that runs afould of weakptr thread safefy checks https://codereview.chromium.org/949293002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:292: chrome::SetBrowserStartupIsComplete(); On 2015/03/10 15:00:32, gab wrote: > Doesn't feel like ExtraPartsMetrics is the right place for this as it doesn't > have to do with metrics. Ditto any guidance. I picked this location because this class and the FirstWebContentsProfiler are responsible for the Startup.FirstWebContents.MainFrameLoad stat. That's exactly what i'd like to consider 'isStartedUp' for now. These classes are also responsible for how that stat is different on android (it's not logged there), so its more clear why/what is different about the the startup compeltion flag for android too.
https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:229: using StartupCompleteFlag = base::CancellationFlag; On 2015/03/10 19:46:26, michaeln wrote: > > Perhaps CancellationFlag should be repurposed to be non-"Cancellation" > specific > > (not in this CL, but just a thought for the Chrome owners to weigh). > > ^^^ this One simple way to do this would be to rename the class, but add a using CancellationFlag = NewFlagClassName; at the top of the header to avoid having to rename every user just yet if ever. https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1140: base::TimeDelta::FromMinutes(3)); On 2015/03/10 19:46:26, michaeln wrote: > On 2015/03/10 15:00:32, gab wrote: > > Is PostBrowserStart() called in background mode launches (i.e. before an > > explicit Chrome launch)? I would guess not. > > > > I assume PostMainMessageLoopStart() would be called though even if in > background > > mode, this would thus be a better place for this code IMO. > > I'm really not sure about which of these methods gets called when and on which > platforms? The NOTREACHED in PostMainMessageLoopStart tells me its not used on > android. Any guidance i can get about where to best place the failsafe and where > to signal 'complete' would be great. > > Maybe the failsafe timer should start earlier in the startup process, as soon as > BrowserThread::PostDelayedTask(UI...) can be called? Makes sense to post it in a safe and early spot IMO since it's a long timer anyways. https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:838: PostAfterStartupTaskImpl(from_here, task_runner, task); On 2015/03/10 19:46:26, michaeln wrote: > On 2015/03/10 15:00:32, gab wrote: > > Why does this need an explicit impl? i.e. why not handle the call right here? > > good question, this method can be called on anythread and in the called before > startup case the bind closure wants a weakptr to 'this', but that runs afould of > weakptr thread safefy checks ChromeContentBrowserClient has a weak_factory_, can't you just use weak_factory_.GetWeakPtr() for |this|? (and also clarify in the API that this tasks will exhibit SKIP_ON_SHUTDOWN behavior (as any delayed task do in TaskRunners FWIW)) https://codereview.chromium.org/949293002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:292: chrome::SetBrowserStartupIsComplete(); On 2015/03/10 19:46:26, michaeln wrote: > On 2015/03/10 15:00:32, gab wrote: > > Doesn't feel like ExtraPartsMetrics is the right place for this as it doesn't > > have to do with metrics. > > Ditto any guidance. > > I picked this location because this class and the FirstWebContentsProfiler are > responsible for the Startup.FirstWebContents.MainFrameLoad stat. That's exactly > what i'd like to consider 'isStartedUp' for now. These classes are also > responsible for how that stat is different on android (it's not logged there), > so its more clear why/what is different about the the startup compeltion flag > for android too. Then perhaps put it directly in FirstWebContentsProfiler besides the metric logging? Makes it feel like that's repurposing FirstWebContentsProfiler a bit beyond strictly "profiling", but that's not too bad IMO (we can rename the class later if we care).
> > Maybe the failsafe timer should start earlier in the startup process, as soon > as > > BrowserThread::PostDelayedTask(UI...) can be called? > > Makes sense to post it in a safe and early spot IMO since it's a long timer > anyways. I'll look more closely at this set of BrowserMainParts methods? > > good question, this method can be called on anythread and in the called before > > startup case the bind closure wants a weakptr to 'this', but that runs afould > of > > weakptr thread safefy checks > > ChromeContentBrowserClient has a weak_factory_, can't you just use > weak_factory_.GetWeakPtr() for |this|? nope, weakptrs have thread affinity https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... > (and also clarify in the API that this tasks will exhibit SKIP_ON_SHUTDOWN > behavior (as any delayed task do in TaskRunners FWIW)) will do > Then perhaps put it directly in FirstWebContentsProfiler besides the metric > logging? Makes it feel like that's repurposing FirstWebContentsProfiler a bit > beyond strictly "profiling", but that's not too bad IMO (we can rename the class > later if we care). Aha... maybe i should make a new 'extrapart' for this purpose which does something very similar to what ExtraPartsMetrics + FirstWebContentsProfiler do do observe the first page load completion? Keeps this decoupled from that and it might help when it comes to making the criteria for 'isStartedUp' more sophistcated (session restore + measure of ui thread intractability). A place to start the failsafe timer could be this extra parts ctor. I think i want to make a chrome browser test to verify the flag gets set prior to the failsafe timeout in normal circumstances. Thnx for looking! I think i have enough to prep a new cl,
https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1135: // Failsafe for signaling startup completion. why do we need a failsafe? under what circumstance would a page load never happen and we want these delayed tasks to run? https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.h (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.h:185: namespace chrome { nit: chrome namespace isn't used anymore (see chromium-dev threads about it) since it's a leaf node https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:830: task_runner->PostTask(from_here, task); It seems simpler that the code below would pass all these tasks to the same header that has IsStartupComplete, and that code would accumulate all the tasks into a vector that it then runs when it's been told that startup is complete https://codereview.chromium.org/949293002/diff/160001/content/browser/browser... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/949293002/diff/160001/content/browser/browser... content/browser/browser_thread_impl.cc:19: #include "content/public/common/content_client.h" nit: by convention we only include content_browser_client.h, since it includes content_client.h (same for all the other content_foo_client.h) https://codereview.chromium.org/949293002/diff/160001/content/public/browser/... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/949293002/diff/160001/content/public/browser/... content/public/browser/browser_thread.h:187: // For use with scheduling non-critical tasks for exeuction after startup. nit: spelling https://codereview.chromium.org/949293002/diff/160001/content/public/browser/... content/public/browser/browser_thread.h:192: // task immediately. nit: i think the "Note" part is confusing. it starts by making it seem like if the CBC method isn't implemented, this method doesn't work. then it says it has default behavior.
https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1135: // Failsafe for signaling startup completion. On 2015/03/10 23:04:34, jam wrote: > why do we need a failsafe? > > under what circumstance would a page load never happen and we want these delayed > tasks to run? The first page is closed prior to getting to the loaded state. Chrome is being started headless just to run ServiceWorkers for sync'ing purposes so no page is loaded. I may be thinking ahead toward hooking this into session restore too much, but the notion of when the browser is started up is not well defined, the sum of some set of signals == started up. I want to make it easy/safe to experiment with exactly what constitutes 'started up' and I'm wary of corner cases that might prevent a signal. https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.h (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.h:185: namespace chrome { On 2015/03/10 23:04:34, jam wrote: > nit: chrome namespace isn't used anymore (see chromium-dev threads about it) > since it's a leaf node that's too bad, chrome::IsBrowserStartupComplete() reads nicely https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:830: task_runner->PostTask(from_here, task); On 2015/03/10 23:04:34, jam wrote: > It seems simpler that the code below would pass all these tasks to the same > header that has IsStartupComplete, and that code would accumulate all the tasks > into a vector that it then runs when it's been told that startup is complete Is it? We'd need a thread safe collection somewhere to hold the tasks. Alloc'ing storage for the collection and then synchronizing access to it seemed less simple to me. Having said that, i'd be fine with switching over to something along those lines, but it's not an obvious improvement to me. https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:838: PostAfterStartupTaskImpl(from_here, task_runner, task); On 2015/03/10 19:55:27, gab wrote: > On 2015/03/10 19:46:26, michaeln wrote: > > On 2015/03/10 15:00:32, gab wrote: > > > Why does this need an explicit impl? i.e. why not handle the call right > here? > > > > good question, this method can be called on anythread and in the called before > > startup case the bind closure wants a weakptr to 'this', but that runs afould > of > > weakptr thread safefy checks > > ChromeContentBrowserClient has a weak_factory_, can't you just use > weak_factory_.GetWeakPtr() for |this|? > > (and also clarify in the API that this tasks will exhibit SKIP_ON_SHUTDOWN > behavior (as any delayed task do in TaskRunners FWIW)) nope, weakptrs have thread affinity https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
On 2015/03/10 at 23:54:54, michaeln wrote: > https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:1135: // Failsafe for signaling startup completion. > On 2015/03/10 23:04:34, jam wrote: > > why do we need a failsafe? > > > > under what circumstance would a page load never happen and we want these delayed > > tasks to run? > > The first page is closed prior to getting to the loaded state. > > Chrome is being started headless just to run ServiceWorkers for sync'ing purposes so no page is loaded. > > I may be thinking ahead toward hooking this into session restore too much, but the notion of when the browser is started up is not well defined, the sum of some set of signals == started up. I want to make it easy/safe to experiment with exactly what constitutes 'started up' and I'm wary of corner cases that might prevent a signal. > > https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.h (right): > > https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.h:185: namespace chrome { > On 2015/03/10 23:04:34, jam wrote: > > nit: chrome namespace isn't used anymore (see chromium-dev threads about it) > > since it's a leaf node > > that's too bad, chrome::IsBrowserStartupComplete() reads nicely > > https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... > File chrome/browser/chrome_content_browser_client.cc (right): > > https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... > chrome/browser/chrome_content_browser_client.cc:830: task_runner->PostTask(from_here, task); > On 2015/03/10 23:04:34, jam wrote: > > It seems simpler that the code below would pass all these tasks to the same > > header that has IsStartupComplete, and that code would accumulate all the tasks > > into a vector that it then runs when it's been told that startup is complete > > Is it? We'd need a thread safe collection somewhere to hold the tasks. Alloc'ing storage for the collection and then synchronizing access to it seemed less simple to me. Having said that, i'd be fine with switching over to something along those lines, but it's not an obvious improvement to me. > > https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... > chrome/browser/chrome_content_browser_client.cc:838: PostAfterStartupTaskImpl(from_here, task_runner, task); > On 2015/03/10 19:55:27, gab wrote: > > On 2015/03/10 19:46:26, michaeln wrote: > > > On 2015/03/10 15:00:32, gab wrote: > > > > Why does this need an explicit impl? i.e. why not handle the call right > > here? > > > > > > good question, this method can be called on anythread and in the called before > > > startup case the bind closure wants a weakptr to 'this', but that runs afould > > of > > > weakptr thread safefy checks > > > > ChromeContentBrowserClient has a weak_factory_, can't you just use > > weak_factory_.GetWeakPtr() for |this|? > > > > (and also clarify in the API that this tasks will exhibit SKIP_ON_SHUTDOWN > > behavior (as any delayed task do in TaskRunners FWIW)) > > nope, weakptrs have thread affinity > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... Also, you should also run "git cl format" and "git cl lint" and fixup recommendations.
ptal > Also, you should also run "git cl format" and "git cl lint" and fixup > recommendations. Done https://codereview.chromium.org/949293002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:292: chrome::SetBrowserStartupIsComplete(); > Then perhaps put it directly in FirstWebContentsProfiler besides the metric > logging? Makes it feel like that's repurposing FirstWebContentsProfiler a bit > beyond strictly "profiling", but that's not too bad IMO (we can rename the class > later if we care). I put it directly into ChromeBrowserMainParts, nothing extra needed. https://codereview.chromium.org/949293002/diff/160001/content/browser/browser... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/949293002/diff/160001/content/browser/browser... content/browser/browser_thread_impl.cc:19: #include "content/public/common/content_client.h" On 2015/03/10 23:04:34, jam wrote: > nit: by convention we only include content_browser_client.h, since it includes > content_client.h (same for all the other content_foo_client.h) Done. https://codereview.chromium.org/949293002/diff/160001/content/public/browser/... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/949293002/diff/160001/content/public/browser/... content/public/browser/browser_thread.h:192: // task immediately. On 2015/03/10 23:04:34, jam wrote: > nit: i think the "Note" part is confusing. it starts by making it seem like if > the CBC method isn't implemented, this method doesn't work. then it says it has > default behavior. Done. // Note: see related ContentBrowserClient.PostAfterStartupTask. https://codereview.chromium.org/949293002/diff/200001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/200001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:240: static void SetBrowserStartupIsComplete(); i'll probably have to expose SetBrowserStartupIsComplete for testing
> https://codereview.chromium.org/949293002/diff/200001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:240: static void > SetBrowserStartupIsComplete(); > i'll probably have to expose SetBrowserStartupIsComplete for testing Maybe not, tests look ok in snapshot 11.
@jam, ping https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:269: #if !defined(OS_ANDROID) FUD: Initially mimic'ing chrome_browser_main_extra_parts_metrics.cc which avoids monitoring the first page load on android, although i'm not sure why it does.
https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:830: task_runner->PostTask(from_here, task); On 2015/03/10 23:54:54, michaeln wrote: > On 2015/03/10 23:04:34, jam wrote: > > It seems simpler that the code below would pass all these tasks to the same > > header that has IsStartupComplete, and that code would accumulate all the > tasks > > into a vector that it then runs when it's been told that startup is complete > > Is it? We'd need a thread safe collection somewhere to hold the tasks. sure, but given that only a few places should/will call this, using a lock won't be expensive. > Alloc'ing > storage for the collection and then synchronizing access to it seemed less > simple to me. Having said that, i'd be fine with switching over to something > along those lines, but it's not an obvious improvement to me. I would prefer seeing that because I think reposting every task after a random 5-30s interval is extra complexity which isn't clear to me that it's needed https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:241: class StartupObserver : public WebContentsObserver { nit: this would be much smaller lines if the methods were inlined.. https://codereview.chromium.org/949293002/diff/300001/content/public/browser/... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/949293002/diff/300001/content/public/browser/... content/public/browser/browser_thread.h:191: // Note: see related ContentBrowserClient.PostAfterStartupTask. nit: s/./::
https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:262: }; Not sure: Is DISALLOW_COPY_AND_ASSIGN required for local *.cc classes? https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:266: int kShortDelaySecs = 3; const int kShortDelaySecs = 3; https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:303: if (web_contents()->GetMainFrame() == render_frame_host) The rest of Chrome does if (!render_frame_host->GetParent()). Not sure if there's a difference?
https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:830: task_runner->PostTask(from_here, task); On 2015/03/23 23:19:44, jam wrote: > On 2015/03/10 23:54:54, michaeln wrote: > > On 2015/03/10 23:04:34, jam wrote: > > > It seems simpler that the code below would pass all these tasks to the same > > > header that has IsStartupComplete, and that code would accumulate all the > > tasks > > > into a vector that it then runs when it's been told that startup is complete > > > > Is it? We'd need a thread safe collection somewhere to hold the tasks. > > sure, but given that only a few places should/will call this, using a lock won't > be expensive. > > > Alloc'ing > > storage for the collection and then synchronizing access to it seemed less > > simple to me. Having said that, i'd be fine with switching over to something > > along those lines, but it's not an obvious improvement to me. > > I would prefer seeing that because I think reposting every task after a random > 5-30s interval is extra complexity which isn't clear to me that it's needed Done. There is still a polling timer but only one for all of these tasks. Also avoided using a lock. https://codereview.chromium.org/949293002/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:838: PostAfterStartupTaskImpl(from_here, task_runner, task); On 2015/03/10 23:54:54, michaeln wrote: > On 2015/03/10 19:55:27, gab wrote: > > On 2015/03/10 19:46:26, michaeln wrote: > > > On 2015/03/10 15:00:32, gab wrote: > > > > Why does this need an explicit impl? i.e. why not handle the call right > > here? > > > > > > good question, this method can be called on anythread and in the called > before > > > startup case the bind closure wants a weakptr to 'this', but that runs > afould > > of > > > weakptr thread safefy checks > > > > ChromeContentBrowserClient has a weak_factory_, can't you just use > > weak_factory_.GetWeakPtr() for |this|? > > > > (and also clarify in the API that this tasks will exhibit SKIP_ON_SHUTDOWN > > behavior (as any delayed task do in TaskRunners FWIW)) > > nope, weakptrs have thread affinity > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... initialized a weak_this_ ptr in the constructor https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:241: class StartupObserver : public WebContentsObserver { On 2015/03/23 23:19:44, jam wrote: > nit: this would be much smaller lines if the methods were inlined.. Done. https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:262: }; On 2015/03/23 23:57:20, cmumford wrote: > Not sure: Is DISALLOW_COPY_AND_ASSIGN required for local *.cc classes? Done. https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:266: int kShortDelaySecs = 3; On 2015/03/23 23:57:19, cmumford wrote: > const int kShortDelaySecs = 3; Done. https://codereview.chromium.org/949293002/diff/300001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:303: if (web_contents()->GetMainFrame() == render_frame_host) On 2015/03/23 23:57:19, cmumford wrote: > The rest of Chrome does if (!render_frame_host->GetParent()). Not sure if > there's a difference? thnx, done
https://codereview.chromium.org/949293002/diff/340001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/340001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2612: after_startup_tasks_.clear(); Oh, I think there's a problem with this? I think it requires that any refcounted objects bound up in the 'task' given to PostAfterStartupTask to have a threadsafe refcount because the final ref could be released on this thread rather than the target thread. It's unlikely given the kMinDelay to kMaxDelay involved. I'll massage it to avoid that problem.
https://codereview.chromium.org/949293002/diff/340001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/340001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2612: after_startup_tasks_.clear(); On 2015/03/25 01:09:32, michaeln wrote: > Oh, I think there's a problem with this? I think it requires that any refcounted > objects bound up in the 'task' given to PostAfterStartupTask to have a > threadsafe refcount because the final ref could be released on this thread > rather than the target thread. It's unlikely given the kMinDelay to kMaxDelay > involved. > > I'll massage it to avoid that problem. Done
https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:238: g_startup_complete_flag.Get().Set(); what I had meant earlier (sorry if not clear), is that this would call a method on ChromeContentBrowserClient to notify it that startup is complete. That way CCBC doesn't need to poll. https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2625: const int kMaxDelay = 30; What is the purpose of this random delay? https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.h (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:307: nit: extra line
https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:238: g_startup_complete_flag.Get().Set(); On 2015/03/26 15:38:58, jam wrote: > what I had meant earlier (sorry if not clear), is that this would call a method > on ChromeContentBrowserClient to notify it that startup is complete. That way > CCBC doesn't need to poll. I've been trying to avoid providing an API to listen for the exact moment startup completes to avoid having a bunch of stuff initiated at the same time. Having a call into the CCBC here would continue to avoid that kind of API. OK. I'm also trying to stick to the guidance provided here... #if defined(CONTENT_IMPLEMENTATION) // Content's embedder API should only be used by content. ContentClient* GetContentClient(); #endif ... which makes it tricky to get a ptr to the CCBC and discourages doing that. Right now (afaict), chrome/ doesn't call the CCBC instance directly anywhere, but there are two places a static method is used. How about I add a static CCBC method and call it here. And setup a global instance ptr inside chrome_content_browser_client.cc so that static method can find the instance? https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2625: const int kMaxDelay = 30; On 2015/03/26 15:38:58, jam wrote: > What is the purpose of this random delay? To avoid initiating a bunch of work at the same time. Wouldn't want to tie up threads with a bunches of back to back tasks that aren't tied directly to a user action. I think ideally the time span could be a function of how many there are to run.
https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2625: const int kMaxDelay = 30; On 2015/03/26 19:52:33, michaeln wrote: > On 2015/03/26 15:38:58, jam wrote: > > What is the purpose of this random delay? > > To avoid initiating a bunch of work at the same time. Wouldn't want to tie up > threads with a bunches of back to back tasks that aren't tied directly to a user > action. I think ideally the time span could be a function of how many there are > to run. I'll compute the delay more directly instead of randomizing it. Probably some benefit to keeping the ordering.
https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:238: g_startup_complete_flag.Get().Set(); On 2015/03/26 19:52:33, michaeln wrote: > On 2015/03/26 15:38:58, jam wrote: > > what I had meant earlier (sorry if not clear), is that this would call a > method > > on ChromeContentBrowserClient to notify it that startup is complete. That way > > CCBC doesn't need to poll. > > I've been trying to avoid providing an API to listen for the exact moment > startup completes to avoid having a bunch of stuff initiated at the same time. > Having a call into the CCBC here would continue to avoid that kind of API. OK. > > I'm also trying to stick to the guidance provided here... > > #if defined(CONTENT_IMPLEMENTATION) > // Content's embedder API should only be used by content. > ContentClient* GetContentClient(); > #endif > > ... which makes it tricky to get a ptr to the CCBC and discourages doing that. > > Right now (afaict), chrome/ doesn't call the CCBC instance directly anywhere, > but there are two places a static method is used. > > How about I add a static CCBC method and call it here. And setup a global > instance ptr inside chrome_content_browser_client.cc so that static method can > find the instance? > Why doesn't CCBC forward the tasks it gets to a file that has the startup code that you're adding? i.e. take as much logic out of CCBC, which is already too big. https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2625: const int kMaxDelay = 30; On 2015/03/26 19:52:33, michaeln wrote: > On 2015/03/26 15:38:58, jam wrote: > > What is the purpose of this random delay? > > To avoid initiating a bunch of work at the same time. Wouldn't want to tie up > threads with a bunches of back to back tasks that aren't tied directly to a user > action. I think ideally the time span could be a function of how many there are > to run. hmm, I understand the motivation but it's not clear that this is going to make a difference.
https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:238: g_startup_complete_flag.Get().Set(); On 2015/03/26 20:22:08, jam wrote: > On 2015/03/26 19:52:33, michaeln wrote: > > On 2015/03/26 15:38:58, jam wrote: > > > what I had meant earlier (sorry if not clear), is that this would call a > > method > > > on ChromeContentBrowserClient to notify it that startup is complete. That > way > > > CCBC doesn't need to poll. > > > > I've been trying to avoid providing an API to listen for the exact moment > > startup completes to avoid having a bunch of stuff initiated at the same time. > > Having a call into the CCBC here would continue to avoid that kind of API. OK. > > > > I'm also trying to stick to the guidance provided here... > > > > #if defined(CONTENT_IMPLEMENTATION) > > // Content's embedder API should only be used by content. > > ContentClient* GetContentClient(); > > #endif > > > > ... which makes it tricky to get a ptr to the CCBC and discourages doing that. > > > > Right now (afaict), chrome/ doesn't call the CCBC instance directly anywhere, > > but there are two places a static method is used. > > > > How about I add a static CCBC method and call it here. And setup a global > > instance ptr inside chrome_content_browser_client.cc so that static method can > > find the instance? > > > > Why doesn't CCBC forward the tasks it gets to a file that has the startup code > that you're adding? i.e. take as much logic out of CCBC, which is already too > big. I guess I was trying to avoid the addition of a new class and .h .cc files. I'll have this class and the CCBC utilize a new class and put this startup monitoring code and the task posting code in there. https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2625: const int kMaxDelay = 30; On 2015/03/26 20:22:08, jam wrote: > On 2015/03/26 19:52:33, michaeln wrote: > > On 2015/03/26 15:38:58, jam wrote: > > > What is the purpose of this random delay? > > > > To avoid initiating a bunch of work at the same time. Wouldn't want to tie up > > threads with a bunches of back to back tasks that aren't tied directly to a > user > > action. I think ideally the time span could be a function of how many there > are > > to run. > > hmm, I understand the motivation but it's not clear that this is going to make a > difference. And if the list were very large, what would your gut say then? Avoiding a big hit at time 0 was one of the issues raised on the mailing list about this approach. All of the tasks I have in mind for this involve disk and/or network IO. As a user of this api, i think I'd prefer having them spread out a little so I wouldn't have to ensure that elsewhere. The call to RandInt was a way to do that with minimal amount of code. I'm not real comfortable with running them at the same time.
https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/949293002/diff/360001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2625: const int kMaxDelay = 30; On 2015/03/26 21:39:36, michaeln wrote: > On 2015/03/26 20:22:08, jam wrote: > > On 2015/03/26 19:52:33, michaeln wrote: > > > On 2015/03/26 15:38:58, jam wrote: > > > > What is the purpose of this random delay? > > > > > > To avoid initiating a bunch of work at the same time. Wouldn't want to tie > up > > > threads with a bunches of back to back tasks that aren't tied directly to a > > user > > > action. I think ideally the time span could be a function of how many there > > are > > > to run. > > > > hmm, I understand the motivation but it's not clear that this is going to make > a > > difference. > > And if the list were very large, what would your gut say then? Avoiding a big > hit at time 0 was one of the issues raised on the mailing list about this > approach. All of the tasks I have in mind for this involve disk and/or network > IO. As a user of this api, i think I'd prefer having them spread out a little > so I wouldn't have to ensure that elsewhere. The call to RandInt was a way to do > that with minimal amount of code. > > I'm not real comfortable with running them at the same time. ok that seems fair.
ptal, added after_startup_task.h .cc files and some uma numbers
https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... File chrome/browser/after_startup_task.cc (right): https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:70: g_after_startup_tasks.Get().shrink_to_fit(); bummer, shrink_to_fit() isn't implemented everywhere
Just nits from me. https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... File chrome/browser/after_startup_task.cc (right): https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. Nit: "(c)" https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:117: ~StartupObserver() override { DCHECK(IsBrowserStartupComplete()); } Aren't DCHECKs for programmer errors? If the failsafe ever fires then this'll fail right? https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:156: #if !defined(OS_ANDROID) Why not Android? https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... File chrome/browser/after_startup_task.h (right): https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. Nit: No "(c)" https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.h:19: // Observes startup and when complete runs tasks that have accrued. Nit: double space.
https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... File chrome/browser/after_startup_task.cc (right): https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:117: ~StartupObserver() override { DCHECK(IsBrowserStartupComplete()); } On 2015/03/27 21:59:58, cmumford wrote: > Aren't DCHECKs for programmer errors? If the failsafe ever fires then this'll > fail right? I guess i'm trying to document that this class isn't deleted without having set the flag. If the failsafe fires, its set just like for all the other ways to complete. https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:156: #if !defined(OS_ANDROID) > Why not Android? https://codereview.chromium.org/949293002/#msg35 FUD: Initially mimic'ing chrome_browser_main_extra_parts_metrics.cc which avoids monitoring the first page load on android, although i'm not sure why it does.
lg, thanks Michael! Some comments below. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... File chrome/browser/after_startup_task.cc (right): https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:47: void RunTask(scoped_ptr<AfterStartupTask> queued_task); Fwd-declaring is quite atypical in Chromium, can you avoid this? https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:50: base::LazyInstance<std::deque<AfterStartupTask*>>::Leaky g_after_startup_tasks; Add a comment that this should only be accessed on the UI thread. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:53: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Use DCHECK_CURRENTLY_ON. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:63: UMA_HISTOGRAM_COUNTS_100("AfterStartupTasks.Count", I can imagine this growing beyond 100. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:89: if (IsBrowserStartupComplete()) { Please add a comment as to why checking this is necessary. Also, I think it would make more sense to check this first in this method (i.e., escape hatch at top and then have the remainder of the logic be about "queuing"). https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:113: // Observes the first page load and set the startup complete flag accordingly. I would add "visible" to that (i.e., "first visibile page load"). You can get that below with |render_frame_host->GetVisibilityState() == blink::WebPageVisibilityStateVisible| I think. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:114: class StartupObserver : public WebContentsObserver { I'd feel safer (and it would be self-documenting code) if we marked this class NonThreadSafe and dchecked CalledOnValidThread() in every method since this entire files deals with multi-threads a lot while this specific class is not intended to be thread-safe. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:182: (new StartupObserver)->Start(); This is a little weird, should we make the constructor private and have a StartupObserver::BeginObserving() static method on that class? https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... File chrome/browser/after_startup_task.h (right): https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. I'd name this file after_startup_task_utils.h or something (i.e. otherwise it makes it feels like it's defining some kind of AfterStartupTask class). (actually even more confusing is that I just saw that such an object actually exists, but is an implementation detail of the .cc file -- definitely consider renaming these files) https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.h:22: // Used to augment the behavior or content::BrowserThread::PostAfterStartupTask s/or/of ? https://codereview.chromium.org/949293002/diff/520001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/520001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1116: StartMonitoringStartup(); Add a comment why this is the right spot for this. (i.e. browser was started and we can thus tell whether we should be waiting for something to paint, but nothing can actually have painted yet as the main message loop hasn't started so begin observation now...)
thnx for all the review comments, here's another cut at it https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... File chrome/browser/after_startup_task.cc (right): https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/03/27 21:59:58, cmumford wrote: > Nit: "(c)" Done. https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... File chrome/browser/after_startup_task.h (right): https://codereview.chromium.org/949293002/diff/500001/chrome/browser/after_st... chrome/browser/after_startup_task.h:19: // Observes startup and when complete runs tasks that have accrued. On 2015/03/27 21:59:58, cmumford wrote: > Nit: double space. Done. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... File chrome/browser/after_startup_task.cc (right): https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:47: void RunTask(scoped_ptr<AfterStartupTask> queued_task); On 2015/03/30 18:29:37, gab wrote: > Fwd-declaring is quite atypical in Chromium, can you avoid this? Done https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:50: base::LazyInstance<std::deque<AfterStartupTask*>>::Leaky g_after_startup_tasks; On 2015/03/30 18:29:37, gab wrote: > Add a comment that this should only be accessed on the UI thread. Done. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:53: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2015/03/30 18:29:36, gab wrote: > Use DCHECK_CURRENTLY_ON. Done. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:63: UMA_HISTOGRAM_COUNTS_100("AfterStartupTasks.Count", On 2015/03/30 18:29:36, gab wrote: > I can imagine this growing beyond 100. Done, how about 10000 https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:89: if (IsBrowserStartupComplete()) { On 2015/03/30 18:29:37, gab wrote: > Please add a comment as to why checking this is necessary. Done. > Also, I think it would make more sense to check this first in this method (i.e., > escape hatch at top and then have the remainder of the logic be about > "queuing"). I'd end up testing IsBrowserStartupComplete() on background threads twice to do that. I'd like to avoid heap allocating the AfterStartupTask unless its needed. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:113: // Observes the first page load and set the startup complete flag accordingly. On 2015/03/30 18:29:36, gab wrote: > I would add "visible" to that (i.e., "first visibile page load"). > > You can get that below with |render_frame_host->GetVisibilityState() == > blink::WebPageVisibilityStateVisible| I think. Done, should work well with background mode launches. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:114: class StartupObserver : public WebContentsObserver { On 2015/03/30 18:29:36, gab wrote: > I'd feel safer (and it would be self-documenting code) if we marked this class > NonThreadSafe and dchecked CalledOnValidThread() in every method since this > entire files deals with multi-threads a lot while this specific class is not > intended to be thread-safe. Done, except only put the dcheck in OnStartupComplete() which all methods call. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.cc:182: (new StartupObserver)->Start(); On 2015/03/30 18:29:37, gab wrote: > This is a little weird, should we make the constructor private and have a > StartupObserver::BeginObserving() static method on that class? You mean a static method with this body :) I originally had it that way and decided to remove the extra function call given how single purpose this .cc file is. This is done plenty places elsewhere too, a few examples... // AppLaunchManager manages its own lifetime. (new AppLaunchManager(profile, app_id))->Start(); // IconLoader deletes itself when done. (new IconLoader(AsWeakPtr(), icon_path_))->Start(); // ChromeRestartRequest deletes itself after request sent to session manager. (new ChromeRestartRequest(command_line))->Start(); https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... File chrome/browser/after_startup_task.h (right): https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/03/30 18:29:37, gab wrote: > I'd name this file after_startup_task_utils.h or something (i.e. otherwise it > makes it feels like it's defining some kind of AfterStartupTask class). > > (actually even more confusing is that I just saw that such an object actually > exists, but is an implementation detail of the .cc file -- definitely consider > renaming these files) Done. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/after_st... chrome/browser/after_startup_task.h:22: // Used to augment the behavior or content::BrowserThread::PostAfterStartupTask On 2015/03/30 18:29:37, gab wrote: > s/or/of ? Done. https://codereview.chromium.org/949293002/diff/520001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/949293002/diff/520001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1116: StartMonitoringStartup(); On 2015/03/30 18:29:37, gab wrote: > Add a comment why this is the right spot for this. > > (i.e. browser was started and we can thus tell whether we should be waiting for > something to paint, but nothing can actually have painted yet as the main > message loop hasn't started so begin observation now...) Done.
ptal, added unittests
https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... File chrome/browser/after_startup_task_utils.cc (right): https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:215: g_startup_complete_flag.Get().UnsafeReset(); Another (better?) option here would be to reset the lazy instance to put it back to the unallocated state.
https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... File chrome/browser/after_startup_task_utils_unittest.cc (right): https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils_unittest.cc:30: posted_task_count_(0), Nit: Use Non-Static Class Member Initializers https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils_unittest.cc:137: AfterStartupTaskUtils::SetBrowserStartupIsComplete(); Can you add a comment explaining this test relies on a DCHECK in the impl? https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils_unittest.cc:168: ui_thread_->reset_task_counts(); What do you think about maybe resetting only one of them? Might catch more errors. https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils_unittest.cc:170: // Tasks posted after startup should get posted immediately. Do you want to add EXPECT_EQ(0, db_thread_->total_task_count()), ... just to be on the safe side here?
https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... File chrome/browser/after_startup_task_utils_unittest.cc (right): https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils_unittest.cc:30: posted_task_count_(0), On 2015/04/06 23:18:03, cmumford wrote: > Nit: Use Non-Static Class Member Initializers Done https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils_unittest.cc:137: AfterStartupTaskUtils::SetBrowserStartupIsComplete(); On 2015/04/06 23:18:03, cmumford wrote: > Can you add a comment explaining this test relies on a DCHECK in the impl? Done. https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils_unittest.cc:168: ui_thread_->reset_task_counts(); On 2015/04/06 23:18:03, cmumford wrote: > What do you think about maybe resetting only one of them? Might catch more > errors. Instead, I made some of the tasks verify they're running on the right thread. https://codereview.chromium.org/949293002/diff/580001/chrome/browser/after_st... chrome/browser/after_startup_task_utils_unittest.cc:170: // Tasks posted after startup should get posted immediately. On 2015/04/06 23:18:03, cmumford wrote: > Do you want to add EXPECT_EQ(0, db_thread_->total_task_count()), ... just to be > on the safe side here? Done.
ping!
lgtm
lgtm someone from base/owners should review the base change https://codereview.chromium.org/949293002/diff/660001/base/synchronization/ca... File base/synchronization/cancellation_flag.h (right): https://codereview.chromium.org/949293002/diff/660001/base/synchronization/ca... base/synchronization/cancellation_flag.h:33: void UnsafeReset(); nit: testing methods need to end with ForTesting() so that presubmit checks catch if they're called by non-testing code
@thakis for base/OWNERS https://codereview.chromium.org/949293002/diff/660001/base/synchronization/ca... File base/synchronization/cancellation_flag.h (right): https://codereview.chromium.org/949293002/diff/660001/base/synchronization/ca... base/synchronization/cancellation_flag.h:33: void UnsafeReset(); On 2015/04/08 20:34:27, jam wrote: > nit: testing methods need to end with ForTesting() so that presubmit checks > catch if they're called by non-testing code Done.
michaeln@chromium.org changed reviewers: + thakis@chromium.org
@thakis again, this time with him in the reviewers list
base lgtm with a comment. I looked at one non-base file to figure out what this was used for and left comments there too (sorry). Should this get an "intent to implement" thread on chromium-dev? Everyone should use this instead of posting after a random delay, right? https://codereview.chromium.org/949293002/diff/680001/base/synchronization/ca... File base/synchronization/cancellation_flag.h (right): https://codereview.chromium.org/949293002/diff/680001/base/synchronization/ca... base/synchronization/cancellation_flag.h:32: void UnsafeResetForTesting(); add method comment (explain how this is unsafe) https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... File chrome/browser/after_startup_task_utils.cc (right): https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:26: using StartupCompleteFlag = base::CancellationFlag; This is used in a single place. Why is this useful? Why not write base::CancellationFlag in that one place directly? https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:49: // Be sure to allocate the flag on the main thread. (i found this comment confusing. i get it now, but it took me a bit. no allocation is done in this function.) https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:64: const int kMaxDelay = 10; suffix name with unit (kMinDelaySec, kMaxDelaySec) Do you have a sense of how many of these tasks we'll enqueue? https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:174: #endif // !defined(OS_ANDROID) shouldn't we default to more than 3 unconditional seconds on android?
thnx! https://codereview.chromium.org/949293002/diff/680001/base/synchronization/ca... File base/synchronization/cancellation_flag.h (right): https://codereview.chromium.org/949293002/diff/680001/base/synchronization/ca... base/synchronization/cancellation_flag.h:32: void UnsafeResetForTesting(); On 2015/04/09 20:10:15, Nico wrote: > add method comment (explain how this is unsafe) Done (but not sure what to say really) // For subtle reasons that may be different on different architectures, // a different thread testing IsSet() may erroneously read 'true' after // this method has been called. https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... File chrome/browser/after_startup_task_utils.cc (right): https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:26: using StartupCompleteFlag = base::CancellationFlag; On 2015/04/09 20:10:15, Nico wrote: > This is used in a single place. Why is this useful? Why not write > base::CancellationFlag in that one place directly? This should help tip readers off to the fact that its not being used as a cancellation flag. An earlier review comment was something like, 'its weird to be using a cancellation flag here, but it does exactly what we want'. It might be nice to rename the base class to something more generic, SettableFlag? https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:49: // Be sure to allocate the flag on the main thread. On 2015/04/09 20:10:15, Nico wrote: > (i found this comment confusing. i get it now, but it took me a bit. no > allocation is done in this function.) Done how about 'initialize the lazyinstance on the main thread since the flag may only be set on initializing thread' https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:64: const int kMaxDelay = 10; On 2015/04/09 20:10:15, Nico wrote: > suffix name with unit (kMinDelaySec, kMaxDelaySec) Done. > Do you have a sense of how many of these tasks we'll enqueue? I don't. I have in mind a few things that may generate 10s of tasks, as for other uses, idk. An uma stat is in place to keep an eye on that number. https://codereview.chromium.org/949293002/diff/680001/chrome/browser/after_st... chrome/browser/after_startup_task_utils.cc:174: #endif // !defined(OS_ANDROID) On 2015/04/09 20:10:15, Nico wrote: > shouldn't we default to more than 3 unconditional seconds on android? i think android should probably monitor the initial page load too. the only reason i'm not doing it in this cl is because ChromeBrowserMainExtraPartsMetrics avoid observing the first page load on android (i'm not sure why). for now, i'll bumped it up to an unconditional 10secs for OS_ANDROID
> Should this get an "intent to implement" thread on chromium-dev? Everyone should > use this instead of posting after a random delay, right? I'll send mail to the chrome-fast team to start with and go from there.
On Thu, Apr 9, 2015 at 2:50 PM, <michaeln@chromium.org> wrote: > Should this get an "intent to implement" thread on chromium-dev? Everyone >> > should > >> use this instead of posting after a random delay, right? >> > > I'll send mail to the chrome-fast team to start with and go from there. > That's not public, is it? > > > https://codereview.chromium.org/949293002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
michaeln@chromium.org changed reviewers: + isherman@chromium.org
@isherman for histogram labels > That's not public, is it? Right, I don't think it is. I'll send mail to chromium-dev too.
histograms.xml lgtm % several nits: https://codereview.chromium.org/949293002/diff/700001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/949293002/diff/700001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:250: +<histogram name="AfterStartupTasks.Count"> Rather than creating a new "AfterStartupTasks" top-level group, could you please declare this as a subgroup of a reasonable existing group? The "Startup" group seems like a pretty good choice, IMO. https://codereview.chromium.org/949293002/diff/700001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:253: + The number of after startup tasks that were queued prior to startup nit: I think "after-startup" might be easier to read and parse than "after startup" https://codereview.chromium.org/949293002/diff/700001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:254: + completion. Please document when this is recorded. I assume that it's at startup completion time, but I'm not certain. https://codereview.chromium.org/949293002/diff/700001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:261: + Time from the process creation until after startup tasks were run. Is this when the first task started to be run, or when the tasks completed, or when all of them were started, but perhaps not yet completed? Please clarify in the description.
The CQ bit was checked by michaeln@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, cmumford@chromium.org, thakis@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/949293002/#ps720001 (title: " ")
The CQ bit was unchecked by michaeln@chromium.org
The CQ bit was checked by michaeln@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, thakis@chromium.org, isherman@chromium.org, cmumford@chromium.org Link to the patchset: https://codereview.chromium.org/949293002/#ps740001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949293002/740001
Message was sent while issue was closed.
Committed patchset #38 (id:740001)
Message was sent while issue was closed.
Patchset 38 (id:??) landed as https://crrev.com/96f887e253b8583cb5ea9dccda2aa796cdce2b98 Cr-Commit-Position: refs/heads/master@{#324950} |
