added CTRL+Click and SHIFT+Click handler for context menu, Back and Forward.

added CTRL+Click handler for context menu.
added Shift+Click handler for context menu and Back & Forward button.

BUG=79128, 40359, 10878
TEST= open and navigate to a couple of pages, and shift+click back or forward button, or right click to show context menu and ctrl+click or shift+click back or forward.

[shinyak (Google), 2011-04-27 01:42:50]

Hi,
I have added the following;
- SHIFT+click handler to Back and Forward button,
- SHIFT+click handler and CTRL+click handler to back and forward in context
menu.

Cloud you review it?

[brettw, 2011-04-28 05:59:29]

Sorry I didn't get to this today, and I'll be out tomorrow. If you're in a
hurry, you can ask pkasting, otherwise I'll look at this on Friday.

[shinyak (Google), 2011-04-28 07:51:31]

brettw,

It's ok if tomorrow.

On Wed, Apr 27, 2011 at 10:59 PM, <brettw@chromium.org> wrote:

> Sorry I didn't get to this today, and I'll be out tomorrow. If you're in a
> hurry, you can ask pkasting, otherwise I'll look at this on Friday.
>
>
> http://codereview.chromium.org/6893046/
>



-- 
Shinya Kawanaka
shinyak@google.com

[brettw, 2011-04-28 20:43:39]

http://codereview.chromium.org/6893046/diff/1/chrome/browser/tab_contents/ren...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/6893046/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_context_menu.cc:44: #include
"chrome/browser/ui/browser.h"
You can see we've been careful to never call into the browser from the existing
code in this file, because the TabContents layer is at a lower level of
abstraction.

Now that the actual TabContents object has moved out into it's own even-lower
level, it no longer makes sense to call into the TabContentsDelegate just to
call back into the browser (this is what we would have done with your patch
before TabContents moved).

Instead, we should use the TabContentsWrapperDelegate for this, which is
imlpemented by the browser. You can see some code in this file uses the
TabContentsWrapper and it has an easy delegate() getter.

