Refactor the win KeywordEditorView for cross platform friendliness.

Refactor the win KeywordEditorView for cross platform friendliness.

The TableModel is moved to search_engines/template_url_table_model.*

The shared logic, and ownership of the url_model_ and table_model_, is moved to search_engines/keyword_editor_controller.*

BUG=13326
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19421

[mattm, 2009-06-25 21:43:53]

Think this is good but I've only been using trybot for testing.  Would one of
you mind patching it in and testing the dialog a bit?

[sky, 2009-06-25 22:10:32]

To make this easier to review, and so keep history can you copy the controller
from the view. Something like:

cp search_engines/keyword_editor_controller.h /tmp
svn rm search_engines/keyword_editor_controller.h
svn cp views/keyword_editor_view.h search_engines/keyword_editor_controller.h
cp /tmp/keyword_editor_controller.h search_engines

Same thing for the .cc .

Thanks!

http://codereview.chromium.org/146138/diff/1003/2002
File chrome/browser/search_engines/keyword_editor_controller.h (right):

http://codereview.chromium.org/146138/diff/1003/2002#newcode19
Line 19: KeywordEditorController(Profile* profile);
nit: explicit

[mattm, 2009-06-25 22:44:47]

Patched it into an svn checkout and did that (for the relevant other files too.)

http://codereview.chromium.org/146138/diff/1003/2002
File chrome/browser/search_engines/keyword_editor_controller.h (right):

http://codereview.chromium.org/146138/diff/1003/2002#newcode19
Line 19: KeywordEditorController(Profile* profile);
On 2009/06/25 22:10:32, sky wrote:
> nit: explicit

Done.

[sky, 2009-06-26 16:00:18]

http://codereview.chromium.org/146138/diff/2029/2034
File chrome/browser/search_engines/keyword_editor_controller.cc (right):

