Fixes bug 1925 (context menus for text selections in...

Fixes bug 1925 (context menus for text selections in
editable boxes and links do not include the Search
Google menu item).

This patch includes the changes originally made by
wei.hoo in http://codereview.chromium.org/1649/show,
but I've additionally fixed the remaining outstanding
review comments (including converting the constants
in context_node_types.h to a bitfield).

BUG=1925

[Peter Kasting, 2009-01-05 20:39:03]

http://codereview.chromium.org/16510/diff/1/2
File chrome/browser/render_view_context_menu.cc (right):

http://codereview.chromium.org/16510/diff/1/2#newcode36
Line 36: if ((type.bits & ContextNode::PAGE) != 0) {
Nit: Don't use "!= 0" here.

Nit: Don't use {} here.

http://codereview.chromium.org/16510/diff/1/4
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/16510/diff/1/4#newcode560
Line 560: if (p.bits == 0) {
Nit: Use "!p.bits" here

http://codereview.chromium.org/16510/diff/1/4#newcode564
Line 564: if ((p.bits & ContextNode::PAGE) != 0) {
Nit: Don't use "!= 0" here

http://codereview.chromium.org/16510/diff/1/6
File webkit/glue/context_node_types.h (right):

http://codereview.chromium.org/16510/diff/1/6#newcode12
Line 12: class ContextNode {
Seems like instead of this class you can now have a struct ContextNode with a
public TypeBit member |type|.  This obviates the need for the inner class.

http://codereview.chromium.org/16510/diff/1/6#newcode15
Line 15: // Allowable bits for the type.
This comment doesn't add much.

http://codereview.chromium.org/16510/diff/1/6#newcode41
Line 41: // Something is selected which can be copied to the clipboards
Nit: "clipboards" -> "clipboard".  Also, isn't this not set when EDITABLE is
set?  If so, you should comment that.

[Brian Duff, 2009-01-20 08:41:24]

http://codereview.chromium.org/16510/diff/1/2
File chrome/browser/render_view_context_menu.cc (right):

http://codereview.chromium.org/16510/diff/1/2#newcode36
Line 36: if ((type.bits & ContextNode::PAGE) != 0) {
On 2009/01/05 20:39:03, pkasting wrote:
> Nit: Don't use "!= 0" here.
> 
> Nit: Don't use {} here.

Done... Need to try harder to get rid of my Java habits ;)

http://codereview.chromium.org/16510/diff/1/4
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/16510/diff/1/4#newcode560
Line 560: if (p.bits == 0) {
On 2009/01/05 20:39:03, pkasting wrote:
> Nit: Use "!p.bits" here

Done.

http://codereview.chromium.org/16510/diff/1/4#newcode564
Line 564: if ((p.bits & ContextNode::PAGE) != 0) {
On 2009/01/05 20:39:03, pkasting wrote:
> Nit: Don't use "!= 0" here

Done.

http://codereview.chromium.org/16510/diff/1/6
File webkit/glue/context_node_types.h (right):

http://codereview.chromium.org/16510/diff/1/6#newcode12
Line 12: class ContextNode {
On 2009/01/05 20:39:03, pkasting wrote:
> Seems like instead of this class you can now have a struct ContextNode with a
> public TypeBit member |type|.  This obviates the need for the inner class.

Done.

http://codereview.chromium.org/16510/diff/1/6#newcode15
Line 15: // Allowable bits for the type.
On 2009/01/05 20:39:03, pkasting wrote:
> This comment doesn't add much.

Removed.

http://codereview.chromium.org/16510/diff/1/6#newcode41
Line 41: // Something is selected which can be copied to the clipboards
On 2009/01/05 20:39:03, pkasting wrote:
> Nit: "clipboards" -> "clipboard".  Also, isn't this not set when EDITABLE is
> set?  If so, you should comment that.

Removed this flag, since it's confusing, wasn't there before, and doesn't really
add much value.

[Peter Kasting, 2009-01-20 18:20:22]

LGTM

http://codereview.chromium.org/16510/diff/619/417
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/16510/diff/619/417#newcode114
Line 114: ContextNode node = ContextNode(ContextNode::NONE);
Nit: Just do
ContextNode node;
...since you provided a null constructor.

http://codereview.chromium.org/16510/diff/619/417#newcode147
Line 147: ContextNode node = ContextNode(ContextNode::NONE);
Nit: See above.

[Brian Duff, 2009-01-21 08:53:35]

http://codereview.chromium.org/16510/diff/619/417
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/16510/diff/619/417#newcode114
Line 114: ContextNode node = ContextNode(ContextNode::NONE);
On 2009/01/20 18:20:22, pkasting wrote:
> Nit: Just do
> ContextNode node;
> ...since you provided a null constructor.

Done.

http://codereview.chromium.org/16510/diff/619/417#newcode147
Line 147: ContextNode node = ContextNode(ContextNode::NONE);
On 2009/01/20 18:20:22, pkasting wrote:
> Nit: See above.

Done.

[Brian Duff, 2009-01-28 04:09:54]

Hi Peter,

Can you (or someone else) commit this for me?

Thanks,
Brian

On 2009/01/21 08:53:35, Brian Duff wrote:
> http://codereview.chromium.org/16510/diff/619/417
> File webkit/glue/context_menu_client_impl.cc (right):
> 
> http://codereview.chromium.org/16510/diff/619/417#newcode114
> Line 114: ContextNode node = ContextNode(ContextNode::NONE);
> On 2009/01/20 18:20:22, pkasting wrote:
> > Nit: Just do
> > ContextNode node;
> > ...since you provided a null constructor.
> 
> Done.
> 
> http://codereview.chromium.org/16510/diff/619/417#newcode147
> Line 147: ContextNode node = ContextNode(ContextNode::NONE);
> On 2009/01/20 18:20:22, pkasting wrote:
> > Nit: See above.
> 
> Done.

[Peter Kasting, 2009-01-28 18:40:00]

On 2009/01/28 04:09:54, Brian Duff wrote:
> Hi Peter,
> 
> Can you (or someone else) commit this for me?

Yep, I was waiting for Ben to comment if he wished, but I'm OK just landing with
my review.

Landed in r8815.