Remove blank items from ExternalPopupMenu

Remove blank items from ExternalPopupMenu

This is a regression caused by https://codereview.chromium.org/415343003/
The list with "display: none" items should be trimmed, before being given
to the platform for display.
Modified test - https://codereview.chromium.org/475733002/

BUG=398051, 403299
R=keishi@chromium.org

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

[spartha, 2014-08-14 09:39:18]

Sorry, this check got missed in the final patch. The regression has been
addressed. PTAL!! Sorry for the inconvinience.

[tkent, 2014-08-14 09:41:24]

Please add C++ unit test.

[spartha, 2014-08-14 11:10:26]

On 2014/08/14 09:41:24, tkent wrote:
> Please add C++ unit test.

Done.

[tkent, 2014-08-14 13:40:14]

On 2014/08/14 11:10:26, spartha wrote:
> On 2014/08/14 09:41:24, tkent wrote:
> > Please add C++ unit test.
> 
> Done.

Please add Source/web/ExternalPopupMenuTest.cpp in Blink repo.  Adding
cross-repo, cross-process test for such simple logic is not good for runtime
cost and maintenance cost.

You may add a static function like |void getPopupMenuInfo(PopupMenuClient&,
WebPopupMenuInfo*)| to improve testability.

[spartha, 2014-08-15 10:28:15]

On 2014/08/14 13:40:14, tkent wrote:
> On 2014/08/14 11:10:26, spartha wrote:
> > On 2014/08/14 09:41:24, tkent wrote:
> > > Please add C++ unit test.
> > 
> > Done.
> 
> Please add Source/web/ExternalPopupMenuTest.cpp in Blink repo.  Adding
> cross-repo, cross-process test for such simple logic is not good for runtime
> cost and maintenance cost.
> 
> You may add a static function like |void getPopupMenuInfo(PopupMenuClient&,
> WebPopupMenuInfo*)| to improve testability.

Are you suggesting implementing getPopupMenuInfo again in
ExternalPopupMenuTest.cpp?
Would that not be a problem, if the ExternalPopupMenu changes, we would have to
make
changes at 2 places?

[tkent, 2014-08-15 10:43:28]

On 2014/08/15 10:28:15, spartha wrote:
> > Please add Source/web/ExternalPopupMenuTest.cpp in Blink repo.  Adding
> > cross-repo, cross-process test for such simple logic is not good for runtime
> > cost and maintenance cost.
> > 
> > You may add a static function like |void getPopupMenuInfo(PopupMenuClient&,
> > WebPopupMenuInfo*)| to improve testability.
> 
> Are you suggesting implementing getPopupMenuInfo again in
> ExternalPopupMenuTest.cpp?
> Would that not be a problem, if the ExternalPopupMenu changes, we would have
to
> make
> changes at 2 places?

No.  It would make no sense.
I mean changing ExternalPopupMenu::getPopupMenuInfo in ExternalPopupMenu.cpp.

[spartha, 2014-08-20 14:57:35]

I have added unit tests as per the earlier review comments. Please take a look,
if the new changes are fine. Thank you!

[tkent, 2014-08-21 00:53:34]

Yeah, Patch Set 2 matches to my intention.

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:34: #include "base/gtest_prod_util.h"
Remove this.  Blink must not include base/ files.
Presubmit task must have shown an error when you uploaded this patch.

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:77: static void
getPopupMenuInfo(WebPopupMenuInfo*, PopupMenuClient*);
PopupMenuClient argument should be |PopupMenuClient&| because it must not be
null.

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:78: static int toPopupMenuItemIndex(int index,
PopupMenuClient*);
Ditto.

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:79: static int toExternalPopupMenuItemIndex(int
index, PopupMenuClient*);
Ditto.

