Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(339)

Issue 8817007: Revert 113015 - speculative revert to see if this fixes the interactive test breakage (Closed)

Created:
9 years ago by sail
Modified:
9 years ago
Reviewers:
jam
CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, ajwong+watch_chromium.org, jam, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, Paweł Hajdan Jr., darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

Revert 113015 - speculative revert to see if this fixes the interactive test breakage Remove OnMessageReceived that was using internal content IPCs in a chrome test. Dispatch the IPC in RenderViewHost instead of TabContents to solve this (it's only used by tests anyways). Remove test_utils methods that weren't being used. BUG=98716 Review URL: http://codereview.chromium.org/8801002 TBR=jam@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113110

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -21 lines) Patch
M chrome/browser/browser_focus_uitest.cc View 7 chunks +28 lines, -10 lines 0 comments Download
M chrome/test/base/ui_test_utils.h View 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/test/base/ui_test_utils.cc View 1 chunk +12 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_view_host.h View 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_view_host.cc View 2 chunks +0 lines, -8 lines 0 comments Download
M content/browser/tab_contents/tab_contents.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/tab_contents/tab_contents.cc View 2 chunks +8 lines, -0 lines 0 comments Download
M content/public/browser/notification_types.h View 1 chunk +3 lines, -2 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
sail
9 years ago (2011-12-06 04:33:43 UTC) #1
jam
9 years ago (2011-12-06 06:24:55 UTC) #2
note: reverts on trees as experiments to fix issues are not free: the file
history becomes harder to see. it would  be nice to do this via trybot with
just that test

On Mon, Dec 5, 2011 at 8:33 PM, <sail@chromium.org> wrote:

> Reviewers: John Abd-El-Malek,
>
> Description:
> Revert 113015 - speculative revert to see if this fixes the interactive
> test
> breakage
> Remove OnMessageReceived that was using internal content IPCs in a chrome
> test.
> Dispatch the IPC in RenderViewHost instead of TabContents to solve this
> (it's
> only used by tests anyways). Remove test_utils methods that weren't being
> used.
>
> BUG=98716
> Review URL:
http://codereview.chromium.**org/8801002<http://codereview.chromium.org/8801002>
>
> TBR=jam@chromium.org
>
> Please review this at
http://codereview.chromium.**org/8817007/<http://codereview.chromium.org/8817...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     chrome/browser/browser_focus_**uitest.cc
>  M     chrome/test/base/ui_test_**utils.h
>  M     chrome/test/base/ui_test_**utils.cc
>  M     content/browser/renderer_host/**render_view_host.h
>  M     content/browser/renderer_host/**render_view_host.cc
>  M     content/browser/tab_contents/**tab_contents.h
>  M     content/browser/tab_contents/**tab_contents.cc
>  M     content/public/browser/**notification_types.h
>
>
> Index: chrome/browser/browser_focus_**uitest.cc
> ==============================**==============================**=======
> --- chrome/browser/browser_focus_**uitest.cc      (revision 113109)
> +++ chrome/browser/browser_focus_**uitest.cc      (working copy)
> @@ -26,6 +26,7 @@
>  #include "content/browser/tab_contents/**interstitial_page.h"
>  #include "content/browser/tab_contents/**tab_contents.h"
>  #include "content/browser/tab_contents/**tab_contents_view.h"
> +#include "content/common/view_messages.**h"
>  #include "content/public/browser/**notification_service.h"
>  #include "net/test/test_server.h"
>
> @@ -63,9 +64,9 @@
>  #define MAYBE_**TabsRememberFocusFindInPage FAILS_**
> TabsRememberFocusFindInPage
>  #elif defined(OS_WIN)
>  // Disabled, http://crbug.com/62543.
> -#define MAYBE_FocusTraversal FocusTraversal
> +#define MAYBE_FocusTraversal DISABLED_FocusTraversal
>  // Disabled, http://crbug.com/62544.
> -#define MAYBE_**FocusTraversalOnInterstitial FocusTraversalOnInterstitial
> +#define MAYBE_**FocusTraversalOnInterstitial DISABLED_**
> FocusTraversalOnInterstitial
>  // Flaky, http://crbug.com/62537.
>  #define MAYBE_**TabsRememberFocusFindInPage FLAKY_**
> TabsRememberFocusFindInPage
>  #endif
> @@ -172,6 +173,23 @@
>     return render_view_host()->view()->**HasFocus();
>   }
>
> + protected:
> +  bool OnMessageReceived(const IPC::Message& message) {
> +    bool handled = true;
> +    IPC_BEGIN_MESSAGE_MAP(**TestInterstitialPage, message)
> +      IPC_MESSAGE_HANDLER(**ViewHostMsg_**FocusedNodeChanged,
> OnFocusedNodeChanged)
> +      IPC_MESSAGE_UNHANDLED(handled = false)
> +    IPC_END_MESSAGE_MAP()
> +    return handled;
> +  }
> +
> +  void OnFocusedNodeChanged(bool is_editable_node) {
> +    content::NotificationService::**current()->Notify(
> +        content::NOTIFICATION_FOCUS_**CHANGED_IN_PAGE,
> +        content::Source<TabContents>(**tab()),
> +        content::Details<const bool>(&is_editable_node));
> +  }
> +
>  private:
>   std::string html_contents_;
>  };
> @@ -493,8 +511,8 @@
>         ASSERT_TRUE(ui_test_utils::**SendKeyPressAndWaitWithDetails**(
>             browser(), ui::VKEY_TAB, false, false, false, false,
>             content::NOTIFICATION_FOCUS_**CHANGED_IN_PAGE,
> -            content::NotificationSource(**content::Source<**
> RenderViewHost>(
> -                browser()->**GetSelectedTabContents()->**
> render_view_host())),
> +            content::NotificationSource(**content::Source<TabContents>(
> +                browser()->**GetSelectedTabContents())),
>             details));
>       } else {
>         // On the last tab key press, the focus returns to the browser.
> @@ -536,8 +554,8 @@
>         ASSERT_TRUE(ui_test_utils::**SendKeyPressAndWaitWithDetails**(
>             browser(), ui::VKEY_TAB, false, true, false, false,
>             content::NOTIFICATION_FOCUS_**CHANGED_IN_PAGE,
> -            content::NotificationSource(**content::Source<**
> RenderViewHost>(
> -                browser()->**GetSelectedTabContents()->**
> render_view_host())),
> +            content::NotificationSource(**content::Source<TabContents>(
> +                browser()->**GetSelectedTabContents())),
>             details));
>       } else {
>         // On the last tab key press, the focus returns to the browser.
> @@ -620,8 +638,8 @@
>           content::NotificationService::**AllSources();
>       if (j < arraysize(kExpElementIDs) - 1) {
>         notification_type = content::NOTIFICATION_FOCUS_**CHANGED_IN_PAGE;
> -        notification_source = content::Source<**RenderViewHost>(
> -            interstitial_page->render_**view_host());
> +        notification_source = content::Source<TabContents>(
> +            interstitial_page->tab());
>       } else {
>         // On the last tab key press, the focus returns to the browser.
>         notification_type = chrome::NOTIFICATION_FOCUS_**
> RETURNED_TO_BROWSER;
> @@ -655,8 +673,8 @@
>           content::NotificationService::**AllSources();
>       if (j < arraysize(kExpElementIDs) - 1) {
>         notification_type = content::NOTIFICATION_FOCUS_**CHANGED_IN_PAGE;
> -        notification_source = content::Source<**RenderViewHost>(
> -            interstitial_page->render_**view_host());
> +        notification_source = content::Source<TabContents>(
> +            interstitial_page->tab());
>       } else {
>         // On the last tab key press, the focus returns to the browser.
>         notification_type = chrome::NOTIFICATION_FOCUS_**
> RETURNED_TO_BROWSER;
> Index: chrome/test/base/ui_test_**utils.cc
> ==============================**==============================**=======
> --- chrome/test/base/ui_test_**utils.cc   (revision 113109)
> +++ chrome/test/base/ui_test_**utils.cc   (working copy)
> @@ -549,6 +549,18 @@
>                   content::Source<content::**RenderProcessHost>(rph));
>  }
>
> +void WaitForFocusChange(**TabContents* tab_contents) {
> +  TestNotificationObserver observer;
> +  RegisterAndWait(&observer, content::NOTIFICATION_FOCUS_**
> CHANGED_IN_PAGE,
> +                  content::Source<TabContents>(**tab_contents));
> +}
> +
> +void WaitForFocusInBrowser(Browser* browser) {
> +  TestNotificationObserver observer;
> +  RegisterAndWait(&observer, chrome::NOTIFICATION_FOCUS_**
> RETURNED_TO_BROWSER,
> +                  content::Source<Browser>(**browser));
> +}
> +
>  int FindInPage(TabContentsWrapper* tab_contents, const string16&
> search_string,
>                bool forward, bool match_case, int* ordinal) {
>   tab_contents->
> Index: chrome/test/base/ui_test_**utils.h
> ==============================**==============================**=======
> --- chrome/test/base/ui_test_**utils.h    (revision 113109)
> +++ chrome/test/base/ui_test_**utils.h    (working copy)
> @@ -194,6 +194,13 @@
>  // Causes the specified tab to crash. Blocks until it is crashed.
>  void CrashTab(TabContents* tab);
>
> +// Waits for the focus to change in the specified tab.
> +void WaitForFocusChange(**TabContents* tab_contents);
> +
> +// Waits for the renderer to return focus to the browser (happens through
> tab
> +// traversal).
> +void WaitForFocusInBrowser(Browser* browser);
> +
>  // Performs a find in the page of the specified tab. Returns the number of
>  // matches found.  |ordinal| is an optional parameter which is set to the
> index
>  // of the current match.
> Index: content/browser/renderer_host/**render_view_host.cc
> ==============================**==============================**=======
> --- content/browser/renderer_host/**render_view_host.cc   (revision
> 113109)
> +++ content/browser/renderer_host/**render_view_host.cc   (working copy)
> @@ -712,7 +712,6 @@
>     IPC_MESSAGE_HANDLER(**DragHostMsg_UpdateDragCursor,
> OnUpdateDragCursor)
>     IPC_MESSAGE_HANDLER(**DragHostMsg_TargetDrop_ACK, OnTargetDropACK)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_TakeFocus, OnTakeFocus)
> -    IPC_MESSAGE_HANDLER(**ViewHostMsg_**FocusedNodeChanged,
> OnFocusedNodeChanged)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_**AddMessageToConsole,
> OnAddMessageToConsole)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_ShouldClose_ACK,
> OnMsgShouldCloseACK)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_ClosePage_ACK, OnMsgClosePageACK)
> @@ -1132,13 +1131,6 @@
>     view->TakeFocus(reverse);
>  }
>
> -void RenderViewHost::**OnFocusedNodeChanged(bool is_editable_node) {
> -  content::NotificationService::**current()->Notify(
> -      content::NOTIFICATION_FOCUS_**CHANGED_IN_PAGE,
> -      content::Source<**RenderViewHost>(this),
> -      content::Details<const bool>(&is_editable_node));
> -}
> -
>  void RenderViewHost::**OnAddMessageToConsole(int32 level,
>                                            const string16& message,
>                                            int32 line_no,
> Index: content/browser/renderer_host/**render_view_host.h
> ==============================**==============================**=======
> --- content/browser/renderer_host/**render_view_host.h    (revision
> 113109)
> +++ content/browser/renderer_host/**render_view_host.h    (working copy)
> @@ -564,7 +564,6 @@
>   void OnUpdateDragCursor(WebKit::**WebDragOperation drag_operation);
>   void OnTargetDropACK();
>   void OnTakeFocus(bool reverse);
> -  void OnFocusedNodeChanged(bool is_editable_node);
>   void OnAddMessageToConsole(int32 level,
>                              const string16& message,
>                              int32 line_no,
> Index: content/browser/tab_contents/**tab_contents.cc
> ==============================**==============================**=======
> --- content/browser/tab_contents/**tab_contents.cc        (revision
> 113109)
> +++ content/browser/tab_contents/**tab_contents.cc        (working copy)
> @@ -315,6 +315,7 @@
>                         OnUpdateContentRestrictions)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_GoToEntryAtOffset,
> OnGoToEntryAtOffset)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_UpdateZoomLimits,
> OnUpdateZoomLimits)
> +    IPC_MESSAGE_HANDLER(**ViewHostMsg_**FocusedNodeChanged,
> OnFocusedNodeChanged)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_SaveURLAs, OnSaveURL)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_**EnumerateDirectory,
> OnEnumerateDirectory)
>     IPC_MESSAGE_HANDLER(**ViewHostMsg_JSOutOfMemory, OnJSOutOfMemory)
> @@ -1151,6 +1152,13 @@
>   temporary_zoom_settings_ = !remember;
>  }
>
> +void TabContents::**OnFocusedNodeChanged(bool is_editable_node) {
> +  content::NotificationService::**current()->Notify(
> +      content::NOTIFICATION_FOCUS_**CHANGED_IN_PAGE,
> +      content::Source<TabContents>(**this),
> +      content::Details<const bool>(&is_editable_node));
> +}
> +
>  void TabContents::**OnEnumerateDirectory(int request_id,
>                                        const FilePath& path) {
>   delegate()->**EnumerateDirectory(this, request_id, path);
> Index: content/browser/tab_contents/**tab_contents.h
> ==============================**==============================**=======
> --- content/browser/tab_contents/**tab_contents.h (revision 113109)
> +++ content/browser/tab_contents/**tab_contents.h (working copy)
> @@ -642,6 +642,7 @@
>   void OnUpdateZoomLimits(int minimum_percent,
>                           int maximum_percent,
>                           bool remember);
> +  void OnFocusedNodeChanged(bool is_editable_node);
>   void OnEnumerateDirectory(int request_id, const FilePath& path);
>   void OnJSOutOfMemory();
>   void OnRegisterProtocolHandler(**const std::string& protocol,
> Index: content/public/browser/**notification_types.h
> ==============================**==============================**=======
> --- content/public/browser/**notification_types.h (revision 113109)
> +++ content/public/browser/**notification_types.h (working copy)
> @@ -357,8 +357,9 @@
>   NOTIFICATION_RENDER_WIDGET_**VISIBILITY_CHANGED,
>
>   // The focused element inside a page has changed.  The source is the
> -  // RenderViewHost. The details is a Details<const bool> that indicates
> whether
> -  // or not an editable node was focused.
> +  // TabContents containing the render view host for the page. The
> details is
> +  // a Details<const bool> that indicates whether or not an editable node
> was
> +  // focused.
>   NOTIFICATION_FOCUS_CHANGED_IN_**PAGE,
>
>   // Notification posted from ExecuteJavascriptInWebFrameNot**ifyResult.
> The
>
>
>

Powered by Google App Engine
This is Rietveld 408576698