chrome/browser/translate/translate_bubble_showable.h - Issue 25373009: Translate: New Bubble UX (for the view toolkit)

Side by Side Diff: chrome/browser/translate/translate_bubble_showable.h

Issue 25373009: Translate: New Bubble UX (for the view toolkit) (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Sky's review (2) Created 7 years, 2 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« chrome/browser/tab_contents/language_state.h ('K') | « chrome/browser/translate/page_translated_details.h ('k') | chrome/browser/translate/translate_manager.h » ('j') | chrome/browser/ui/browser.cc » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2013 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #ifndef CHROME_BROWSER_TRANSLATE_TRANSLATE_BUBBLE_SHOWABLE_H_

	6 #define CHROME_BROWSER_TRANSLATE_TRANSLATE_BUBBLE_SHOWABLE_H_

	7

	8 #include "chrome/browser/ui/translate/translate_bubble_model.h"

	9

	10 namespace content {

	11 class WebContents;

	12 }

	13

	14 // The interface to show the translate bubble.

	15 class TranslateBubbleShowable {

	16 public:

	17 virtual ~TranslateBubbleShowable() {}

	18

	19 // Shows the Translate bubble.

	20 virtual void Show(content::WebContents* web_contents,
	sky 2013/10/22 16:09:22 How about putting this in browser_dialogs and nami How about putting this in browser_dialogs and naming it ShowTranslateBubble? If it needs a browser or webcontents, make it take those. The function should take everything it needs and not have to search through the browserlist. The implementation should reside in the approriate platforms, eg c/b/u/views. If you went this route to make it mockable, then the following would be better: class TranslateBubbleFactory { public: static void Show(XXX) // internally uses factory, if no factory then calls ShowImplementation(). static void SetFactory(...); protected: virtual void ShowImplementation(); }; Platforms then implement TranslateBubbleFactory::ShowImplementation Also, this should be in the chrome/browser/ui. sky 2013/10/22 16:16:00 Also, you could keep the Show method in browser_di Show quoted text On 2013/10/22 16:09:22, sky wrote: > How about putting this in browser_dialogs and naming it ShowTranslateBubble? If > it needs a browser or webcontents, make it take those. The function should take > everything it needs and not have to search through the browserlist. The > implementation should reside in the approriate platforms, eg c/b/u/views. > > If you went this route to make it mockable, then the following would be better: > > class TranslateBubbleFactory { > public: > static void Show(XXX) // internally uses factory, if no factory then calls > ShowImplementation(). > static void SetFactory(...); > > protected: > virtual void ShowImplementation(); > > }; > > Platforms then implement TranslateBubbleFactory::ShowImplementation > > Also, this should be in the chrome/browser/ui. Also, you could keep the Show method in browser_dialogs like I outlined and have TranslateBubbleFactory::Show() invoke it if no factory is set. That way ShowImplementation() can remain pure virtual. hajimehoshi 2013/10/23 10:45:09 Done. Showing the bubble via BrowserWindow is alre Show quoted text On 2013/10/22 16:16:00, sky wrote: > On 2013/10/22 16:09:22, sky wrote: > > How about putting this in browser_dialogs and naming it ShowTranslateBubble? > If > > it needs a browser or webcontents, make it take those. The function should > take > > everything it needs and not have to search through the browserlist. The > > implementation should reside in the approriate platforms, eg c/b/u/views. > > > > If you went this route to make it mockable, then the following would be > better: > > > > class TranslateBubbleFactory { > > public: > > static void Show(XXX) // internally uses factory, if no factory then calls > > ShowImplementation(). > > static void SetFactory(...); > > > > protected: > > virtual void ShowImplementation(); > > > > }; > > > > Platforms then implement TranslateBubbleFactory::ShowImplementation > > > > Also, this should be in the chrome/browser/ui. > > Also, you could keep the Show method in browser_dialogs like I outlined and have > TranslateBubbleFactory::Show() invoke it if no factory is set. That way > ShowImplementation() can remain pure virtual. Done. Showing the bubble via BrowserWindow is already platform-independent, so just created TranslateBubbleFactory.
	21 TranslateBubbleModel::ViewState view_state) = 0;

	22 };

	23

	24 #endif // CHROME_BROWSER_TRANSLATE_TRANSLATE_BUBBLE_SHOWABLE_H_

OLD	NEW