http://codereview.chromium.org/6893046/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_context_menu.cc:1163: void
RenderViewContextMenu::ExecuteCommandWithDisposition(
Be sure the implementation order matches the header (this should go below
ExecuteCommand).

I wonder if it would be cleaner to have all of the command handling in this
version and have ExecuteCommand just forward here with a disposition of
CURRENT_TAB. Would that mean there's less code?

[shinyak (Google), 2011-05-02 08:15:26]

Brettw,

I have fixed the code not to use Browser but to use TabContentsWrapperDelegate
instead.

> I wonder if it would be cleaner to have all of the command handling in this
> version and have ExecuteCommand just forward here with a disposition of
> CURRENT_TAB. Would that mean there's less code?

I have tried it, but ui::SimpleMenuModel::Delegate is used in several places,
and some of them does not require any disposition. So I thought adding 
ExecuteCommandWithDisposition in all of them is not good decision. However, if
you think it is better, I will change the interface so.

[brettw, 2011-05-03 19:43:53]

http://codereview.chromium.org/6893046/diff/9/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/6893046/diff/9/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:1319: Browser* browser = new
Browser(Browser::TYPE_NORMAL, profile_);
It seems like this logic should be inside GetOrCloneTabForDisposition so you
don't have to duplicate it. It looks like all you have to do is implement
knowledge of "new window" in that function and the existing code in
GoForward/Back will work.

http://codereview.chromium.org/6893046/diff/9/ui/base/models/simple_menu_mode...
File ui/base/models/simple_menu_model.cc (right):

http://codereview.chromium.org/6893046/diff/9/ui/base/models/simple_menu_mode...
ui/base/models/simple_menu_model.cc:299: if (delegate_)
BTW if you have >1 line in a conditional, we always use {} (even though it's not
syntactically required).

http://codereview.chromium.org/6893046/diff/9/ui/base/models/simple_menu_model.h
File ui/base/models/simple_menu_model.h (right):

http://codereview.chromium.org/6893046/diff/9/ui/base/models/simple_menu_mode...
ui/base/models/simple_menu_model.h:127: virtual void
ActivatedAtWithDisposition(int index, int disposition) OVERRIDE;
I don't understand your changes to this file. How are these new functions hooked
up? Are there changes in the platform-specific menu implementation that are
supposed to call these?

I talked to Ben and we think that the src/ui layer shouldn't know about the
disposition. Here you've passed it as an int to avoid this problem. But probably
we should have a src/ui-specific definition of state flags that the user maps
into a disposition (or whatever else they need it for).

[shinyak (Google), 2011-05-10 03:49:24]

Brettw,

>
http://codereview.chromium.org/6893046/diff/9/chrome/browser/ui/browser.cc#ne...
> chrome/browser/ui/browser.cc:1319: Browser* browser = new
> Browser(Browser::TYPE_NORMAL, profile_);
> It seems like this logic should be inside GetOrCloneTabForDisposition so you
> don't have to duplicate it. It looks like all you have to do is implement
> knowledge of "new window" in that function and the existing code in
> GoForward/Back will work.

I changed the code like the following:

=== CODE ===
TabContents* Browser::GetOrCloneTabForDisposition(                              
                                                                                
                                                                           
       WindowOpenDisposition disposition) {                                     
                                                                                
                                                                           
  TabContentsWrapper* current_tab = GetSelectedTabContentsWrapper();            
                                                                                
                                                                           
                                                                                
                                                                                
                                                                           
  if (disposition == NEW_WINDOW) {                                              
                                                                                
                                                                           
    TabContentsWrapper* new_wrapper = current_tab->Clone();                     
                                                                                
                                                                           
                                                                                
                                                                                
                                                                           
    Browser* browser = Browser::Create(profile_);                               
                                                                                
                                                                           
    browser->AddTab(new_wrapper, PageTransition::LINK);                         
                                                                                
                                                                           
    browser->window()->Show();                                                  
                                                                                
                                                                           
                                                                                
                                                                                
                                                                           
    return new_wrapper->tab_contents();
<snip />
=== END ===

then, wrote the following code.

=== CODE ===
  GetOrCloneTabForDisposition(disposition)->controller().GoForward();
=== END ===

But this code fails doing GoForward(). 
If GoForward() is called before browser->AddTab(...), it works.

Do you have any suggestions to avoid this situation?

[brettw, 2011-05-10 21:14:52]

On Mon, May 9, 2011 at 8:49 PM,  <shinyak@google.com> wrote:
> Brettw,
>
>
>
http://codereview.chromium.org/6893046/diff/9/chrome/browser/ui/browser.cc#ne...
>>
>> chrome/browser/ui/browser.cc:1319: Browser* browser = new
>> Browser(Browser::TYPE_NORMAL, profile_);
>> It seems like this logic should be inside GetOrCloneTabForDisposition so
>> you
>> don't have to duplicate it. It looks like all you have to do is implement
>> knowledge of "new window" in that function and the existing code in
>> GoForward/Back will work.
>
> I changed the code like the following:
>
> === CODE ===
> TabContents* Browser::GetOrCloneTabForDisposition(
>
>
>       WindowOpenDisposition disposition) {
>
>
>  TabContentsWrapper* current_tab = GetSelectedTabContentsWrapper();
>
>
>
>
>
>  if (disposition == NEW_WINDOW) {
>
>
>    TabContentsWrapper* new_wrapper = current_tab->Clone();
>
>
>
>
>
>    Browser* browser = Browser::Create(profile_);
>
>
>    browser->AddTab(new_wrapper, PageTransition::LINK);
>
>
>    browser->window()->Show();
>
>
>
>
>
>    return new_wrapper->tab_contents();
> <snip />
> === END ===
>
> then, wrote the following code.
>
> === CODE ===
>  GetOrCloneTabForDisposition(disposition)->controller().GoForward();
> === END ===
>
> But this code fails doing GoForward().
> If GoForward() is called before browser->AddTab(...), it works.

Why does GoForward fail?

Brett

[shinyak (Google), 2011-05-11 01:24:34]

I don't know the detailed reason yet, but it has no effect.
I'm guessing that something like ownership of TabContent is moved to the new
Browser.

On 2011/05/10 21:14:52, brettw wrote:
> On Mon, May 9, 2011 at 8:49 PM,  <mailto:shinyak@google.com> wrote:
> > Brettw,
> >
> >
> >
>
http://codereview.chromium.org/6893046/diff/9/chrome/browser/ui/browser.cc#ne...
> >>
> >> chrome/browser/ui/browser.cc:1319: Browser* browser = new
> >> Browser(Browser::TYPE_NORMAL, profile_);
> >> It seems like this logic should be inside GetOrCloneTabForDisposition so
> >> you
> >> don't have to duplicate it. It looks like all you have to do is implement
> >> knowledge of "new window" in that function and the existing code in
> >> GoForward/Back will work.
> >
> > I changed the code like the following:
> >
> > === CODE ===
> > TabContents* Browser::GetOrCloneTabForDisposition(
> >
> >
> > &nbsp; &nbsp; &nbsp; WindowOpenDisposition disposition) {
> >
> >
> > &nbsp;TabContentsWrapper* current_tab = GetSelectedTabContentsWrapper();
> >
> >
> >
> >
> >
> > &nbsp;if (disposition == NEW_WINDOW) {
> >
> >
> > &nbsp; &nbsp;TabContentsWrapper* new_wrapper = current_tab->Clone();
> >
> >
> >
> >
> >
> > &nbsp; &nbsp;Browser* browser = Browser::Create(profile_);
> >
> >
> > &nbsp; &nbsp;browser->AddTab(new_wrapper, PageTransition::LINK);
> >
> >
> > &nbsp; &nbsp;browser->window()->Show();
> >
> >
> >
> >
> >
> > &nbsp; &nbsp;return new_wrapper->tab_contents();
> > <snip />
> > === END ===
> >
> > then, wrote the following code.
> >
> > === CODE ===
> > &nbsp;GetOrCloneTabForDisposition(disposition)->controller().GoForward();
> > === END ===
> >
> > But this code fails doing GoForward().
> > If GoForward() is called before browser->AddTab(...), it works.
> 
> Why does GoForward fail?
> 
> Brett

[brettw, 2011-05-11 04:41:58]

On Tue, May 10, 2011 at 6:24 PM,  <shinyak@google.com> wrote:
> I don't know the detailed reason yet, but it has no effect.
> I'm guessing that something like ownership of TabContent is moved to the new
> Browser.

I don't think it should be failing to load, but thinking about it, I'm
not surprised there are some problems. It seems like there will be
state updates sent to the parent that will be missed if there's no
window attached yet.

Can you find a better way to structure the code so there isn't so much
duplication and it still works?

Brett

[shinyak (Google), 2011-05-11 09:08:47]

Brett,

> Can you find a better way to structure the code so there isn't so much
> duplication and it still works?

I have created CreateNewBrowserDisposition to be called after treating
TabContentsWrapper. If C-language has a closure, using it will seem the most
beautiful to ensure doing some routine before and after some code...

BTW, you have wrote the following before;
>
http://codereview.chromium.org/6893046/diff/9/ui/base/models/simple_menu_mode...
> ui/base/models/simple_menu_model.h:127: virtual void
> ActivatedAtWithDisposition(int index, int disposition) OVERRIDE;
> I don't understand your changes to this file. How are these new functions
hooked
> up? Are there changes in the platform-specific menu implementation that are
> supposed to call these?
> 
> I talked to Ben and we think that the src/ui layer shouldn't know about the
> disposition. Here you've passed it as an int to avoid this problem. But
probably
> we should have a src/ui-specific definition of state flags that the user maps
> into a disposition (or whatever else they need it for).

ActivatedAtWithDisposition is originally defined in the super class (MenuModel),
and its default implementation is just to call ActivatedAt with ignoring
disposition. However, I didn't want to ignore the disposition, I overrode it not
to ignore it.

You said "src/ui layer shouldn't know about the disposition" and "we should have
a src/ui-specific definition of state flags". Should I change the MenuModel's
interface too?

[shinyak (Google), 2011-05-11 09:10:45]

I upload the current code. Cloud you check the discussion message and provide
suggestions if any?

[Ben Goodger (Google), 2011-05-13 19:46:26]

http://codereview.chromium.org/6893046/diff/13002/ui/base/models/simple_menu_...
File ui/base/models/simple_menu_model.h (right):

http://codereview.chromium.org/6893046/diff/13002/ui/base/models/simple_menu_...
ui/base/models/simple_menu_model.h:127: virtual void
ActivatedAtWithDisposition(int index, int disposition) OVERRIDE;
This is an implicit coupling between this model code and a WebKit concept -
WindowOpenDisposition. Even if you don't expose the type here, the
calling/receiving code expects it.

Why does the model need to know about the disposition?

In fact, who even calls this method? I don't see any callers in this CL.

[shinyak (Google), 2011-05-16 08:47:59]

Let me consider about WindowOpenDisposition.

BTW, about the method calling, I attach the stack trace.
The SimpleMenuModel::ActivatedAtWithDisposition is called in
MenuGtk::ExecuteCommand.

MenuGtk::ExecuteCommand calls MenuModel::ActivatedAtWithDisposition, and since 
SimpleMenuModel is a subclass of MenuModel,
SimpleMenuModel::ActivatedAtWithDisposition is called.


#0  ui::SimpleMenuModel::ActivatedAtWithDisposition (this=0x7fffb5970238,
index=0, disposition=1)
    at ui/base/models/simple_menu_model.cc:299
#1  0x000000000085d671 in MenuGtk::ExecuteCommand (this=0x7fffb5971190,
model=0x7fffb5970238, id=0)
    at chrome/browser/ui/gtk/menu_gtk.cc:711
#2  0x000000000085d1bb in MenuGtk::OnMenuItemActivated (this=0x7fffb5971190,
menuitem=0x7fffb5b72350)
    at chrome/browser/ui/gtk/menu_gtk.cc:608
#3  0x000000000085df6e in MenuGtk::OnMenuItemActivatedThunk
(sender=0x7fffb5b72350, userdata=0x7fffb5971190)
    at ./chrome/browser/ui/gtk/menu_gtk.h:147
#4  0x00007ffff50c15de in IA__g_closure_invoke (closure=0x7fffb592c360,
return_value=0x0, n_param_values=1,
    param_values=0x7fffe56f5480, invocation_hint=0x7fffffffb350)
    at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gclosure.c:767
#5  0x00007ffff50d5598 in signal_emit_unlocked_R (node=0x7fffeb9634b0,
detail=<value optimized out>,
    instance=<value optimized out>, emission_return=<value optimized out>,
instance_and_params=<value optimized out>)
    at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gsignal.c:3248
#6  0x00007ffff50d6a76 in IA__g_signal_emit_valist (instance=0x7fffb5b72350,
signal_id=<value optimized out>,
    detail=0, var_args=0x7fffffffb540) at
/tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gsignal.c:2981
#7  0x00007ffff50d7033 in IA__g_signal_emit (instance=0x7fffb5970238,
signal_id=0, detail=1)
    at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gsignal.c:3038