https://codereview.chromium.org/475723002/diff/60001/Source/web/tests/Externa...
File Source/web/tests/ExternalPopupMenuTest.cpp (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/tests/Externa...
Source/web/tests/ExternalPopupMenuTest.cpp:5: #include "config.h"
The main header file for this file is ExternalPopupMenu.h.  So, includes should
be:

#include "config.h"
#include "web/ExternalPopupMenu.h"

#include "platform/PopupMenu.h"
#include "Platform/PopupMenuClient.h"
...

https://codereview.chromium.org/475723002/diff/60001/Source/web/tests/Externa...
Source/web/tests/ExternalPopupMenuTest.cpp:14: using namespace blink;
See #4 of http://www.chromium.org/blink/coding-style#TOC-using-Statements

https://codereview.chromium.org/475723002/diff/60001/Source/web/tests/Externa...
Source/web/tests/ExternalPopupMenuTest.cpp:78: virtual void TearDown() OVERRIDE
{ }
Remove this.  It's unnecessary.

https://codereview.chromium.org/475723002/diff/60001/Source/web/web.gypi
File Source/web/web.gypi (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/web.gypi#newc...
Source/web/web.gypi:265: # FIXME: Move the tests from web/tests/ to appropriate
places.
Do not add ExternalPopupMenuTest.cpp to Source/web/tests/.  It should be in
Source/web/.

[spartha, 2014-08-21 06:33:23]

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:34: #include "base/gtest_prod_util.h"
On 2014/08/21 00:53:33, tkent wrote:
> Remove this.  Blink must not include base/ files.
> Presubmit task must have shown an error when you uploaded this patch.
It did give me a DEPS error. Other than that, it did not crib about anything
else. Reason for inclusion is to use the FRIEND_TEST_XXX macros to test the
private member functions. I see there are no helpers on the same lines available
in blink. Will adding something similar be a good idea?

[tkent, 2014-08-21 06:40:05]

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:34: #include "base/gtest_prod_util.h"
On 2014/08/21 06:33:23, spartha wrote:
> On 2014/08/21 00:53:33, tkent wrote:
> > Remove this.  Blink must not include base/ files.
> > Presubmit task must have shown an error when you uploaded this patch.
> It did give me a DEPS error. Other than that, it did not crib about anything
> else. Reason for inclusion is to use the FRIEND_TEST_XXX macros to test the
> private member functions. I see there are no helpers on the same lines
available
> in blink. Will adding something similar be a good idea?

We can use FRIEND_TEST defined in gtest/gtest_prod.h, or make the static
functions public.

[spartha, 2014-08-21 07:07:26]

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:34: #include "base/gtest_prod_util.h"
On 2014/08/21 06:40:05, tkent wrote:
> On 2014/08/21 06:33:23, spartha wrote:
> > On 2014/08/21 00:53:33, tkent wrote:
> > > Remove this.  Blink must not include base/ files.
> > > Presubmit task must have shown an error when you uploaded this patch.
> > It did give me a DEPS error. Other than that, it did not crib about anything
> > else. Reason for inclusion is to use the FRIEND_TEST_XXX macros to test the
> > private member functions. I see there are no helpers on the same lines
> available
> > in blink. Will adding something similar be a good idea?
> 
> We can use FRIEND_TEST defined in gtest/gtest_prod.h, or make the static
> functions public.
> 
I see that FRIEND_TEST is not encouraged to be used. presubmit.py has a check
against using FRIEND_TEST. Is it ok to make the members public only for testing?

[tkent, 2014-08-21 07:09:31]

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:34: #include "base/gtest_prod_util.h"
On 2014/08/21 07:07:26, spartha wrote:
> On 2014/08/21 06:40:05, tkent wrote:
> > On 2014/08/21 06:33:23, spartha wrote:
> > > On 2014/08/21 00:53:33, tkent wrote:
> > > > Remove this.  Blink must not include base/ files.
> > > > Presubmit task must have shown an error when you uploaded this patch.
> > > It did give me a DEPS error. Other than that, it did not crib about
anything
> > > else. Reason for inclusion is to use the FRIEND_TEST_XXX macros to test
the
> > > private member functions. I see there are no helpers on the same lines
> > available
> > > in blink. Will adding something similar be a good idea?
> > 
> > We can use FRIEND_TEST defined in gtest/gtest_prod.h, or make the static
> > functions public.
> > 
> I see that FRIEND_TEST is not encouraged to be used. presubmit.py has a check
> against using FRIEND_TEST. Is it ok to make the members public only for
testing?

Oh, I see.  Then, making them public is the only way.
Please add comments like "// This is public for testing."

[spartha, 2014-08-21 07:18:57]

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:34: #include "base/gtest_prod_util.h"
On 2014/08/21 07:09:31, tkent wrote:
> On 2014/08/21 07:07:26, spartha wrote:
> > On 2014/08/21 06:40:05, tkent wrote:
> > > On 2014/08/21 06:33:23, spartha wrote:
> > > > On 2014/08/21 00:53:33, tkent wrote:
> > > > > Remove this.  Blink must not include base/ files.
> > > > > Presubmit task must have shown an error when you uploaded this patch.
> > > > It did give me a DEPS error. Other than that, it did not crib about
> anything
> > > > else. Reason for inclusion is to use the FRIEND_TEST_XXX macros to test
> the
> > > > private member functions. I see there are no helpers on the same lines
> > > available
> > > > in blink. Will adding something similar be a good idea?
> > > 
> > > We can use FRIEND_TEST defined in gtest/gtest_prod.h, or make the static
> > > functions public.
> > > 
> > I see that FRIEND_TEST is not encouraged to be used. presubmit.py has a
check
> > against using FRIEND_TEST. Is it ok to make the members public only for
> testing?
> 
> Oh, I see.  Then, making them public is the only way.
> Please add comments like "// This is public for testing."
> 

Done. Would it not be useful if gtest_prod_util.h is available in blink?

[tkent, 2014-08-21 07:49:01]

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:34: #include "base/gtest_prod_util.h"
On 2014/08/21 07:18:57, spartha wrote:
> Done. Would it not be useful if gtest_prod_util.h is available in blink?

It would be useful, but I don't think we should do anything for it now.  We'll
merge Blink repo into Chromium repo in some months, then we'll allow base/
includes in Blink.

[spartha, 2014-08-21 10:05:44]

PTAL! Thanks

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
File Source/web/ExternalPopupMenu.h (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:77: static void
getPopupMenuInfo(WebPopupMenuInfo*, PopupMenuClient*);
On 2014/08/21 00:53:33, tkent wrote:
> PopupMenuClient argument should be |PopupMenuClient&| because it must not be
> null.

Done.

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:78: static int toPopupMenuItemIndex(int index,
PopupMenuClient*);
On 2014/08/21 00:53:33, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/475723002/diff/60001/Source/web/ExternalPopup...
Source/web/ExternalPopupMenu.h:79: static int toExternalPopupMenuItemIndex(int
index, PopupMenuClient*);
On 2014/08/21 00:53:33, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/475723002/diff/60001/Source/web/tests/Externa...
File Source/web/tests/ExternalPopupMenuTest.cpp (right):

https://codereview.chromium.org/475723002/diff/60001/Source/web/tests/Externa...
Source/web/tests/ExternalPopupMenuTest.cpp:5: #include "config.h"
On 2014/08/21 00:53:33, tkent wrote:
> The main header file for this file is ExternalPopupMenu.h.  So, includes
should
> be:
> 
> #include "config.h"
> #include "web/ExternalPopupMenu.h"
> 
> #include "platform/PopupMenu.h"
> #include "Platform/PopupMenuClient.h"
> ...

Done.

https://codereview.chromium.org/475723002/diff/60001/Source/web/tests/Externa...
Source/web/tests/ExternalPopupMenuTest.cpp:14: using namespace blink;
On 2014/08/21 00:53:33, tkent wrote:
> See #4 of http://www.chromium.org/blink/coding-style#TOC-using-Statements
I have changed the namespace usage pattern. Most of the test cases are in
anonymous namespace. Now this should be compliant with the coding styles.

https://codereview.chromium.org/475723002/diff/60001/Source/web/tests/Externa...
Source/web/tests/ExternalPopupMenuTest.cpp:78: virtual void TearDown() OVERRIDE
{ }
On 2014/08/21 00:53:33, tkent wrote:
> Remove this.  It's unnecessary.

Done.

[tkent, 2014-08-25 00:15:23]

https://codereview.chromium.org/475723002/diff/160001/Source/web/web.gypi
File Source/web/web.gypi (right):

https://codereview.chromium.org/475723002/diff/160001/Source/web/web.gypi#new...
Source/web/web.gypi:265: # FIXME: Move the tests from web/tests/ to appropriate
places.
Please do not add ExternalPopupMenuTest.cpp to Source/web/tests/.  It should be
put to Source/web/.

[spartha, 2014-08-25 05:32:14]

On 2014/08/25 00:15:23, tkent wrote:
> https://codereview.chromium.org/475723002/diff/160001/Source/web/web.gypi
> File Source/web/web.gypi (right):
> 
>
https://codereview.chromium.org/475723002/diff/160001/Source/web/web.gypi#new...
> Source/web/web.gypi:265: # FIXME: Move the tests from web/tests/ to
appropriate
> places.
> Please do not add ExternalPopupMenuTest.cpp to Source/web/tests/.  It should
be
> put to Source/web/.

Done.

[tkent, 2014-08-25 05:33:59]

https://codereview.chromium.org/475723002/diff/180001/Source/web/web.gypi
File Source/web/web.gypi (right):

https://codereview.chromium.org/475723002/diff/180001/Source/web/web.gypi#new...
Source/web/web.gypi:264: 'WebNodeTest.cpp',
ExternalPopupMenuTest.cpp should be inserted before this line.

[spartha, 2014-08-25 05:41:04]

https://codereview.chromium.org/475723002/diff/180001/Source/web/web.gypi
File Source/web/web.gypi (right):

https://codereview.chromium.org/475723002/diff/180001/Source/web/web.gypi#new...
Source/web/web.gypi:264: 'WebNodeTest.cpp',
On 2014/08/25 05:33:59, tkent wrote:
> ExternalPopupMenuTest.cpp should be inserted before this line.

Done.

[tkent, 2014-08-25 06:03:56]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-08-25 06:03:57]

lgtm

[commit-bot: I haz the power, 2014-08-25 06:04:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/475723002/200001

[commit-bot: I haz the power, 2014-08-25 06:05:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 06:05:04]

Failed to apply patch for Source/web/ExternalPopupMenu.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/ExternalPopupMenu.cpp
  Hunk #1 succeeded at 71 with fuzz 1 (offset -1 lines).
  Hunk #5 FAILED at 181.
  Hunk #6 FAILED at 209.
  Hunk #7 succeeded at 231 (offset -3 lines).
  2 out of 7 hunks FAILED -- saving rejects to file
Source/web/ExternalPopupMenu.cpp.rej

Patch:       Source/web/ExternalPopupMenu.cpp
Index: Source/web/ExternalPopupMenu.cpp
diff --git a/Source/web/ExternalPopupMenu.cpp b/Source/web/ExternalPopupMenu.cpp
index
f2252ba3bd196b8381782cf01892fa21aef8d734..87874a7aabbc78d7d48f6b08c8ecafb21d8b696a
100644
--- a/Source/web/ExternalPopupMenu.cpp
+++ b/Source/web/ExternalPopupMenu.cpp
@@ -72,7 +72,7 @@ void ExternalPopupMenu::show(const FloatQuad& controlPosition,
const IntSize&, i
     }
 
     WebPopupMenuInfo info;
-    getPopupMenuInfo(&info);
+    getPopupMenuInfo(info, *m_popupMenuClient);
     if (info.items.isEmpty())
         return;
     m_webExternalPopupMenu = m_webView.client()->createExternalPopupMenu(info,
this);
@@ -126,7 +126,7 @@ void ExternalPopupMenu::disconnectClient()
 void ExternalPopupMenu::didChangeSelection(int index)
 {
     if (m_popupMenuClient)
-        m_popupMenuClient->selectionChanged(toPopupMenuItemIndex(index));
+        m_popupMenuClient->selectionChanged(toPopupMenuItemIndex(index,
*m_popupMenuClient));
 }
 
 void ExternalPopupMenu::didAcceptIndex(int index)
@@ -134,7 +134,7 @@ void ExternalPopupMenu::didAcceptIndex(int index)
     // Calling methods on the PopupMenuClient might lead to this object being
     // derefed. This ensures it does not get deleted while we are running this
     // method.
-    int popupMenuItemIndex = toPopupMenuItemIndex(index);
+    int popupMenuItemIndex = toPopupMenuItemIndex(index, *m_popupMenuClient);
     RefPtr<ExternalPopupMenu> guard(this);
 
     if (m_popupMenuClient) {
@@ -160,7 +160,7 @@ void ExternalPopupMenu::didAcceptIndices(const
WebVector<int>& indices)
         m_popupMenuClient->valueChanged(static_cast<unsigned>(-1), true);
     else {
         for (size_t i = 0; i < indices.size(); ++i)
-           
m_popupMenuClient->listBoxSelectItem(toPopupMenuItemIndex(indices[i]), (i > 0),
false, (i == indices.size() - 1));
+           
m_popupMenuClient->listBoxSelectItem(toPopupMenuItemIndex(indices[i],
*m_popupMenuClient), (i > 0), false, (i == indices.size() - 1));
     }
 
     // The call to valueChanged above might have lead to a call to
@@ -181,27 +181,27 @@ void ExternalPopupMenu::didCancel()
     m_webExternalPopupMenu = 0;
 }
 
-void ExternalPopupMenu::getPopupMenuInfo(WebPopupMenuInfo* info)
+void ExternalPopupMenu::getPopupMenuInfo(WebPopupMenuInfo& info,
PopupMenuClient& popupMenuClient)
 {
-    int itemCount = m_popupMenuClient->listSize();
+    int itemCount = popupMenuClient.listSize();
     int count = 0;
     Vector<WebMenuItemInfo> items(static_cast<size_t>(itemCount));
     for (int i = 0; i < itemCount; ++i) {
-        PopupMenuStyle style = m_popupMenuClient->itemStyle(i);
+        PopupMenuStyle style = popupMenuClient.itemStyle(i);
         if (style.isDisplayNone())
             continue;
 
         WebMenuItemInfo& popupItem = items[count++];
-        popupItem.label = m_popupMenuClient->itemText(i);
-        popupItem.toolTip = m_popupMenuClient->itemToolTip(i);
-        if (m_popupMenuClient->itemIsSeparator(i))
+        popupItem.label = popupMenuClient.itemText(i);
+        popupItem.toolTip = popupMenuClient.itemToolTip(i);
+        if (popupMenuClient.itemIsSeparator(i))
             popupItem.type = WebMenuItemInfo::Separator;
-        else if (m_popupMenuClient->itemIsLabel(i))
+        else if (popupMenuClient.itemIsLabel(i))
             popupItem.type = WebMenuItemInfo::Group;
         else
             popupItem.type = WebMenuItemInfo::Option;
-        popupItem.enabled = m_popupMenuClient->itemIsEnabled(i);
-        popupItem.checked = m_popupMenuClient->itemIsSelected(i);
+        popupItem.enabled = popupMenuClient.itemIsEnabled(i);
+        popupItem.checked = popupMenuClient.itemIsSelected(i);
         if (style.textDirection() == blink::RTL)
             popupItem.textDirection = WebTextDirectionRightToLeft;
         else
@@ -209,24 +209,25 @@ void ExternalPopupMenu::getPopupMenuInfo(WebPopupMenuInfo*
info)
         popupItem.hasTextDirectionOverride = style.hasTextDirectionOverride();
     }
 
-    info->itemHeight =
m_popupMenuClient->menuStyle().font().fontMetrics().height();
-    info->itemFontSize =
static_cast<int>(m_popupMenuClient->menuStyle().font().fontDescription().computedSize());
-    info->selectedIndex =
toExternalPopupMenuItemIndex(m_popupMenuClient->selectedIndex());
-    info->rightAligned = m_popupMenuClient->menuStyle().textDirection() ==
blink::RTL;
-    info->allowMultipleSelection = m_popupMenuClient->multiple();
-    info->items = items;
+    info.itemHeight =
popupMenuClient.menuStyle().font().fontMetrics().height();
+    info.itemFontSize =
static_cast<int>(popupMenuClient.menuStyle().font().fontDescription().computedSize());
+    info.selectedIndex =
toExternalPopupMenuItemIndex(popupMenuClient.selectedIndex(), popupMenuClient);
+    info.rightAligned = popupMenuClient.menuStyle().textDirection() ==
blink::RTL;
+    info.allowMultipleSelection = popupMenuClient.multiple();
+    if (count < itemCount)
+        items.shrink(count);
+    info.items = items;
 }
 
-int ExternalPopupMenu::toPopupMenuItemIndex(int externalPopupMenuItemIndex)
+int ExternalPopupMenu::toPopupMenuItemIndex(int externalPopupMenuItemIndex,
PopupMenuClient& popupMenuClient)
 {
-    ASSERT(m_popupMenuClient);
     if (externalPopupMenuItemIndex < 0)
         return externalPopupMenuItemIndex;
 
-    int itemCount = m_popupMenuClient->listSize();
+    int itemCount = popupMenuClient.listSize();
     int indexTracker = 0;
     for (int i = 0; i < itemCount ; ++i) {
-        if (m_popupMenuClient->itemStyle(i).isDisplayNone())
+        if (popupMenuClient.itemStyle(i).isDisplayNone())
             continue;
         if (indexTracker++ == externalPopupMenuItemIndex)
             return i;
@@ -234,16 +235,15 @@ int ExternalPopupMenu::toPopupMenuItemIndex(int
externalPopupMenuItemIndex)
     return -1;
 }
 
-int ExternalPopupMenu::toExternalPopupMenuItemIndex(int popupMenuItemIndex)
+int ExternalPopupMenu::toExternalPopupMenuItemIndex(int popupMenuItemIndex,
PopupMenuClient& popupMenuClient)
 {
-    ASSERT(m_popupMenuClient);
     if (popupMenuItemIndex < 0)
         return popupMenuItemIndex;
 
-    int itemCount = m_popupMenuClient->listSize();
+    int itemCount = popupMenuClient.listSize();
     int indexTracker = 0;
     for (int i = 0; i < itemCount; ++i) {
-        if (m_popupMenuClient->itemStyle(i).isDisplayNone())
+        if (popupMenuClient.itemStyle(i).isDisplayNone())
             continue;
         if (popupMenuItemIndex == i)
             return indexTracker;

[spartha, 2014-08-25 11:23:34]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-08-25 11:24:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/475723002/220001

[commit-bot: I haz the power, 2014-08-25 12:27:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/24196)

[commit-bot: I haz the power, 2014-08-25 13:31:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 13:31:44]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/24206)

[spartha, 2014-08-25 14:28:39]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-08-25 14:28:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/475723002/220001

[commit-bot: I haz the power, 2014-08-25 15:39:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/24220)

[commit-bot: I haz the power, 2014-08-25 16:44:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 16:44:33]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/24230)

[tkent, 2014-08-25 17:47:40]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-08-25 17:48:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/475723002/220001

[commit-bot: I haz the power, 2014-08-25 18:35:50]

Committed patchset #6 (220001) as 180861