Delivering the FIRST_NONEMPTY_PAINT phase changing event to base/

content/browser/profiler_controller_impl.cc

Index: content/browser/profiler_controller_impl.cc
diff --git a/content/browser/profiler_controller_impl.cc b/content/browser/profiler_controller_impl.cc
index 98fd2c068ac377ccde4b6025ab2758721c1fa738..8388413311bcb4ffbc37b764325087cc67c5447d 100644
--- a/content/browser/profiler_controller_impl.cc
+++ b/content/browser/profiler_controller_impl.cc
@@ -72,7 +72,8 @@ void ProfilerControllerImpl::Unregister(const ProfilerSubscriber* subscriber) {
 }
 
 void ProfilerControllerImpl::GetProfilerDataFromChildProcesses(
-    int sequence_number) {
+    int sequence_number,
+    int current_profiling_phase) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   int pending_processes = 0;
@@ -85,8 +86,10 @@ void ProfilerControllerImpl::GetProfilerDataFromChildProcesses(
       continue;
 
     ++pending_processes;
-    if (!iter.Send(new ChildProcessMsg_GetChildProfilerData(sequence_number)))
+    if (!iter.Send(new ChildProcessMsg_GetChildProfilerData(
+            sequence_number, current_profiling_phase))) {
       --pending_processes;
+    }
   }
 
   BrowserThread::PostTask(
@@ -100,26 +103,59 @@ void ProfilerControllerImpl::GetProfilerDataFromChildProcesses(
           true));
 }
 
-void ProfilerControllerImpl::GetProfilerData(int sequence_number) {
+void ProfilerControllerImpl::NotifyChildProcessesOfProfilingPhaseCompletion(
+    int profiling_phase) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  for (BrowserChildProcessHostIterator iter; !iter.Done(); ++iter) {
+    // In some cases, there may be no child process of the given type (for
+    // example, the GPU process may not exist and there may instead just be a
+    // GPU thread in the browser process). If that's the case, then the process
+    // handle will be base::kNullProcessHandle and we shouldn't send it a
+    // message.
+    if (iter.GetData().handle == base::kNullProcessHandle)
+      continue;
+
+    iter.Send(new ChildProcessMsg_OnProfilingPhase(profiling_phase));
+  }
+}
+
+void ProfilerControllerImpl::GetProfilerData(int sequence_number,
+                                             int current_profiling_phase) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   int pending_processes = 0;
   for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
        !it.IsAtEnd(); it.Advance()) {
     ++pending_processes;
-    if (!it.GetCurrentValue()->Send(
-            new ChildProcessMsg_GetChildProfilerData(sequence_number))) {
+    if (!it.GetCurrentValue()->Send(new ChildProcessMsg_GetChildProfilerData(
+            sequence_number, current_profiling_phase))) {
       --pending_processes;
     }
   }
   OnPendingProcesses(sequence_number, pending_processes, false);
 
   BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
+      BrowserThread::IO, FROM_HERE,
       base::Bind(&ProfilerControllerImpl::GetProfilerDataFromChildProcesses,
-                 base::Unretained(this),
-                 sequence_number));
+                 base::Unretained(this), sequence_number,
+                 current_profiling_phase));
+}
+
+void ProfilerControllerImpl::OnProfilingPhaseCompletion(int profiling_phase) {

[Alexei Svitkine (slow), 2015/04/02 16:59:31]

Should probably have a comment in this function explaining why it's OK not to
wait for the child processes to acknowledge this.

i.e. if the assumption is that if this phase completes before the data
collection for UMA, then that will be done in a follow-up Send() also on UI
thread and that messages will be processed in sequence - so when we get reply
with the profiler data, that would be guaranteed after those children handled
any phase completion events they've received. (If this description is correct -
please add something among these lines as a comment here.)

[vadimt, 2015/04/06 23:25:12]

Hmm. I don't wait for any result simply because there is no result from phase
completion... It's simply a signal to a child to start labelling tasks with a
new phase number.

Not sure that the order of messages is important too, and we don't rely on it.
If a message collecting data for sending is sent after phase change, but somehow
gets processed first in the child process - no big deal: the child will respond
to Send with less phases, i.e. tasks from the last very short phase will be in
the previous phase...

So, I'm not sure what to write in the comment.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());

[Alexei Svitkine (slow), 2015/04/02 16:59:31]

Hmm, I realise you're just following the same pattern as the previous code, but
I am confused why we notify renderers and child processes separately - and what
prevents renderer processes being notified twice? (i.e. wouldn't renderers
already be included in BrowserChildProcessHostIterator?).

If indeed this is the correct thing to do, please add a comment explaining why
that's the case - since it's entirely not obvious just from reading the code.

[vadimt, 2015/04/06 23:25:12]

I checked that everything works correctly, and that this is a correct way to do
things. As for the deep roots for such separation, it would take time to figure
this out. Not sure this belongs to the scope of this CL.

[Alexei Svitkine (slow), 2015/04/09 15:39:05]

Sorry, forgot to follow-up on this earlier. When you said you checked, could you
clarify how?
I just want to make sure we don't send a message to the same process twice (as a
result of the two mechanisms) that accidentally doesn't result in a problem.

To avoid more back and forth discussing this - let me suggest a concrete thing
to do that would convince me this is ok: Can you make the code print the process
ids/types in both of the loops (the RenderProcessHost and
BrowserChildProcessHostIterator loops) - and reply with the output from a Chrome
that has a few tabs open. This should answer my question (i.e. whether the lists
overlap or not - and if they don't overlap, the reason for it - i.e. presumably
based on the type of process). Thanks!

[vadimt, 2015/04/09 21:28:39]

I've set a breakpoint in the renderer, and checked that the message gets
delivered only once. Will this work?

+       !it.IsAtEnd(); it.Advance()) {
+    it.GetCurrentValue()->Send(
+        new ChildProcessMsg_OnProfilingPhase(profiling_phase));
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&ProfilerControllerImpl::
+                     NotifyChildProcessesOfProfilingPhaseCompletion,
+                 base::Unretained(this), profiling_phase));
 }
 
 }  // namespace content