#8  0x00007ffff6cb5a2e in IA__gtk_widget_activate (widget=0x7fffb5b72350)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:4982
#9  0x00007ffff6ba956d in IA__gtk_menu_shell_activate_item
(menu_shell=0x7fffe3757020, menu_item=0x7fffb5b72350,
    force_deactivate=<value optimized out>) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkmenushell.c:1293
# skip the rest of stack trace.

[Ben Goodger (Google), 2011-05-16 17:56:11]

But you're not changing MenuGtk so I don't understand where you get the
disposition to pass to ActivatedAtWithDisposition.

[shinyak (Google), 2011-05-17 01:37:13]

Ben,

Let me explain it again.
I think you suspect that I fail uploading MenuGtk, However I have not touched
MenuGtk at all.

1. MenuGtk::ExecuteCommand calls MenuModel::ActivatedAtWithDisposition().
2. Recall that SimpleMenuModel extends MenuModel.
3. MenuGtk::ExecuteCommand actually calls
SimpleMenuModel::ActivatedAtWithDisposition(). In the current implementation
(not mine!), since SimpleMenuModel does not define ActivatedAtWithDisposition(),
MenuModel::ActivatedAtWithDisposition() is called. 
4. In the current implementation (not mine!), MenuModel has the following code.

menu_model.h
  // Called when the item at the specified index has been activated.            
                                                                                
                                                                           
  virtual void ActivatedAt(int index) = 0;

  // Called when the item has been activated with a given disposition (for the  
                                                                                
                                                                           
  // case where the activation involves a navigation).                          
                                                                                
                                                                           
  virtual void ActivatedAtWithDisposition(int index, int disposition);

