PopupMenuImpl should get menu items via PopupMenuClient. (issue 1267113002 by tkent@chromium.org)

[tk...@chromium.org]

Reviewers: keishi,

Message:
Keishi, would you review this please?


Description:
PopupMenuImpl should get menu items via PopupMenuClient.

We should not have item iteration code in multiple classes, and should
refer to
HTMLSelectElement::m_listItems.

BUG=515013

Please review this at https://codereview.chromium.org/1267113002/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+45, -30 lines):
M Source/core/html/forms/PopupMenuClient.h
M Source/core/layout/LayoutMenuList.h
M Source/core/layout/LayoutMenuList.cpp
M Source/web/ExternalPopupMenuTest.cpp
M Source/web/PopupMenuImpl.cpp


Index: Source/core/html/forms/PopupMenuClient.h
diff --git a/Source/core/html/forms/PopupMenuClient.h
b/Source/core/html/forms/PopupMenuClient.h
index
a071789468ab51ebd7ff59df9c2db022f7bbf58f..1f7875b0bf5faa4d23e9ddc41aed6dbc5849499f
100644
--- a/Source/core/html/forms/PopupMenuClient.h
+++ b/Source/core/html/forms/PopupMenuClient.h
@@ -33,6 +33,7 @@ namespace blink {
class Element;
class ComputedStyle;

+// TODO(tkent): Remove this interface. crbug.com/515013.
class CORE_EXPORT PopupMenuClient {
public:
virtual ~PopupMenuClient() { }
@@ -40,7 +41,8 @@ public:
virtual void selectionChanged(unsigned listIndex, bool fireEvents =
true) = 0;
virtual void selectionCleared() = 0;

- // TODO(tkent): Introduce itemElement(), and remove itemFoo()
functions.
+ virtual Element& itemElement(unsigned listIndex) const = 0;
+ // TODO(tkent): Remove itemFoo() functions. Use itemElement() instead.
virtual String itemText(unsigned listIndex) const = 0;
virtual String itemToolTip(unsigned listIndex) const = 0;
virtual String itemAccessibilityText(unsigned listIndex) const = 0;
@@ -62,7 +64,6 @@ public:
virtual Element& ownerElement() const = 0;
// computedStyleForItem() returns nullptr only if the owner Document
is not
// active. So, It returns a valid object when we open a popup.
- virtual const ComputedStyle* computedStyleForItem(Element&) const = 0;
virtual const ComputedStyle* computedStyleForItem(unsigned listIndex)
const = 0;

virtual void listBoxSelectItem(int /*listIndex*/, bool
/*allowMultiplySelections*/, bool /*shift*/, bool /*fireOnChangeNow*/ =
true) { ASSERT_NOT_REACHED(); }
Index: Source/core/layout/LayoutMenuList.cpp
diff --git a/Source/core/layout/LayoutMenuList.cpp
b/Source/core/layout/LayoutMenuList.cpp
index
d7ca095422b4bf7009982a7f2006fa03c74e8242..5c3b49f453564aae36a3d36756e33ac5ea8d2024
100644
--- a/Source/core/layout/LayoutMenuList.cpp
+++ b/Source/core/layout/LayoutMenuList.cpp
@@ -400,11 +400,6 @@ Element& LayoutMenuList::ownerElement() const
return *selectElement();
}

-const ComputedStyle* LayoutMenuList::computedStyleForItem(Element&
element) const
-{
- return element.computedStyle() ? element.computedStyle() :
element.ensureComputedStyle();
-}
-
const ComputedStyle* LayoutMenuList::computedStyleForItem(unsigned
listIndex) const
{
Element& element = *selectElement()->listItems()[listIndex];
@@ -440,6 +435,11 @@ void LayoutMenuList::didUpdateActiveOption(int
optionIndex)
document().existingAXObjectCache()->handleUpdateActiveMenuOption(this,
optionIndex);
}

+Element& LayoutMenuList::itemElement(unsigned listIndex) const
+{
+ return *selectElement()->listItems()[listIndex];
+}
+
String LayoutMenuList::itemText(unsigned listIndex) const
{
HTMLSelectElement* select = selectElement();
@@ -500,7 +500,7 @@ bool LayoutMenuList::itemIsDisplayNone(unsigned
listIndex) const
Element& element = *selectElement()->listItems()[listIndex];
if (isHTMLOptionElement(element))
return toHTMLOptionElement(element).isDisplayNone();
- if (const ComputedStyle* style = computedStyleForItem(element))
+ if (const ComputedStyle* style = computedStyleForItem(listIndex))
return style->display() == NONE;
return false;
}
Index: Source/core/layout/LayoutMenuList.h
diff --git a/Source/core/layout/LayoutMenuList.h
b/Source/core/layout/LayoutMenuList.h
index
9d7a3b8b2f806f1eaa8383ffc94e7805315d7ddc..1cb31256fa7b73a159c64bdfedd3736ab9c6a304
100644
--- a/Source/core/layout/LayoutMenuList.h
+++ b/Source/core/layout/LayoutMenuList.h
@@ -81,6 +81,7 @@ private:
void valueChanged(unsigned listIndex, bool fireOnChange = true)
override;
void selectionChanged(unsigned, bool) override { }
void selectionCleared() override { }
+ Element& itemElement(unsigned listIndex) const override;
String itemText(unsigned listIndex) const override;
String itemToolTip(unsigned listIndex) const override;
String itemAccessibilityText(unsigned listIndex) const override;
@@ -100,7 +101,6 @@ private:
bool multiple() const override;
IntRect elementRectRelativeToViewport() const override;
Element& ownerElement() const override;
- const ComputedStyle* computedStyleForItem(Element&) const override;

bool hasLineIfEmpty() const override { return true; }

Index: Source/web/ExternalPopupMenuTest.cpp
diff --git a/Source/web/ExternalPopupMenuTest.cpp
b/Source/web/ExternalPopupMenuTest.cpp
index
3a9e9175a24d7bfa2355dd2c23eae87604f4b5a3..b3ab7fa2c83a3fabd81c9586cc762207dd82434a
100644
--- a/Source/web/ExternalPopupMenuTest.cpp
+++ b/Source/web/ExternalPopupMenuTest.cpp
@@ -36,6 +36,7 @@ public:
void selectionChanged(unsigned listIndex, bool fireEvents = true)
override { }
void selectionCleared() override { }

+ Element& itemElement(unsigned listIndex) const override { return
*m_ownerElement->listItems()[listIndex]; }
String itemText(unsigned listIndex) const override { return
emptyString(); }
String itemToolTip(unsigned listIndex) const override { return
emptyString(); }
String itemAccessibilityText(unsigned listIndex) const override {
return emptyString(); }
@@ -54,7 +55,6 @@ public:
bool multiple() const override { return false; }
IntRect elementRectRelativeToViewport() const override { return
IntRect(); }
Element& ownerElement() const override { return *m_ownerElement; }
- const ComputedStyle* computedStyleForItem(Element& element) const
override { return nullptr; }
const ComputedStyle* computedStyleForItem(unsigned listIndex) const
override
{
Element* element = m_ownerElement->listItems()[listIndex];
Index: Source/web/PopupMenuImpl.cpp
diff --git a/Source/web/PopupMenuImpl.cpp b/Source/web/PopupMenuImpl.cpp
index
05ea02149ce9fd5dbe89ee2d1c3420a26ca28290..2cef174070d796c676f2de52e2b16624fc5fe682
100644
--- a/Source/web/PopupMenuImpl.cpp
+++ b/Source/web/PopupMenuImpl.cpp
@@ -165,6 +165,7 @@ public:
, m_textTransform(style.textTransform())
, m_fontDescription(style.fontDescription())
, m_listIndex(0)
+ , m_isInGroup(false)
, m_buffer(buffer)
{
ASSERT(m_buffer);
@@ -198,6 +199,20 @@ public:
PagePopupClient::addString("},\n", m_buffer);
}

+ void startGroupChildren()
+ {
+ ASSERT(!m_isInGroup);
+ PagePopupClient::addString("children: [", m_buffer);
+ m_isInGroup = true;
+ }
+ void finishGroupIfNecessary()
+ {
+ if (!m_isInGroup)
+ return;
+ PagePopupClient::addString("],},\n", m_buffer);
+ m_isInGroup = false;
+ }
+
int fontSize() const { return m_fontDescription.computedPixelSize(); }
FontStyle fontStyle() const { return m_fontDescription.style(); }
FontVariant fontVariant() const { return m_fontDescription.variant(); }
@@ -210,6 +225,7 @@ public:
const FontDescription& m_fontDescription;

int m_listIndex;
+ bool m_isInGroup;
SharedBuffer* m_buffer;
};

@@ -252,14 +268,18 @@ void PopupMenuImpl::writeDocument(SharedBuffer* data)
ItemIterationContext context(*ownerStyle, data);
context.serializeBaseStyle();
PagePopupClient::addString("children: [\n", data);
- for (HTMLElement& child :
Traversal<HTMLElement>::childrenOf(ownerElement())) {
+ for (; context.m_listIndex < m_client->listSize();
++context.m_listIndex) {
+ Element& child = m_client->itemElement(context.m_listIndex);
+ if (!isHTMLOptGroupElement(child.parentNode()))
+ context.finishGroupIfNecessary();
if (isHTMLOptionElement(child))
addOption(context, toHTMLOptionElement(child));
- if (isHTMLOptGroupElement(child))
+ else if (isHTMLOptGroupElement(child))
addOptGroup(context, toHTMLOptGroupElement(child));
- if (isHTMLHRElement(child))
+ else if (isHTMLHRElement(child))
addSeparator(context, toHTMLHRElement(child));
}
+ context.finishGroupIfNecessary();
PagePopupClient::addString("],\n", data);

addProperty("anchorRectInScreen", anchorRectInScreen, data);
@@ -274,7 +294,7 @@ void PopupMenuImpl::writeDocument(SharedBuffer* data)

void PopupMenuImpl::addElementStyle(ItemIterationContext& context,
HTMLElement& element)
{
- const ComputedStyle* style = m_client->computedStyleForItem(element);
+ const ComputedStyle* style =
m_client->computedStyleForItem(context.m_listIndex);
ASSERT(style);
SharedBuffer* data = context.m_buffer;
// TODO(tkent): We generate unnecessary "style: {\n},\n" even if no
@@ -324,8 +344,7 @@ void PopupMenuImpl::addOption(ItemIterationContext&
context, HTMLOptionElement&
SharedBuffer* data = context.m_buffer;
PagePopupClient::addString("{", data);
addProperty("label", element.text(), data);
- ASSERT(context.m_listIndex == element.listIndex());
- addProperty("value", context.m_listIndex++, data);
+ addProperty("value", context.m_listIndex, data);
if (!element.title().isEmpty())
addProperty("title", element.title(), data);
const AtomicString& ariaLabel =
element.fastGetAttribute(HTMLNames::aria_labelAttr);
@@ -340,7 +359,6 @@ void PopupMenuImpl::addOption(ItemIterationContext&
context, HTMLOptionElement&
void PopupMenuImpl::addOptGroup(ItemIterationContext& context,
HTMLOptGroupElement& element)
{
SharedBuffer* data = context.m_buffer;
- ++context.m_listIndex;
PagePopupClient::addString("{\n", data);
PagePopupClient::addString("type: \"optgroup\",\n", data);
addProperty("label", element.groupLabelText(), data);
@@ -348,21 +366,13 @@ void PopupMenuImpl::addOptGroup(ItemIterationContext&
context, HTMLOptGroupEleme
addProperty("ariaLabel",
element.fastGetAttribute(HTMLNames::aria_labelAttr), data);
addProperty("disabled", element.isDisabledFormControl(), data);
addElementStyle(context, element);
- PagePopupClient::addString("children: [", data);
- for (HTMLElement& child : Traversal<HTMLElement>::childrenOf(element))
{
- if (isHTMLOptionElement(child))
- addOption(context, toHTMLOptionElement(child));
- if (isHTMLHRElement(child))
- addSeparator(context, toHTMLHRElement(child));
- }
- PagePopupClient::addString("],\n", data);
- PagePopupClient::addString("},\n", data);
+ context.startGroupChildren();
+ // We should call ItemIterationContext::finishGroupIfNecessary() later.
}

void PopupMenuImpl::addSeparator(ItemIterationContext& context,
HTMLHRElement& element)
{
SharedBuffer* data = context.m_buffer;
- ++context.m_listIndex;
PagePopupClient::addString("{\n", data);
PagePopupClient::addString("type: \"separator\",\n", data);
addProperty("title", element.title(), data);
@@ -476,14 +486,18 @@ void PopupMenuImpl::update()
ItemIterationContext context(*ownerElement().computedStyle(),
data.get());
context.serializeBaseStyle();
PagePopupClient::addString("children: [", data.get());
- for (HTMLElement& child :
Traversal<HTMLElement>::childrenOf(ownerElement())) {
+ for (; context.m_listIndex < m_client->listSize();
++context.m_listIndex) {
+ Element& child = m_client->itemElement(context.m_listIndex);
+ if (!isHTMLOptGroupElement(child.parentNode()))
+ context.finishGroupIfNecessary();
if (isHTMLOptionElement(child))
addOption(context, toHTMLOptionElement(child));
- if (isHTMLOptGroupElement(child))
+ else if (isHTMLOptGroupElement(child))
addOptGroup(context, toHTMLOptGroupElement(child));
- if (isHTMLHRElement(child))
+ else if (isHTMLHRElement(child))
addSeparator(context, toHTMLHRElement(child));
}
+ context.finishGroupIfNecessary();
PagePopupClient::addString("],\n", data.get());
PagePopupClient::addString("}\n", data.get());
m_popup->postMessage(String::fromUTF8(data->data(), data->size()));

[kei...@chromium.org]

LGTM

https://codereview.chromium.org/1267113002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1267113002/1
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1267113002/1


https://codereview.chromium.org/1267113002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199888

https://codereview.chromium.org/1267113002/