New issue
Advanced search Search tips

Issue 738389 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

If both toHTMLElement templates are in scope at the same time, they can't be used on HTMLObjectElement

Project Member Reported by brat...@opera.com, Jun 30 2017

Issue description

There are three different ToElement functions in Blink core that accept pointers:

template <typename T>
inline T* ToElement(Node* node);

template <typename T>
inline const T* ToElement(const Node* node);

template <typename T>
inline const T* ToElement(const ListedELement*);

Two of them are in core/dom/Element.h, the last one in core/html/HTMLObjectElement.h.

If code includes both headers and try to use one of the casting/conversion macros you will get a compilation error:

In file included from gen/third_party/WebKit/Source/core/exported/exported_jumbo_1.cc:60:
gen/third_party/WebKit/Source/core/exported/../../../../../../../../third_party/WebKit/Source/core/exported/WebSearchableFormData.cpp:170:24: error: call to 'ToElement' is ambiguous
        text_element = toHTMLInputElement(&control);
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
gen/blink/core/HTMLElementTypeHelpers.h:1898:31: note: expanded from macro 'toHTMLInputElement'
#define toHTMLInputElement(x) blink::ToElement<blink::HTMLInputElement>(x)
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/core/dom/Element.h:1005:11: note: candidate function [with T = blink::HTMLInputElement]
inline T* ToElement(Node* node) {
          ^
../../third_party/WebKit/Source/core/dom/Element.h:1015:17: note: candidate function [with T = blink::HTMLInputElement]
inline const T* ToElement(const Node* node) {
                ^
../../third_party/WebKit/Source/core/html/HTMLObjectElement.h:131:17: note: candidate function [with T = blink::HTMLInputElement]
inline const T* ToElement(const ListedElement*);

This happens when jumbo compiling core/exported unless you exclude WebSearchableFormData.cpp because it uses macros toHTMLSelectElement and toHTMLInputElement that expands to calls to blink::ToElement and it is used on objects that are both Nodes and ListedElements.

I think ToElement for ListedElement should be renamed but it's a small mess of templates and macros so I don't know how far such a rename would spread.
 

Comment 1 by tkent@chromium.org, Jul 3 2017

Labels: Hotlist-CodeHealth
Status: Available (was: Untriaged)
https://codereview.chromium.org/175693007  This CL added |const T* ToElement(const ListedElement*)|.

Renaming ToElement<HTMLObjectElement>(const ListedElement...) (to ToHTMLObjectElementFromListedElement?) and removing unimplemented templates would be reasonable.

Project Member

Comment 2 by bugdroid1@chromium.org, Jul 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62bc3d1efebf3a5d0217409656189c5ec43ec24a

commit 62bc3d1efebf3a5d0217409656189c5ec43ec24a
Author: Daniel Bratell <bratell@opera.com>
Date: Wed Jul 12 09:38:50 2017

Rename ToElement<>(ListedElement) to ToHTMLObjectElementFromListedElement

Having two sets of templates named ToElement caused compilation failures
when both templates were visible to the compiler at the same time.
Since one of the templates doesn't benefit from being a template
this patch renames it and makes it an ordinary function.

R=fs@opera.com, tkent@chromium.org

Bug:  738389 
Change-Id: Ia6247ad0f794cc4094f3f73cd7492ad5d94407c2
Reviewed-on: https://chromium-review.googlesource.com/567141
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Commit-Queue: bratell at Opera <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#485904}
[modify] https://crrev.com/62bc3d1efebf3a5d0217409656189c5ec43ec24a/third_party/WebKit/Source/core/html/HTMLObjectElement.cpp
[modify] https://crrev.com/62bc3d1efebf3a5d0217409656189c5ec43ec24a/third_party/WebKit/Source/core/html/HTMLObjectElement.h
[modify] https://crrev.com/62bc3d1efebf3a5d0217409656189c5ec43ec24a/third_party/WebKit/Source/core/html/ListedElement.cpp

Comment 3 by brat...@opera.com, Sep 5 2017

Owner: brat...@opera.com
Status: Fixed (was: Available)
Fixed in July but forgot to close the bug.

Sign in to add a comment