menu_model.cc

// Default implementation ignores the disposition.                              
                                                                                
                                                                           
void MenuModel::ActivatedAtWithDisposition(int index, int disposition) {
  ActivatedAt(index);
}

5. So, disposition is ignored in the current implementation.

On 2011/05/16 17:56:11, Ben Goodger wrote:
> But you're not changing MenuGtk so I don't understand where you get the
> disposition to pass to ActivatedAtWithDisposition.

[Ben Goodger (Google), 2011-05-17 02:37:12]

Does this mean MenuGtk knows about WindowOpenDisposition?

[shinyak (Google), 2011-05-17 03:49:58]

Yes.

I attach the bit of MenuGtk code.
MenuGtk takes WindowOpenDisposition from mouse button state, and pass it to
MenuModel.

===
void MenuGtk::ExecuteCommand(ui::MenuModel* model, int id) {
  if (delegate_)
    delegate_->CommandWillBeExecuted();

  GdkEvent* event = gtk_get_current_event();
  if (event && event->type == GDK_BUTTON_RELEASE) {
    model->ActivatedAtWithDisposition(
        id, event_utils::DispositionFromEventFlags(event->button.state));
  } else {
    model->ActivatedAt(id);
  }

  if (event)
    gdk_event_free(event);
}

[shinyak (Google), 2011-05-17 03:50:47]