http://codereview.chromium.org/146138/diff/2029/2034#newcode14
Line 14: : profile_(profile), url_model_(profile->GetTemplateURLModel()) {
each of these should be on its own line.

http://codereview.chromium.org/146138/diff/2029/2034#newcode16
Line 16: url_model_->AddObserver(this);
With the approach you've taken there isn't a need for the controller to be an
observer on the model, instead the tablemodel should directly be an observer of
the templateurltablemodel.

http://codereview.chromium.org/146138/diff/2029/2035
File chrome/browser/search_engines/keyword_editor_controller.h (right):

http://codereview.chromium.org/146138/diff/2029/2035#newcode36
Line 36: // Return true if the given |url| could be made the default.
nit: could -> can

http://codereview.chromium.org/146138/diff/2029/2035#newcode39
Line 39: // Return true if the given |url| could be removed.
nit: could -> can

http://codereview.chromium.org/146138/diff/2029/2035#newcode43
Line 43: void RemoveSearchProvider(int index);
RemoveSearchProvider -> RemoveTemplateURL.

http://codereview.chromium.org/146138/diff/2029/2035#newcode47
Line 47: int MakeDefaultSearchProvider(int index);
MakeDefaultSearchProvider -> MakeDefaultTemplateURL.

http://codereview.chromium.org/146138/diff/2029/2035#newcode50
Line 50: bool loaded();
const

http://codereview.chromium.org/146138/diff/2029/2036
File chrome/browser/search_engines/template_url_table_model.cc (right):

http://codereview.chromium.org/146138/diff/2029/2036#newcode36
Line 36: load_state_ = NOT_LOADED;
can you move load_state_ into the member initializer too.

http://codereview.chromium.org/146138/diff/2029/2037
File chrome/browser/search_engines/template_url_table_model.h (right):

http://codereview.chromium.org/146138/diff/2029/2037#newcode9
Line 9: #include "third_party/skia/include/core/SkBitmap.h"
do you really need this include here? Can't it be class SkBitmap?

http://codereview.chromium.org/146138/diff/2029/2032
File chrome/browser/views/keyword_editor_view.cc (right):

http://codereview.chromium.org/146138/diff/2029/2032#newcode258
Line 258: if (new_index > 0) {
>=

http://codereview.chromium.org/146138/diff/2029/2032#newcode260
Line 260: }
nit: no {}

[mattm, 2009-06-26 18:01:40]

http://codereview.chromium.org/146138/diff/2029/2034
File chrome/browser/search_engines/keyword_editor_controller.cc (right):

http://codereview.chromium.org/146138/diff/2029/2034#newcode14
Line 14: : profile_(profile), url_model_(profile->GetTemplateURLModel()) {
On 2009/06/26 16:00:18, sky wrote:
> each of these should be on its own line.

Done.

http://codereview.chromium.org/146138/diff/2029/2034#newcode16
Line 16: url_model_->AddObserver(this);
On 2009/06/26 16:00:18, sky wrote:
> With the approach you've taken there isn't a need for the controller to be an
> observer on the model, instead the tablemodel should directly be an observer
of
> the templateurltablemodel.

Hm, what about all the gymnastics with adding and removing observer around
modifying the url_model_ so that the table_model doesn't get reloaded?

I guess it could be reorganized so that the tablemodel does the updates of
urlmodel itself, and can thus manage it's own observing state?  Or the
controller could set a flag on the tablemodel to ignore updates?

http://codereview.chromium.org/146138/diff/2029/2035
File chrome/browser/search_engines/keyword_editor_controller.h (right):

http://codereview.chromium.org/146138/diff/2029/2035#newcode36
Line 36: // Return true if the given |url| could be made the default.
On 2009/06/26 16:00:18, sky wrote:
> nit: could -> can

Done.

http://codereview.chromium.org/146138/diff/2029/2035#newcode39
Line 39: // Return true if the given |url| could be removed.
On 2009/06/26 16:00:18, sky wrote:
> nit: could -> can

Done.

http://codereview.chromium.org/146138/diff/2029/2035#newcode43
Line 43: void RemoveSearchProvider(int index);
On 2009/06/26 16:00:18, sky wrote:
> RemoveSearchProvider -> RemoveTemplateURL.

Done.

http://codereview.chromium.org/146138/diff/2029/2035#newcode47
Line 47: int MakeDefaultSearchProvider(int index);
On 2009/06/26 16:00:18, sky wrote:
> MakeDefaultSearchProvider -> MakeDefaultTemplateURL.

Done.

http://codereview.chromium.org/146138/diff/2029/2035#newcode50
Line 50: bool loaded();
On 2009/06/26 16:00:18, sky wrote:
> const

Done.

http://codereview.chromium.org/146138/diff/2029/2036
File chrome/browser/search_engines/template_url_table_model.cc (right):

http://codereview.chromium.org/146138/diff/2029/2036#newcode36
Line 36: load_state_ = NOT_LOADED;
On 2009/06/26 16:00:18, sky wrote:
> can you move load_state_ into the member initializer too.

Done.

http://codereview.chromium.org/146138/diff/2029/2037
File chrome/browser/search_engines/template_url_table_model.h (right):

http://codereview.chromium.org/146138/diff/2029/2037#newcode9
Line 9: #include "third_party/skia/include/core/SkBitmap.h"
On 2009/06/26 16:00:18, sky wrote:
> do you really need this include here? Can't it be class SkBitmap?

Done.

http://codereview.chromium.org/146138/diff/2029/2032
File chrome/browser/views/keyword_editor_view.cc (right):

http://codereview.chromium.org/146138/diff/2029/2032#newcode258
Line 258: if (new_index > 0) {
On 2009/06/26 16:00:18, sky wrote:
> >=

Done.

http://codereview.chromium.org/146138/diff/2029/2032#newcode260
Line 260: }
On 2009/06/26 16:00:18, sky wrote:
> nit: no {}

Done.

[sky, 2009-06-26 18:15:55]

> http://codereview.chromium.org/146138/diff/2029/2034#newcode16
> Line 16: url_model_->AddObserver(this);
> On 2009/06/26 16:00:18, sky wrote:
> > With the approach you've taken there isn't a need for the controller to be
an
> > observer on the model, instead the tablemodel should directly be an observer
> of
> > the templateurltablemodel.
> 
> Hm, what about all the gymnastics with adding and removing observer around
> modifying the url_model_ so that the table_model doesn't get reloaded?
> 
> I guess it could be reorganized so that the tablemodel does the updates of
> urlmodel itself, and can thus manage it's own observing state?  

Exactly. It feels much cleaner this way as it's really only the tablemodel that
needs to know whether it's modifying the templateurlmodel.

  -Scott

[mattm, 2009-06-26 20:51:36]

On 2009/06/26 18:15:55, sky wrote:
> > http://codereview.chromium.org/146138/diff/2029/2034#newcode16
> > Line 16: url_model_->AddObserver(this);
> > On 2009/06/26 16:00:18, sky wrote:
> > > With the approach you've taken there isn't a need for the controller to be
> an
> > > observer on the model, instead the tablemodel should directly be an
observer
> > of
> > > the templateurltablemodel.
> > 
> > Hm, what about all the gymnastics with adding and removing observer around
> > modifying the url_model_ so that the table_model doesn't get reloaded?
> > 
> > I guess it could be reorganized so that the tablemodel does the updates of
> > urlmodel itself, and can thus manage it's own observing state?  
> 
> Exactly. It feels much cleaner this way as it's really only the tablemodel
that
> needs to know whether it's modifying the templateurlmodel.
> 
>   -Scott

done

[sky, 2009-06-26 21:01:51]

LGTM