Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6036004:
Refactor AutocompleteEditViewGtk so that AutocompleteEditView impl can be swapped. (Closed)
|
Created:
9 years, 12 months ago by oshima Modified:
9 years, 6 months ago CC:
chromium-reviews, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionI'm working on autocomplete edit view which does not use native controls for Chromeos and Touch UI.
The goal of this refactoring is to make it easy to swap AutocompleteEditView implementation between Gtk based impl and Views based one. This is 1st phrase and there will be more refacotring needed to implement AutocompleteEditViewViews.
- Added factory method AutocompleteEditViewGtk::Create,
which hides initialization details for gtk based impl.
- Moved gtk specific accessibilty code to AutocompleteEditViewGtk.
- Added TOOLKIT_VIWES only methods to AutocompleteEditView
to hide platform specific implementation. (AddToView, CommitInstatntSuggestion/SetInstantSuggestion)
BUG=none
TEST=none no functional change. all tests should pass.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70746
Patch Set 1 #Patch Set 2 : " #Patch Set 3 : " #
Total comments: 28
Patch Set 4 : " #Patch Set 5 : " #
Total comments: 8
Patch Set 6 : " #
Total comments: 12
Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:796: views::View* AutocompleteEditViewGtk::AddToView(views::View* parent) { Nit: This function is the same in Windows and Gtk. Are you intending to create some common third file eventually which contains this and any other "common to all views implementations" definitions? http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:807: return CommitInstantSuggestion(); Nit: Can't the body of that function be rolled into this? http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:79: ); Nit: I slightly prefer leaving the ); on the individual lines http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:150: void CreateAccessibleWidgetHelper(int res); Nit: |res| is a meaningless name; pick something more descriptive (e.g. xxx_id) and add a comment about what this function does http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:152: static AutocompleteEditView* Create(AutocompleteEditController* controller, Nit: A comment about what this function's responsibilities are would also be nice. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:159: int TextWidth() const; Nit: Since there's only one definition of this function, it seems better to have one declaration outside the #ifdefs than one in each branch. Adding "virtual" in the non-views case isn't going to make any real difference. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:773: #if defined(OS_WIN) Nit: While I like const locals, most Chrome code avoids them. You could save a line, then, by rewriting: std::wstring suggestion; #if defined(OS_WIN) suggestion = suggested_text_view_->GetText(); #endif http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1252: return true; Nit: Wouldn't it be better to call the AutocompleteEditViewGtk and ask it to do the check that currently begins CommitInstantSuggestion()? http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.h:48: #if defined(OS_WIN) Nit: Leave this section where it was (Chrome convention is to put #if XXX declarations below all others).
I'll review this CL asap. Just FYI: I'm working on http://codereview.chromium.org/5966006 which may conflict with this CL.
http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:845: autocomplete->CreateAccessibleWidgetHelper(IDS_ACCNAME_LOCATION); Why should this be a separate step? Could it be done in Init? Same for the function calls above - is there any reason why gtk_widget_show_all and gtk_widget_hide couldn't be done inside of Init()? Or could they be abstracted into another post-init method? http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:150: void CreateAccessibleWidgetHelper(int res); On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: |res| is a meaningless name; pick something more descriptive (e.g. xxx_id) > and add a comment about what this function does Maybe EnableAccessibility would be a more clear name? I'd just have it take no arguments unless you expect it'd ever be called with anything other than IDS_ACCNAME_LOCATION (like if you expect the AutocompleteEditView to be used for something other than the omnibox?) http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:152: static AutocompleteEditView* Create(AutocompleteEditController* controller, On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: A comment about what this function's responsibilities are would also be > nice. What's the reason for adding a static Create method when the constructor sufficed before? Would it make sense to do the same to AutocompleteEditViewWin for consistency?
http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:796: views::View* AutocompleteEditViewGtk::AddToView(views::View* parent) { On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: This function is the same in Windows and Gtk. Are you intending to create > some common third file eventually which contains this and any other "common to > all views implementations" definitions? Views implementation will be different (there will be no NativeViewHost), and Gtk's implemntation will be gone eventually once we eliminate gtk. I'm planning to look into possibility to move SuggestedTextView from location_bar_view to AutocompleteEditView and i may (or may not) introduce such class that time. I'm fine to add it now if you want. Please let me know. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:807: return CommitInstantSuggestion(); On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: Can't the body of that function be rolled into this? CommitInstantSuggestoni() is used by gtk version. I'll look into if I can merge them nicely along with SuggestedTextViwe move. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:845: autocomplete->CreateAccessibleWidgetHelper(IDS_ACCNAME_LOCATION); On 2011/01/06 15:03:03, Dominic Mazzoni wrote: > Why should this be a separate step? Could it be done in Init? > > Same for the function calls above - is there any reason why gtk_widget_show_all > and gtk_widget_hide couldn't be done inside of Init()? Or could they be > abstracted into another post-init method? Above functions are not called in gtk impl. I can move them to Init with ifdefs if you want. I think adding post init for linux_view is overkill. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:79: ); On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: I slightly prefer leaving the ); on the individual lines unbalanced parenthesis and brackets confused emacs indentation, so I'd like to keep this if you don't mind. I can change it back if you feel strongly about it, so please let me know. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:150: void CreateAccessibleWidgetHelper(int res); On 2011/01/06 15:03:03, Dominic Mazzoni wrote: > On 2011/01/06 02:06:48, Peter Kasting wrote: > > Nit: |res| is a meaningless name; pick something more descriptive (e.g. > xxx_id) > > and add a comment about what this function does > > Maybe EnableAccessibility would be a more clear name? I'd just have it take no > arguments unless you expect it'd ever be called with anything other than > IDS_ACCNAME_LOCATION (like if you expect the AutocompleteEditView to be used for > something other than the omnibox?) Done. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:152: static AutocompleteEditView* Create(AutocompleteEditController* controller, On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: A comment about what this function's responsibilities are would also be > nice. Done. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:152: static AutocompleteEditView* Create(AutocompleteEditController* controller, On 2011/01/06 15:03:03, Dominic Mazzoni wrote: > On 2011/01/06 02:06:48, Peter Kasting wrote: > > Nit: A comment about what this function's responsibilities are would also be > > nice. > > What's the reason for adding a static Create method when the constructor > sufficed before? Would it make sense to do the same to AutocompleteEditViewWin > for consistency? It will return either AutocompleteEditViewGtk or AutocompleteEditViewViews. I may move this to more common place though, but i'd rather keep it here for now. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:159: int TextWidth() const; On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: Since there's only one definition of this function, it seems better to have > one declaration outside the #ifdefs than one in each branch. Adding "virtual" > in the non-views case isn't going to make any real difference. Done. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:773: #if defined(OS_WIN) On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: While I like const locals, most Chrome code avoids them. You could save a > line, then, by rewriting: > > std::wstring suggestion; > #if defined(OS_WIN) > suggestion = suggested_text_view_->GetText(); > #endif Done. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1252: return true; On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: Wouldn't it be better to call the AutocompleteEditViewGtk and ask it to do > the check that currently begins CommitInstantSuggestion()? Thank you for suggestion. Yes, i'm planning to refactor this piece further (I want to move HasValidSuggestText to AutocompleteEditView) to something similar to your suggestion, but not in this CL. This was just one step towards it, but it is probably immasure and shouldn't be in this CL. I reverted this change. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.h:48: #if defined(OS_WIN) On 2011/01/06 02:06:48, Peter Kasting wrote: > Nit: Leave this section where it was (Chrome convention is to put #if XXX > declarations below all others). Done.
LGTM http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:796: views::View* AutocompleteEditViewGtk::AddToView(views::View* parent) { On 2011/01/06 19:43:35, oshima wrote: > I'm planning > to look into possibility to move SuggestedTextView from location_bar_view to > AutocompleteEditView and i may (or may not) introduce such class that time. I'm > fine to add it now if you want. Please let me know. I'm fine with whatever route you want to do as long as you have some kind of plan. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:807: return CommitInstantSuggestion(); On 2011/01/06 19:43:35, oshima wrote: > On 2011/01/06 02:06:48, Peter Kasting wrote: > > Nit: Can't the body of that function be rolled into this? > > CommitInstantSuggestoni() is used by gtk version. OK, but since this ignores its args anyway, couldn't any calls to that have (std::wstring(), std::wstring()) appended so they'd call here? http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:79: ); On 2011/01/06 19:43:35, oshima wrote: > On 2011/01/06 02:06:48, Peter Kasting wrote: > > Nit: I slightly prefer leaving the ); on the individual lines > > unbalanced parenthesis and brackets confused emacs indentation, so I'd like to > keep this if you don't mind. > I can change it back if you feel strongly about it, so please let me know. I don't feel strongly, but I do think the old way was better. MSVC indentation is constantly confused by style quirks we have and I don't worry much about that; I'm not sure emacs is more important. http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view.h (right): http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:166: // Returns the width in pixels needed to display the current text. The Nit: Might want to move this to the end so that the declaration order in AutocompleteEditViewGtk will match? (Also updating the Windows declaration too) http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right): http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:152: // A factory method to creates AutocompleteEditView instance initialized for Nit: "creates" -> "create an" http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:155: // be added to an option when TextfieldViews is enabled. Nit: "to" -> "as" http://codereview.chromium.org/6036004/diff/16009/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/6036004/diff/16009/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:832: !location_entry_->model()->popup_model()->IsOpen()) { Nit: No {}
Thank you for review. I'll wait for comments from James and Dominic. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:807: return CommitInstantSuggestion(); On 2011/01/06 20:51:38, Peter Kasting wrote: > On 2011/01/06 19:43:35, oshima wrote: > > On 2011/01/06 02:06:48, Peter Kasting wrote: > > > Nit: Can't the body of that function be rolled into this? > > > > CommitInstantSuggestoni() is used by gtk version. > > OK, but since this ignores its args anyway, couldn't any calls to that have > (std::wstring(), std::wstring()) appended so they'd call here? Let me refactor further before making this change. I may be able to merge them. If not, i'll change gtk to use the same interface. http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right): http://codereview.chromium.org/6036004/diff/27001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:79: ); On 2011/01/06 20:51:38, Peter Kasting wrote: > On 2011/01/06 19:43:35, oshima wrote: > > On 2011/01/06 02:06:48, Peter Kasting wrote: > > > Nit: I slightly prefer leaving the ); on the individual lines > > > > unbalanced parenthesis and brackets confused emacs indentation, so I'd like to > > keep this if you don't mind. > > I can change it back if you feel strongly about it, so please let me know. > > I don't feel strongly, but I do think the old way was better. MSVC indentation > is constantly confused by style quirks we have and I don't worry much about > that; I'm not sure emacs is more important. This code is gtk (thus linux) code and I assume there will be more emacs users needs to edit this file often than MSVC users. This part will become obsolete once we migrate, so I'd like to keep this way for now. http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view.h (right): http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:166: // Returns the width in pixels needed to display the current text. The On 2011/01/06 20:51:38, Peter Kasting wrote: > Nit: Might want to move this to the end so that the declaration order in > AutocompleteEditViewGtk will match? (Also updating the Windows declaration too) Done. moved implementation as well. http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right): http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:152: // A factory method to creates AutocompleteEditView instance initialized for On 2011/01/06 20:51:38, Peter Kasting wrote: > Nit: "creates" -> "create an" Done. http://codereview.chromium.org/6036004/diff/16009/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.h:155: // be added to an option when TextfieldViews is enabled. On 2011/01/06 20:51:38, Peter Kasting wrote: > Nit: "to" -> "as" Done. http://codereview.chromium.org/6036004/diff/16009/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/6036004/diff/16009/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:832: !location_entry_->model()->popup_model()->IsOpen()) { On 2011/01/06 20:51:38, Peter Kasting wrote: > Nit: No {} Done.
LGTM On Thu, Jan 6, 2011 at 11:43 AM, <oshima@chromium.org> wrote: > On 2011/01/06 15:03:03, Dominic Mazzoni wrote: > >> Why should this be a separate step? Could it be done in Init? >> > > Same for the function calls above - is there any reason why >> > gtk_widget_show_all > >> and gtk_widget_hide couldn't be done inside of Init()? Or could they >> > be > >> abstracted into another post-init method? >> > > Above functions are not called in gtk impl. I can move them to Init with > ifdefs if you want. I think adding post init for linux_view is overkill. I understand now, thanks. Looks good. > What's the reason for adding a static Create method when the >> > constructor > >> sufficed before? Would it make sense to do the same to >> > AutocompleteEditViewWin > >> for consistency? >> > > It will return either AutocompleteEditViewGtk or > AutocompleteEditViewViews. I may move this to more common place though, > but i'd rather keep it here for now. > If you're planning to move Create into autocomplete_edit_view.h eventually then this makes sense, thanks. - Dominic > > http://codereview.chromium.org/6036004/ >
http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view.h (right): http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:168: const std::wstring& suggested_text) = 0; The implementation of this method on all platforms should be same. Actually, the current implementation is just a call to model()->FinalizeInstantQuery(). So maybe it's better to remove this method from all implementations and just call FinalizeInstantQuery() in location bar view. Then the only thing we need for this View interface is a GetInstantSuggestText() method. Maybe we can add a same method to AutocompleteEditController interface and provide different implementations in location bar classes. What's your opinion about it? http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:175: virtual int TextWidth() const = 0; How about move it out of the #if block? It's necessary for both Views and Gtk ports. http://codereview.chromium.org/6036004/diff/45001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/6036004/diff/45001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:770: bool LocationBarView::OnCommitSuggestedText(const std::wstring& typed_text) { How about remove this method's parameter? The typed text can always be retrieved by calling location_entry_->GetText().
http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view.h (right): http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:168: const std::wstring& suggested_text) = 0; On 2011/01/06 23:01:35, James Su wrote: > The implementation of this method on all platforms should be same. Actually, the > current implementation is just a call to model()->FinalizeInstantQuery(). So > maybe it's better to remove this method from all implementations and just call > FinalizeInstantQuery() in location bar view. Then the only thing we need for > this View interface is a GetInstantSuggestText() method. Maybe we can add a same > method to AutocompleteEditController interface and provide different > implementations in location bar classes. > What's your opinion about it? I really like the idea to have the same implementation, but I need to look into more before making big changes to it. I'm planning to move suggested_text_view to AutocompleteEditView impl, which matches other platforms impl, and it will have some impact on this change too, so I'd like to defer it to future refactoring. http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:175: virtual int TextWidth() const = 0; On 2011/01/06 23:01:35, James Su wrote: > How about move it out of the #if block? It's necessary for both Views and Gtk > ports. I didn't do so because mac doesn't have it. I can add NOTREACHED impl to mac if you want. Please let me know.
And how about to remove the parameter of AutocompleteEditController::OnCommitSuggestedText() method? http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view.h (right): http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:168: const std::wstring& suggested_text) = 0; On 2011/01/06 23:32:45, oshima wrote: > On 2011/01/06 23:01:35, James Su wrote: > > The implementation of this method on all platforms should be same. Actually, > the > > current implementation is just a call to model()->FinalizeInstantQuery(). So > > maybe it's better to remove this method from all implementations and just call > > FinalizeInstantQuery() in location bar view. Then the only thing we need for > > this View interface is a GetInstantSuggestText() method. Maybe we can add a > same > > method to AutocompleteEditController interface and provide different > > implementations in location bar classes. > > What's your opinion about it? > > I really like the idea to have the same implementation, but I need to look into > more before > making big changes to it. I'm planning to move suggested_text_view to > AutocompleteEditView impl, > which matches other platforms impl, and it will have some impact on this change > too, so > I'd like to defer it to future refactoring. Then I'd prefer not to refactor this part for now. It's not worth to add this method temporarily if we are going to remove it later. And I'm wondering how would you move suggest_text_view into the AutocompleteEditView's impl? It seems not doable for AutocompleteEditViewWin. http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:171: virtual void SetInstantSuggestion(const string16& input) = 0; It's probably better to generalize this method for all platforms as well. Mac port has SetSuggestText() for the same purpose. http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:175: virtual int TextWidth() const = 0; On 2011/01/06 23:32:45, oshima wrote: > On 2011/01/06 23:01:35, James Su wrote: > > How about move it out of the #if block? It's necessary for both Views and Gtk > > ports. > > I didn't do so because mac doesn't have it. I can add NOTREACHED impl to mac if > you want. Please let me know. > I'd prefer to move it out of the #if block, as Gtk port also has it.
http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view.h (right): http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:168: const std::wstring& suggested_text) = 0; On 2011/01/06 23:49:08, James Su wrote: > On 2011/01/06 23:32:45, oshima wrote: > > On 2011/01/06 23:01:35, James Su wrote: > > > The implementation of this method on all platforms should be same. Actually, > > the > > > current implementation is just a call to model()->FinalizeInstantQuery(). So > > > maybe it's better to remove this method from all implementations and just > call > > > FinalizeInstantQuery() in location bar view. Then the only thing we need for > > > this View interface is a GetInstantSuggestText() method. Maybe we can add a > > same > > > method to AutocompleteEditController interface and provide different > > > implementations in location bar classes. > > > What's your opinion about it? > > > > I really like the idea to have the same implementation, but I need to look > into > > more before > > making big changes to it. I'm planning to move suggested_text_view to > > AutocompleteEditView impl, > > which matches other platforms impl, and it will have some impact on this > change > > too, so > > I'd like to defer it to future refactoring. > > Then I'd prefer not to refactor this part for now. It's not worth to add this > method temporarily if we are going to remove it later. I can't eliminate Gtk's type without common method. It's pertty common to add/update/remove methods during refactoring (and you'll never know the final form until you finish it), so I think it's okay to add this now even if we may modify it later. > And I'm wondering how would you move suggest_text_view into the > AutocompleteEditView's impl? It seems not doable for AutocompleteEditViewWin. It's possible. Whether or not it will be LGTM by scott is different story though :) I think it's good move because it will make all implementation consistent (mac and gtk has this info inside AutocompleteEditView, and Views impl will be the same). http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:171: virtual void SetInstantSuggestion(const string16& input) = 0; On 2011/01/06 23:49:08, James Su wrote: > It's probably better to generalize this method for all platforms as well. Mac > port has SetSuggestText() for the same purpose. Good idea. I'll do it, but in separate CL. I'd like to avoid touching mac file in this CL. http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view.h:175: virtual int TextWidth() const = 0; On 2011/01/06 23:49:08, James Su wrote: > On 2011/01/06 23:32:45, oshima wrote: > > On 2011/01/06 23:01:35, James Su wrote: > > > How about move it out of the #if block? It's necessary for both Views and > Gtk > > > ports. > > > > I didn't do so because mac doesn't have it. I can add NOTREACHED impl to mac > if > > you want. Please let me know. > > > I'd prefer to move it out of the #if block, as Gtk port also has it. Ok, but let me do it in separate CL, same reason as above. http://codereview.chromium.org/6036004/diff/45001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/6036004/diff/45001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:770: bool LocationBarView::OnCommitSuggestedText(const std::wstring& typed_text) { On 2011/01/06 23:01:35, James Su wrote: > How about remove this method's parameter? The typed text can always be retrieved > by calling location_entry_->GetText(). Which assumes that controller has these information, which is not a part of AutocompleteEditController contract, so I don't think it's good idea. Let me proceed step by step. I like your idea to use same code to commit change but I need to digest the code a bit more.
LGTM, as long as you'll do further refactory soon. On 2011/01/07 01:49:39, oshima wrote: > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... > File chrome/browser/autocomplete/autocomplete_edit_view.h (right): > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... > chrome/browser/autocomplete/autocomplete_edit_view.h:168: const std::wstring& > suggested_text) = 0; > On 2011/01/06 23:49:08, James Su wrote: > > On 2011/01/06 23:32:45, oshima wrote: > > > On 2011/01/06 23:01:35, James Su wrote: > > > > The implementation of this method on all platforms should be same. > Actually, > > > the > > > > current implementation is just a call to model()->FinalizeInstantQuery(). > So > > > > maybe it's better to remove this method from all implementations and just > > call > > > > FinalizeInstantQuery() in location bar view. Then the only thing we need > for > > > > this View interface is a GetInstantSuggestText() method. Maybe we can add > a > > > same > > > > method to AutocompleteEditController interface and provide different > > > > implementations in location bar classes. > > > > What's your opinion about it? > > > > > > I really like the idea to have the same implementation, but I need to look > > into > > > more before > > > making big changes to it. I'm planning to move suggested_text_view to > > > AutocompleteEditView impl, > > > which matches other platforms impl, and it will have some impact on this > > change > > > too, so > > > I'd like to defer it to future refactoring. > > > > Then I'd prefer not to refactor this part for now. It's not worth to add this > > method temporarily if we are going to remove it later. > > I can't eliminate Gtk's type without common method. It's pertty common to > add/update/remove methods during refactoring (and you'll never know the final > form until you finish it), so I think it's okay to add this now even if we may > modify it later. > > > > And I'm wondering how would you move suggest_text_view into the > > AutocompleteEditView's impl? It seems not doable for AutocompleteEditViewWin. > > It's possible. Whether or not it will be LGTM by scott is different story though > :) I think it's good move because it will make all implementation consistent > (mac and gtk has this info inside AutocompleteEditView, and Views impl will be > the same). > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... > chrome/browser/autocomplete/autocomplete_edit_view.h:171: virtual void > SetInstantSuggestion(const string16& input) = 0; > On 2011/01/06 23:49:08, James Su wrote: > > It's probably better to generalize this method for all platforms as well. Mac > > port has SetSuggestText() for the same purpose. > > Good idea. I'll do it, but in separate CL. I'd like to avoid touching mac file > in this CL. > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... > chrome/browser/autocomplete/autocomplete_edit_view.h:175: virtual int > TextWidth() const = 0; > On 2011/01/06 23:49:08, James Su wrote: > > On 2011/01/06 23:32:45, oshima wrote: > > > On 2011/01/06 23:01:35, James Su wrote: > > > > How about move it out of the #if block? It's necessary for both Views and > > Gtk > > > > ports. > > > > > > I didn't do so because mac doesn't have it. I can add NOTREACHED impl to mac > > if > > > you want. Please let me know. > > > > > I'd prefer to move it out of the #if block, as Gtk port also has it. > > Ok, but let me do it in separate CL, same reason as above. > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/ui/views/loc... > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/ui/views/loc... > chrome/browser/ui/views/location_bar/location_bar_view.cc:770: bool > LocationBarView::OnCommitSuggestedText(const std::wstring& typed_text) { > On 2011/01/06 23:01:35, James Su wrote: > > How about remove this method's parameter? The typed text can always be > retrieved > > by calling location_entry_->GetText(). > > Which assumes that controller has these information, which is not a part of > AutocompleteEditController contract, so I don't think it's good idea. > > Let me proceed step by step. I like your idea to use same code to commit change > but I need to > digest the code a bit more.
On Thu, Jan 6, 2011 at 6:28 PM, <suzhe@chromium.org> wrote: > LGTM, as long as you'll do further refactory soon. > > Thanks. I'll continue working on it and well include you in reviewers. - oshima > > On 2011/01/07 01:49:39, oshima wrote: > > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... > >> File chrome/browser/autocomplete/autocomplete_edit_view.h (right): >> > > > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... > >> chrome/browser/autocomplete/autocomplete_edit_view.h:168: const >> std::wstring& >> suggested_text) = 0; >> On 2011/01/06 23:49:08, James Su wrote: >> > On 2011/01/06 23:32:45, oshima wrote: >> > > On 2011/01/06 23:01:35, James Su wrote: >> > > > The implementation of this method on all platforms should be same. >> Actually, >> > > the >> > > > current implementation is just a call to >> > model()->FinalizeInstantQuery(). > >> So >> > > > maybe it's better to remove this method from all implementations and >> > just > >> > call >> > > > FinalizeInstantQuery() in location bar view. Then the only thing we >> need >> for >> > > > this View interface is a GetInstantSuggestText() method. Maybe we >> can >> > add > >> a >> > > same >> > > > method to AutocompleteEditController interface and provide different >> > > > implementations in location bar classes. >> > > > What's your opinion about it? >> > > >> > > I really like the idea to have the same implementation, but I need to >> look >> > into >> > > more before >> > > making big changes to it. I'm planning to move suggested_text_view to >> > > AutocompleteEditView impl, >> > > which matches other platforms impl, and it will have some impact on >> this >> > change >> > > too, so >> > > I'd like to defer it to future refactoring. >> > >> > Then I'd prefer not to refactor this part for now. It's not worth to add >> > this > >> > method temporarily if we are going to remove it later. >> > > I can't eliminate Gtk's type without common method. It's pertty common to >> add/update/remove methods during refactoring (and you'll never know the >> final >> form until you finish it), so I think it's okay to add this now even if we >> may >> modify it later. >> > > > > And I'm wondering how would you move suggest_text_view into the >> > AutocompleteEditView's impl? It seems not doable for >> > AutocompleteEditViewWin. > > It's possible. Whether or not it will be LGTM by scott is different story >> > though > >> :) I think it's good move because it will make all implementation >> consistent >> (mac and gtk has this info inside AutocompleteEditView, and Views impl >> will be >> the same). >> > > > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... > >> chrome/browser/autocomplete/autocomplete_edit_view.h:171: virtual void >> SetInstantSuggestion(const string16& input) = 0; >> On 2011/01/06 23:49:08, James Su wrote: >> > It's probably better to generalize this method for all platforms as >> well. >> > Mac > >> > port has SetSuggestText() for the same purpose. >> > > Good idea. I'll do it, but in separate CL. I'd like to avoid touching mac >> file >> in this CL. >> > > > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/autocomplete... > >> chrome/browser/autocomplete/autocomplete_edit_view.h:175: virtual int >> TextWidth() const = 0; >> On 2011/01/06 23:49:08, James Su wrote: >> > On 2011/01/06 23:32:45, oshima wrote: >> > > On 2011/01/06 23:01:35, James Su wrote: >> > > > How about move it out of the #if block? It's necessary for both >> Views >> > and > >> > Gtk >> > > > ports. >> > > >> > > I didn't do so because mac doesn't have it. I can add NOTREACHED impl >> to >> > mac > >> > if >> > > you want. Please let me know. >> > > >> > I'd prefer to move it out of the #if block, as Gtk port also has it. >> > > Ok, but let me do it in separate CL, same reason as above. >> > > > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/ui/views/loc... > >> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): >> > > > > http://codereview.chromium.org/6036004/diff/45001/chrome/browser/ui/views/loc... > >> chrome/browser/ui/views/location_bar/location_bar_view.cc:770: bool >> LocationBarView::OnCommitSuggestedText(const std::wstring& typed_text) { >> On 2011/01/06 23:01:35, James Su wrote: >> > How about remove this method's parameter? The typed text can always be >> retrieved >> > by calling location_entry_->GetText(). >> > > Which assumes that controller has these information, which is not a part >> of >> AutocompleteEditController contract, so I don't think it's good idea. >> > > Let me proceed step by step. I like your idea to use same code to commit >> > change > >> but I need to >> digest the code a bit more. >> > > > > http://codereview.chromium.org/6036004/ > |