NOTE: This code is from the current implementation (not mine).

[Ben Goodger (Google), 2011-05-17 16:02:30]

Hrm. I would rather the disposition conversion happen at a higher level than
the views/ui code... i.e. so that we don't have to have this hard dependency
on WebKit types.

In this case, I think ActivatedAtWithDisposition should become
ActivatedAtWithFlags, and a platform-independent key state mask be passed
up. The browser code can then do the conversion.

-Ben

On Mon, May 16, 2011 at 8:49 PM, <shinyak@google.com> wrote:

> Yes.
>
> I attach the bit of MenuGtk code.
> MenuGtk takes WindowOpenDisposition from mouse button state, and pass it to
> MenuModel.
>
> ===
> void MenuGtk::ExecuteCommand(ui::MenuModel* model, int id) {
>  if (delegate_)
>    delegate_->CommandWillBeExecuted();
>
>  GdkEvent* event = gtk_get_current_event();
>  if (event && event->type == GDK_BUTTON_RELEASE) {
>    model->ActivatedAtWithDisposition(
>        id, event_utils::DispositionFromEventFlags(event->button.state));
>  } else {
>    model->ActivatedAt(id);
>  }
>
>  if (event)
>    gdk_event_free(event);
>
> }
>
> http://codereview.chromium.org/6893046/
>

[shinyak (Google), 2011-05-23 12:54:27]

Ben,

I'm converting ActivatedAtWithDisposition to ActivatedAtWithFlags, and I'm using
ui/base/events.h for platform-independent key state.

