Added code to fix visibility of select all option in context menu.

Added code to fix visibility of select all option in context menu.

If context menu is appearing on some input field. Select all option
should appear only if some text is present inside input field.

BUG=395994
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179636

[ankit, 2014-07-22 10:09:37]

PTAL

[ankit, 2014-07-23 12:39:55]

@darin
PTAL

[ankit, 2014-07-24 15:09:45]

PTAL

[darin (slow to review), 2014-07-25 05:11:57]

https://codereview.chromium.org/402403003/diff/1/Source/web/ContextMenuClient...
File Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/402403003/diff/1/Source/web/ContextMenuClient...
Source/web/ContextMenuClientImpl.cpp:225: if (node->isElementNode() &&
(static_cast<Element *>(node))->isFormControlElement()) {
nit: no " " before "*"

https://codereview.chromium.org/402403003/diff/1/Source/web/ContextMenuClient...
Source/web/ContextMenuClientImpl.cpp:228: len =
(static_cast<HTMLTextFormControlElement *>(element))->value().length();
I would probably skip the "len" local variable and just check the result of
.isEmpty(). You can modify editFlags in this scope.

[ankit, 2014-07-25 08:34:41]

@darin
PTAL new patch set

[darin (slow to review), 2014-07-25 16:43:36]

https://codereview.chromium.org/402403003/diff/20001/Source/web/ContextMenuCl...
File Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/402403003/diff/20001/Source/web/ContextMenuCl...
Source/web/ContextMenuClientImpl.cpp:223: if (node->isElementNode() &&
(static_cast<Element*>(node))->isFormControlElement()) {
why do you bother calling isFormControlElement? why not just call
isTextFormControl?

it looks like you can write this code like so:

  if (isHTMLTextFormControlElement(r.innerNonSharedNode())) {
      if
(!toHTMLTextFormControlElement(r.innerNonSharedNode())->value().isEmpty())
          data.editFlags |= WebContextMenuData::CanSelectAll;
  }

that seems simpler, right?

[ankit, 2014-07-28 04:08:06]

https://codereview.chromium.org/402403003/diff/20001/Source/web/ContextMenuCl...
File Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/402403003/diff/20001/Source/web/ContextMenuCl...
Source/web/ContextMenuClientImpl.cpp:223: if (node->isElementNode() &&
(static_cast<Element*>(node))->isFormControlElement()) {
On 2014/07/25 16:43:36, darin wrote:
> why do you bother calling isFormControlElement? why not just call
> isTextFormControl?
> 
> it looks like you can write this code like so:
> 
>   if (isHTMLTextFormControlElement(r.innerNonSharedNode())) {
>       if
> (!toHTMLTextFormControlElement(r.innerNonSharedNode())->value().isEmpty())
>           data.editFlags |= WebContextMenuData::CanSelectAll;
>   }
> 
> that seems simpler, right?

Acknowledged.
I have made changes as suggested and uploaded a new patch. 
PTAL

[ankit, 2014-07-30 05:58:34]

On 2014/07/28 04:08:06, ankit wrote:
>
https://codereview.chromium.org/402403003/diff/20001/Source/web/ContextMenuCl...
> File Source/web/ContextMenuClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/402403003/diff/20001/Source/web/ContextMenuCl...
> Source/web/ContextMenuClientImpl.cpp:223: if (node->isElementNode() &&
> (static_cast<Element*>(node))->isFormControlElement()) {
> On 2014/07/25 16:43:36, darin wrote:
> > why do you bother calling isFormControlElement? why not just call
> > isTextFormControl?
> > 
> > it looks like you can write this code like so:
> > 
> >   if (isHTMLTextFormControlElement(r.innerNonSharedNode())) {
> >       if
> > (!toHTMLTextFormControlElement(r.innerNonSharedNode())->value().isEmpty())
> >           data.editFlags |= WebContextMenuData::CanSelectAll;
> >   }
> > 
> > that seems simpler, right?
> 
> Acknowledged.
> I have made changes as suggested and uploaded a new patch. 
> PTAL

@darin
PTAL

[ankit, 2014-08-01 12:20:46]

@darin
PTAL

[ankit, 2014-08-04 07:29:02]

On 2014/08/01 12:20:46, ankit wrote:
> @darin
> PTAL

PTAL

[ankit, 2014-08-06 05:09:24]

On 2014/08/04 07:29:02, ankit wrote:
> On 2014/08/01 12:20:46, ankit wrote:
> > @darin
> > PTAL
> 
> PTAL

@darin
PTAL

[darin (slow to review), 2014-08-06 14:57:06]

lgtm

[commit-bot: I haz the power, 2014-08-06 14:58:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ankit2.kumar@samsung.com/402403003/40001

[commit-bot: I haz the power, 2014-08-06 17:29:03]

Change committed as 179636

[ankitkumar, 2015-02-24 03:59:01]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/942183005/ by ankitkumar@chromium.org.

The reason for reverting is: This CL is causing Issue 438135 and Issue 450540.
So reverting this..