Fix issue 5079: Incorrect "Active match ordinal" count during Find-in-page

I introduced a regression in my reimplemenation of Find-in-page. The active match ordinal in Find-in-page (also known as "the 7" in "7 of 9") would be just a little off on pages with frames. 

Problem A: When you search for something in gmail, for example, the ordinal could start off slightly negative or be 0. I wasn't checking the last_match_count_ of a frame for negative numbers before adding it to the total (it starts off as -1 and remains that way if the frame is not deemed to be worthy of being scoped, i.e. if it is hidden).

Problem B: On pages with multiple matches spread across multiple frames the ordinal would not be subtracted correctly after pressing F3 and Shift-F3 to go back to the frame you were on. We shouldn't be increasing/decreasing the active_match_index for a given frame when FindNext/FindPrevious causes us to jump between frames. We should instead reset it.

I added two tests to catch this in the future. They test ordinal values as you use Find in page (including combinations of frames/no-frames & FindNext/FindPrevious).

Oh, and I also removed some traces that were supposed to expose why a test was flaky, but it turns out to have been something unrelated to the test.
Review URL: http://codereview.chromium.org/13130

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@6369 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 30 insertions, 14 deletions

diff --git a/webkit/glue/webframe_impl.cc b/webkit/glue/webframe_impl.cc
index d639558..86fcb02 100644
--- a/webkit/glue/webframe_impl.cc
+++ b/webkit/glue/webframe_impl.cc
@@ -353,7 +353,7 @@ void WebFrameImpl::InternalLoadRequest(const WebRequest* request,
         decodeURLEscapeSequences(kurl.string().substring(sizeof("javascript:")-1));
     WebCore::ScriptValue result = frame_->loader()->executeScript(script, true);
     String scriptResult;
-    if (result.getString(scriptResult) && 
+    if (result.getString(scriptResult) &&
         !frame_->loader()->isScheduledLocationChangePending()) {
       // TODO(darin): We need to figure out how to represent this in session
       // history.  Hint: don't re-eval script when the user or script navigates
@@ -764,6 +764,9 @@ void WebFrameImpl::InvalidateArea(AreaToInvalidate area) {
 }
 
 void WebFrameImpl::IncreaseMatchCount(int count, int request_id) {
+  // This function should only be called on the mainframe.
+  DCHECK(this == static_cast<WebFrameImpl*>(GetView()->GetMainFrame()));
+
   total_matchcount_ += count;
 
   // Update the UI with the latest findings.
@@ -816,6 +819,9 @@ bool WebFrameImpl::Find(const FindInPageRequest& request,
 #if defined(OS_WIN)
     WebCore::RenderThemeWin::setFindInPageMode(true);
 #endif
+    // Store which frame was active. This will come in handy later when we
+    // change the active match ordinal below.
+    WebFrameImpl* old_active_frame = main_frame_impl->active_match_frame_;
     // Set this frame as the active frame (the one with the active highlight).
     main_frame_impl->active_match_frame_ = this;
 
@@ -841,13 +847,23 @@ bool WebFrameImpl::Find(const FindInPageRequest& request,
       // to find the active rect for us so we can update the ordinal (n of m).
       locating_active_rect_ = true;
     } else {
-      // This is FindNext so we need to increment (or decrement) the count and
-      // wrap if needed.
-      request.forward ? ++active_match_index_ : --active_match_index_;
-      if (active_match_index_ + 1 > last_match_count_)
-        active_match_index_ = 0;
-      if (active_match_index_ + 1 == 0)
-        active_match_index_ = last_match_count_ - 1;
+      if (old_active_frame != this) {
+        // If the active frame has changed it means that we have a multi-frame
+        // page and we just switch to searching in a new frame. Then we just
+        // want to reset the index.
+        if (request.forward)
+          active_match_index_ = 0;
+        else
+          active_match_index_ = last_match_count_ - 1;
+      } else {
+        // We are still the active frame, so increment (or decrement) the
+        // |active_match_index|, wrapping if needed (on single frame pages).
+        request.forward ? ++active_match_index_ : --active_match_index_;
+        if (active_match_index_ + 1 > last_match_count_)
+          active_match_index_ = 0;
+        if (active_match_index_ + 1 == 0)
+          active_match_index_ = last_match_count_ - 1;
+      }
     }
 
 #if defined(OS_WIN)
@@ -885,7 +901,8 @@ int WebFrameImpl::OrdinalOfFirstMatchForFrame(WebFrameImpl* frame) const {
        it != frame;
        it = static_cast<WebFrameImpl*>(
            webview_impl_->GetNextFrameAfter(it, true))) {
-    ordinal += it->last_match_count_;
+    if (it->last_match_count_ > 0)
+      ordinal += it->last_match_count_;
   }
 
   return ordinal;
@@ -1539,7 +1556,7 @@ void WebFrameImpl::ExecuteJavaScript(const std::string& js_code,
                                      const GURL& script_url) {
   WebCore::ScriptSourceCode source_code(
       webkit_glue::StdStringToString(js_code),
-      webkit_glue::GURLToKURL(script_url), 
+      webkit_glue::GURLToKURL(script_url),
       1);  // base line number (for errors)
   frame_->loader()->executeScript(source_code);
 }
@@ -1828,4 +1845,3 @@ void WebFrameImpl::ClearPasswordListeners() {
   }
   password_listeners_.clear();
 }
-
diff --git a/webkit/glue/webframe_impl.h b/webkit/glue/webframe_impl.h
index 635b396..44c3ed8 100644
--- a/webkit/glue/webframe_impl.h
+++ b/webkit/glue/webframe_impl.h
@@ -336,7 +336,7 @@ class WebFrameImpl : public WebFrame {
   RefPtr<WebCore::Range> active_match_;
 
   // The index of the active match.
-  size_t active_match_index_;
+  int active_match_index_;
 
   // This flag is used by the scoping effort to determine if we need to figure
   // out which rectangle is the active match. Once we find the active
@@ -357,7 +357,7 @@ class WebFrameImpl : public WebFrame {
   // don't loose count between scoping efforts, and is also used (in conjunction
   // with last_search_string_ and scoping_complete_) to figure out if we need to
   // search the frame again.
-  size_t last_match_count_;
+  int last_match_count_;
 
   // This variable keeps a cumulative total of matches found so far for ALL the
   // frames on the page, and is only incremented by calling IncreaseMatchCount
@@ -375,7 +375,7 @@ class WebFrameImpl : public WebFrame {
 
   // Keeps track of when the scoping effort should next invalidate the scrollbar
   // and the frame area.
-  size_t next_invalidate_after_;
+  int next_invalidate_after_;
 
  private:
   // A bit mask specifying area of the frame to invalidate.