I want to convert the key state to WindowOpenDisposition in
SimpleMenuModel::Delegate (implemented as RenderViewContextMenu), however I
couldn't come up with an appropriate way. I just want to call
event_utils::DispositionFromEventFlags, however it's in views code, it cannot be
linked from RenderViewContextMenu (it's in tab_contents).

Should I move event_utils form views code to somewhere?

[Ben Goodger (Google), 2011-05-23 16:29:32]

Does event_utils contain references to WindowOpenDisposition?

-Ben

On Mon, May 23, 2011 at 5:54 AM, <shinyak@google.com> wrote:

> Ben,
>
> I'm converting ActivatedAtWithDisposition to ActivatedAtWithFlags, and I'm
> using
> ui/base/events.h for platform-independent key state.
>
> I want to convert the key state to WindowOpenDisposition in
> SimpleMenuModel::Delegate (implemented as RenderViewContextMenu), however I
> couldn't come up with an appropriate way. I just want to call
> event_utils::DispositionFromEventFlags, however it's in views code, it
> cannot be
> linked from RenderViewContextMenu (it's in tab_contents).
>
> Should I move event_utils form views code to somewhere?
>
>
> http://codereview.chromium.org/6893046/
>

[shinyak (Google), 2011-05-24 08:11:06]

Yes.

You can see it here.
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/ui/vi...

However, in some case, special event_utils are defined. For example,
gtk has this one.

http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/ui/gt...

[Ben Goodger (Google), 2011-05-24 19:24:14]

I suggest src/chrome/browser/ui then, or src/chrome/browser/ui/views if it's
views-specific.

-Ben

On Tue, May 24, 2011 at 1:11 AM, <shinyak@google.com> wrote:

> Yes.
>
> You can see it here.
>
>
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/ui/vi...
>
> However, in some case, special event_utils are defined. For example,
> gtk has this one.
>
>
>
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/ui/gt...
>
>
>
> http://codereview.chromium.org/6893046/
>

[shinyak (Google), 2011-05-31 06:05:47]

Ben, I have moved event_utils::DispositionFromEventFlags to upper directory,
since it is not view-specific any more. I have created event_disposition_utils.h
to store the function.

Also, I have changed various codes to pass build.


On 2011/05/24 19:24:14, Ben Goodger wrote:
> I suggest src/chrome/browser/ui then, or src/chrome/browser/ui/views if it's
> views-specific.
> 
> -Ben
> 
> On Tue, May 24, 2011 at 1:11 AM, <mailto:shinyak@google.com> wrote:
> 
> > Yes.
> >
> > You can see it here.
> >
> >
>
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/ui/vi...
> >
> > However, in some case, special event_utils are defined. For example,
> > gtk has this one.
> >
> >
> >
>
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/ui/gt...
> >
> >
> >
> > http://codereview.chromium.org/6893046/
> >

[shinyak (Google), 2011-05-31 06:08:00]

I wanted to keep the filename, however, build on Windows won't pass if so. When
linking, obj files were created on the same directory, so the existing obj file
was overwritten.

[brettw, 2011-05-31 06:09:06]

Random drive-by suggestions.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/event_dis...
File chrome/browser/ui/event_disposition_utils.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/event_dis...
chrome/browser/ui/event_disposition_utils.h:5: #ifndef
CHROME_BROWSER_UI_EVENT_DISPOSITION_UTILS_H__
I probably wouldn't have kept "utils" in the filename. That's a bad practice
we're trying to get away from since it doesn't mean anything. Maybe just
event_disposition?

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/event_dis...
chrome/browser/ui/event_disposition_utils.h:11: namespace event_utils {
Seems like this should be "namespace ui"

[Ben Goodger (Google), 2011-05-31 16:00:00]

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1206: case IDC_BACK: {
A general note. It seems weird to me that the context menu handles commands that
don't depend on contextual data locally. It feels like the only TCW delegate
method should be "here's a command (maybe with flags) that I didn't handle,
delegate, figure it out!"

See also all the functions in ExecuteCommand() below that don't use |params_|.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1180: 
I am beginning to feel like all of this code should move to browser_navigator.

I wouldn't add it to Navigate() since these methods are too specialized, but
they can move to the file, and you can add unit tests for all of this in
browser_navigator_browsertest

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1276: WindowOpenDisposition disposition)
nit: curly-brace at the end of this line, not the next one

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1297: {
curly brace to previous line

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:1047: WindowOpenDisposition disposition);
You don't note here what happens to |tab_contents|.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_back_forward_navigator.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_back_forward_navigator.h:13: class
BrowserBackForwardNavigator {
See my note in the previous file about moving this to browser_navigator.h. Also
it is unconventional to have a class with only static methods, so just move the
functions out.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_back_forward_navigator.h:14: 
nit: no new line

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/event_dis...
File chrome/browser/ui/event_disposition_utils.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/event_dis...
chrome/browser/ui/event_disposition_utils.h:11: namespace event_utils {
Probably namespace browser if nothing else. If it's going to be namespace ui it
should be nested inside namespace browser so it doesn't conflict with code from
src/ui.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper_delegate.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_delegate.h:31: virtual void
GoBack(WindowOpenDisposition disposition) = 0;
I am not sure why you are adding these methods to the delegate. It doesn't look
like code in TCW is having to call them. Rather, it's all high level browser
code.

http://codereview.chromium.org/6893046/diff/29001/ui/base/models/menu_model.h
File ui/base/models/menu_model.h (right):

http://codereview.chromium.org/6893046/diff/29001/ui/base/models/menu_model.h...
ui/base/models/menu_model.h:111: // Called when the item has been activated with
an event flags.
I would note here what flags are... is there a place where the possible values
are enumerated?

http://codereview.chromium.org/6893046/diff/29001/views/controls/menu/menu_mo...
File views/controls/menu/menu_model_adapter.cc (right):

http://codereview.chromium.org/6893046/diff/29001/views/controls/menu/menu_mo...
views/controls/menu/menu_model_adapter.cc:58: #if 0
???

[shinyak (Google), 2011-06-09 02:29:04]

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1206: case IDC_BACK: {
I've changed these so that they just call a function.

On 2011/05/31 16:00:00, Ben Goodger wrote:
> A general note. It seems weird to me that the context menu handles commands
that
> don't depend on contextual data locally. It feels like the only TCW delegate
> method should be "here's a command (maybe with flags) that I didn't handle,
> delegate, figure it out!"
> 
> See also all the functions in ExecuteCommand() below that don't use |params_|.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1180: 
I leave it as is now. I would like to do refactoring in another patch.

On 2011/05/31 16:00:00, Ben Goodger wrote:
> I am beginning to feel like all of this code should move to browser_navigator.
> 
> I wouldn't add it to Navigate() since these methods are too specialized, but
> they can move to the file, and you can add unit tests for all of this in
> browser_navigator_browsertest

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1276: WindowOpenDisposition disposition)
On 2011/05/31 16:00:00, Ben Goodger wrote:
> nit: curly-brace at the end of this line, not the next one

Done.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1297: {
On 2011/05/31 16:00:00, Ben Goodger wrote:
> curly brace to previous line

Done.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:1047: WindowOpenDisposition disposition);
On 2011/05/31 16:00:00, Ben Goodger wrote:
> You don't note here what happens to |tab_contents|.

Done.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_back_forward_navigator.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_back_forward_navigator.h:13: class
BrowserBackForwardNavigator {
On 2011/05/31 16:00:00, Ben Goodger wrote:
> See my note in the previous file about moving this to browser_navigator.h.
Also
> it is unconventional to have a class with only static methods, so just move
the
> functions out.

Done.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_back_forward_navigator.h:14: 
On 2011/05/31 16:00:00, Ben Goodger wrote:
> nit: no new line

Done.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/event_dis...
File chrome/browser/ui/event_disposition_utils.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/event_dis...
chrome/browser/ui/event_disposition_utils.h:5: #ifndef
CHROME_BROWSER_UI_EVENT_DISPOSITION_UTILS_H__
On 2011/05/31 06:09:06, brettw wrote:
> I probably wouldn't have kept "utils" in the filename. That's a bad practice
> we're trying to get away from since it doesn't mean anything. Maybe just
> event_disposition?

Done.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/event_dis...
chrome/browser/ui/event_disposition_utils.h:11: namespace event_utils {
On 2011/05/31 16:00:00, Ben Goodger wrote:
> Probably namespace browser if nothing else. If it's going to be namespace ui
it
> should be nested inside namespace browser so it doesn't conflict with code
from
> src/ui.
> 

Done.

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper_delegate.h (right):

http://codereview.chromium.org/6893046/diff/29001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_delegate.h:31: virtual void
GoBack(WindowOpenDisposition disposition) = 0;
Since RenderViewContextMenu, which will call GoBack and GoForward, uses some
delegates instead of Browser itself. So I added these methods here. But
currently I don't think here is appropriate. TabContents seems more appropriate
to have these methods, because it already has OpenURLFromTab, which is a kind of
navigation method. So I moved these functions to TabContentsDelegate.

On 2011/05/31 16:00:00, Ben Goodger wrote:
> I am not sure why you are adding these methods to the delegate. It doesn't
look
> like code in TCW is having to call them. Rather, it's all high level browser
> code.

http://codereview.chromium.org/6893046/diff/29001/ui/base/models/menu_model.h
File ui/base/models/menu_model.h (right):

http://codereview.chromium.org/6893046/diff/29001/ui/base/models/menu_model.h...
ui/base/models/menu_model.h:111: // Called when the item has been activated with
an event flags.
On 2011/05/31 16:00:00, Ben Goodger wrote:
> I would note here what flags are... is there a place where the possible values
> are enumerated?

Done.

http://codereview.chromium.org/6893046/diff/29001/views/controls/menu/menu_mo...
File views/controls/menu/menu_model_adapter.cc (right):

http://codereview.chromium.org/6893046/diff/29001/views/controls/menu/menu_mo...
views/controls/menu/menu_model_adapter.cc:58: #if 0
On 2011/05/31 16:00:00, Ben Goodger wrote:
> ???

Done.

[Ben Goodger (Google), 2011-06-09 16:21:50]

http://codereview.chromium.org/6893046/diff/37001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/6893046/diff/37001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.h:65: virtual void
GoBack(WindowOpenDisposition disposition);
Where is this implemented? I don't see deltas to browser.h making it implement
this method.

Oh wait, now I see, the file already has locally defined GoBack/GoForward
methods that match these signatures.

This is very bad, and very dangerous practice.

There are some things that we do in Chrome code to make it clear where an object
is implementing a method from:

When we have a class (like Browser) that implements an interface (like
TabContentsDelegate), we ensure that all the interface overrides appear next to
each other in the Browser header (and in the .cc). This makes it clear where the
method is coming from.

We also add OVERRIDE to the end of the decl so the compiler can catch mistakes
for us should the method ever get renamed.

With all that said, I think this addition misses the point I was making in my
earlier comments. I don't believe these methods should be on the
TabContentsDelegate. The only method that should be added is
"ExecuteContextMenuCommand" or some such. This gives the delegate a chance to
handle a context menu command. It could return bool true if handled, false if
not handled (and the code in src/content should keep trying to handle it). The
code in Browser would be responsible for calling GoBack/GoForward on itself in
its implementation of this method.

I'm suggesting this because TabContentsDelegate has become far too fat, with a
huge number of methods on it for seemingly random features. In general types
work best when they are more focused rather than an unmaintainable grab-bag of
functionality.

[shinyak (Google), 2011-06-14 04:21:52]

Ben, 

> I don't believe these methods should be on the
> TabContentsDelegate. The only method that should be added is
> "ExecuteContextMenuCommand" or some such.

I'm sorry that I could not understand what you said...
I didn't know that TabContentsDelegate has ExecuteContextMenuCommand.

In this version, I implemented it in Browser class.

[Ben Goodger (Google), 2011-06-14 18:20:48]

Yeah I was requesting you add one and run this code through it.

-Ben

On Mon, Jun 13, 2011 at 9:21 PM, <shinyak@google.com> wrote:

> Ben,
>
>
>  I don't believe these methods should be on the
>> TabContentsDelegate. The only method that should be added is
>> "ExecuteContextMenuCommand" or some such.
>>
>
> I'm sorry that I could not understand what you said...
> I didn't know that TabContentsDelegate has ExecuteContextMenuCommand.
>
> In this version, I implemented it in Browser class.
>
>
>
http://codereview.chromium.**org/6893046/<http://codereview.chromium.org/6893...
>

[shinyak (Google), 2011-06-21 09:25:14]

Ben and brett,

I made this patch applicable to the current trunk.
I would like you to review this patch again.

[shinyak (Google), 2011-06-28 02:57:53]

ping?

[shinyak (Google), 2011-07-04 08:21:51]

On 2011/06/28 02:57:53, shinyak wrote:
> ping?

ping?