Better distinguish didFinishLoad and didStopLoading

components/autofill/content/renderer/password_autofill_agent.cc

Index: components/autofill/content/renderer/password_autofill_agent.cc
diff --git a/components/autofill/content/renderer/password_autofill_agent.cc b/components/autofill/content/renderer/password_autofill_agent.cc
index 9e6df579fc42a7de38533b88c3a43d51f1cad37a..4e9657022b35110f6de5fb6d9e6d71e705045f60 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -692,7 +692,6 @@ PasswordAutofillAgent::PasswordAutofillAgent(content::RenderFrame* render_frame)
       logging_state_active_(false),
       was_username_autofilled_(false),
       was_password_autofilled_(false),
-      did_stop_loading_(false),
       weak_ptr_factory_(this) {
   Send(new AutofillHostMsg_PasswordAutofillAgentConstructed(routing_id()));
 }
@@ -1097,9 +1096,16 @@ void PasswordAutofillAgent::SendPasswordForms(bool only_visible) {
   }
 
   if (only_visible) {
-    Send(new AutofillHostMsg_PasswordFormsRendered(routing_id(),
-                                                   password_forms,
-                                                   did_stop_loading_));
+    bool is_last_load = true;
+    for (blink::WebFrame* frame = render_frame()->GetWebFrame()->top(); frame;
+         frame = frame->traverseNext(false)) {
+      if (frame != render_frame()->GetWebFrame() && frame->isLoading()) {

[Nate Chapin, 2015/10/06 18:35:36]

This is kind of gross, and necessary because didStopLoading() is now called
after didFinishLoad(). I guess the alternative is to add this bit as a parameter
in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can probably
determine it in O(1)?

[dcheng, 2015/10/08 05:34:35]

"didStopLoading does the same thing [no other navigations are in progress], but
fires whether or not the navigation was successful."

With the previous semantics, was it checking for navigations across all frames
in the page, or just navigations in this frame and its descendants?

[Nate Chapin, 2015/10/08 21:03:59]

It was checking all frames. The message blink sends out is per-frame, but
PasswordAutofillAgent was listening to RenderViewObserver::DidStopLoading, which
is per-page.

[dcheng, 2015/10/09 18:34:01]

Hmm... but when this is called, the entire page might not be finished loading
right? So this callback might never run in that case.

[Nate Chapin, 2015/10/09 18:39:44]

This callback is called for each DidFinishLoad. is_last_load should be true if
this will be the last DidFinishLoad in this page (i.e., if this frame is the
only one where isLoading() is true. So it will only not get called if the last
frame fails, which is no change from the status quo.

[dcheng, 2015/10/09 18:45:26]

Hmm. I'm thinking of the OOPI case: subframe renderers will never send the IPC
message in that case, right?

+        is_last_load = false;
+        break;
+      }
+    }
+    Send(new AutofillHostMsg_PasswordFormsRendered(routing_id(), password_forms,
+                                                   is_last_load));
   } else {
     Send(new AutofillHostMsg_PasswordFormsParsed(routing_id(), password_forms));
   }
@@ -1145,14 +1151,6 @@ void PasswordAutofillAgent::DidCommitProvisionalLoad(
   }
 }
 
-void PasswordAutofillAgent::DidStartLoading() {
-  did_stop_loading_ = false;
-}
-
-void PasswordAutofillAgent::DidStopLoading() {
-  did_stop_loading_ = true;
-}
-
 void PasswordAutofillAgent::FrameDetached() {
   // If a sub frame has been destroyed while the user was entering information
   // into a password form, try to save the data. See https://crbug.com/450806
@@ -1561,14 +1559,6 @@ void PasswordAutofillAgent::LegacyPasswordAutofillAgent::OnDestruct() {
   // No op. Do not delete |this|.
 }
 
-void PasswordAutofillAgent::LegacyPasswordAutofillAgent::DidStartLoading() {
-  agent_->DidStartLoading();
-}
-
-void PasswordAutofillAgent::LegacyPasswordAutofillAgent::DidStopLoading() {
-  agent_->DidStopLoading();
-}
-
 void PasswordAutofillAgent::LegacyPasswordAutofillAgent::
     DidStartProvisionalLoad(blink::WebLocalFrame* navigated_frame) {
   agent_->LegacyDidStartProvisionalLoad(navigated_frame);