Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
243403006:
Implement contextmenu attribute with basic support of <menu> (Closed)
|
Created:
6 years, 8 months ago by pals Modified:
6 years, 4 months ago CC:
adamk+blink_chromium.org, arv+blink, blink-reviews, dglazkov+blink, watchdog-blink-watchlist_google.com, sof Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImplement contextmenu attribute with basic support of <menu>. This CL implements <menu> element of popup type. The rest of the specification will be implemented as follow up CLs.
contextmenu attribute which enables web pages to specify custom
context menu along with or in place of default platform context
menu for a particular element. The value of the contextmenu
attribute must be the ID of menu element with type popup.
Specifications:
http://www.whatwg.org/specs/web-apps/current-work/#context-menus
Link to "Intent to implement":
http://goo.gl/UtWWY8
BUG=87553
Patch Set 1 #
Total comments: 3
Patch Set 2 : #Patch Set 3 : #
Total comments: 58
Patch Set 4 : #Patch Set 5 : Added unit tests #
Total comments: 30
Patch Set 6 : #
Total comments: 8
Patch Set 7 : Added RuntimeEnabledFeature flag #
Total comments: 4
Patch Set 8 : Added runtime enabled checks #
Total comments: 36
Patch Set 9 : Fixed nits #Patch Set 10 : Rebased #Patch Set 11 : Rebased global-constructors-listing-expected.txt #
Total comments: 68
Patch Set 12 : Move to oilpan #
Total comments: 54
Patch Set 13 : Addressed review comments #
Total comments: 56
Patch Set 14 : Added few more tests and fixed comments #Patch Set 15 : Oilpan #Patch Set 16 : Added test for RelatedEvent constructors #
Total comments: 11
Patch Set 17 : Reflect HTMLMenuElement as per spec #Messages
Total messages: 55 (0 generated)
This is a initial WIP patch. https://codereview.chromium.org/243403006/diff/1/Source/core/page/ContextMenu... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/1/Source/core/page/ContextMenu... Source/core/page/ContextMenuController.cpp:97: m_menuProvider = menuElement->getCustomProvider(); ContextMenuController should provide a way to update the default context menu. Then we can update the default menu with the custom items in handleEvent for "show" in HTMLMenuElement.
https://codereview.chromium.org/243403006/diff/1/Source/core/page/ContextMenu... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/1/Source/core/page/ContextMenu... Source/core/page/ContextMenuController.cpp:93: if (menuElement && menuElement->hasTagName(HTMLNames::menuTag)) { This code doesn't make sense, an HTMLMenuElement will always have the tagName menuTag.
please review the rest of the patch. https://codereview.chromium.org/243403006/diff/1/Source/core/page/ContextMenu... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/1/Source/core/page/ContextMenu... Source/core/page/ContextMenuController.cpp:93: if (menuElement && menuElement->hasTagName(HTMLNames::menuTag)) { On 2014/04/21 16:42:02, esprehn wrote: > This code doesn't make sense, an HTMLMenuElement will always have the tagName > menuTag. Done.
Friendly ping. PTAL.
On 2014/04/30 12:41:58, sanjoy.pal wrote: > Friendly ping. PTAL. This needs a more detailed description. What does this feature do? Where is the spec?
On 2014/05/20 04:54:34, esprehn wrote: > On 2014/04/30 12:41:58, sanjoy.pal wrote: > > Friendly ping. PTAL. > > This needs a more detailed description. What does this feature do? Where is the > spec? Updated the description. Please take a look.
This needs tests, it also contains a bunch of bad casts (security bugs). https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... File Source/core/events/RelatedEvent.cpp (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.cpp:21: */ Use the shorter 3 line one. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.cpp:40: return adoptRef(new RelatedEvent(type, initializer)); Can you just use a default initializer argument and now have so many overloads? https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.cpp:71: return true; I'd put these in the .h file. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.h:36: class RelatedEvent : public Event { FINAL https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.h:38: no new line after sections https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.h:39: static PassRefPtr<RelatedEvent> create() This method doesn't make sense, what's the type of this event? https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.h:59: no new line. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... File Source/core/events/RelatedEvent.idl (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.idl:18: */ short copyright https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.idl:23: [InitializedByEventConstructor] readonly attribute EventTarget ? relatedTarget; no space before ? https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:38: class HTMLMenuElementEventListener : public EventListener { FINAL This class should also have it's own .h and .cpp file. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:41: static const HTMLMenuElementEventListener* cast(const EventListener* listener) This should be an instance method, toHTMLMenuElementEventListener() or a free function of that same name. Remove ::cast() https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:51: HTMLMenuElementEventListener(HTMLMenuElement* menu) missing explicit https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:88: // Add event listeners remove comment, it doesn't add value. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:90: this->addEventListener("show", listener.release(), false); Remove this->, you don't need it. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:108: for (Element* nextElement = ElementTraversal::firstWithin(*menu); nextElement; nextElement = ElementTraversal::next(*nextElement)) { Missing scoping node, this will traverse all the way back up to the Document. You need ::next(*nextElement, menu) https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:114: m_menuItems.append(toHTMLElement(nextElement)); Does the spec really say that anything is a menu item? This makes <div> and <span> menu items? Also this cast is not safe, what if I put an <svg> in there? https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:115: contextMenu.appendItem(ContextMenuItem(ActionType, static_cast<ContextMenuAction>(ContextMenuItemBaseCustomTag + m_menuItems.size() - 1), nextElement->fastGetAttribute(HTMLNames::labelAttr))); This enum stuff doesn't look right. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:41: PassRefPtr<CustomContextMenuProvider> getCustomProvider() { return m_menuProvider; } Getters don't start with "get". Instead you want customProvider() and ensureCustomProvider() where ensure returns a reference and the regular returns a pointer. This shouldn't return PassRefPtr, you're not returning ownership. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:42: void populateContextMenuItems(HTMLMenuElement* menu, ContextMenu& contextMenu); You don't need the argument names, they don't add value. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:43: HTMLElement* getSelectedMenuItem(unsigned menuId); Getters don't start with "get", selectedMenuItem(menuId) https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:50: class CustomContextMenuProvider FINAL : public ContextMenuProvider { This doesn't need to be all inline. Give it its own file .h and .cpp file https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:57: void disconnect() What is this for? https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:68: virtual ~CustomContextMenuProvider() These don't need to be inline. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:92: HTMLMenuElement* m_menu; Does this need to be a ref ptr? What keeps m_menu alive? https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... File Source/core/html/HTMLMenuitemElement.idl (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuitemElement.idl:21: [ Reflect ] readonly attribute DOMString type; Remove space around Reflect https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... Source/core/page/ContextMenuController.cpp:90: if (node && node->isElementNode()) { I think you want isHTMLElement https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... Source/core/page/ContextMenuController.cpp:91: const HTMLElement& element = toHTMLElement(*event->target()->toNode()); This is not safe, it could be an SVG element or just a regular element. https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... Source/core/page/ContextMenuController.cpp:92: if (HTMLMenuElement* menuElement = element.menuElement()) { This probably needs to be a RefPtr, the script below can blow away menuElement and the entire page. https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... Source/core/page/ContextMenuController.cpp:95: EventDispatcher::dispatchEvent(menuElement, EventDispatchMediator::create(relatedEvent)); Are you sure it's safe to synchronously execute script here?
Updated the patch. Tests are pending. Please review. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... File Source/core/events/RelatedEvent.cpp (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.cpp:21: */ On 2014/05/20 05:51:34, esprehn wrote: > Use the shorter 3 line one. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.cpp:40: return adoptRef(new RelatedEvent(type, initializer)); On 2014/05/20 05:51:34, esprehn wrote: > Can you just use a default initializer argument and now have so many overloads? I followed the CustomEvent implementation https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Other events implementations are similar. https://code.google.com/p/chromium/codesearch#search/&q=create%20package:%5Ec... https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.cpp:71: return true; On 2014/05/20 05:51:34, esprehn wrote: > I'd put these in the .h file. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.h:36: class RelatedEvent : public Event { On 2014/05/20 05:51:34, esprehn wrote: > FINAL Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.h:38: On 2014/05/20 05:51:34, esprehn wrote: > no new line after sections Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.h:39: static PassRefPtr<RelatedEvent> create() On 2014/05/20 05:51:34, esprehn wrote: > This method doesn't make sense, what's the type of this event? This is needed here https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... I followed the CustomEvent implementation https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Other events implementations are similar. https://code.google.com/p/chromium/codesearch#search/&q=create%20package:%5Ec... https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.h:59: On 2014/05/20 05:51:34, esprehn wrote: > no new line. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... File Source/core/events/RelatedEvent.idl (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.idl:18: */ On 2014/05/20 05:51:34, esprehn wrote: > short copyright Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/events/Relat... Source/core/events/RelatedEvent.idl:23: [InitializedByEventConstructor] readonly attribute EventTarget ? relatedTarget; On 2014/05/20 05:51:34, esprehn wrote: > no space before ? Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:38: class HTMLMenuElementEventListener : public EventListener { On 2014/05/20 05:51:34, esprehn wrote: > FINAL > > This class should also have it's own .h and .cpp file. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:41: static const HTMLMenuElementEventListener* cast(const EventListener* listener) On 2014/05/20 05:51:34, esprehn wrote: > This should be an instance method, toHTMLMenuElementEventListener() or a free > function of that same name. Remove ::cast() Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:51: HTMLMenuElementEventListener(HTMLMenuElement* menu) On 2014/05/20 05:51:34, esprehn wrote: > missing explicit Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:88: // Add event listeners On 2014/05/20 05:51:34, esprehn wrote: > remove comment, it doesn't add value. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:90: this->addEventListener("show", listener.release(), false); On 2014/05/20 05:51:34, esprehn wrote: > Remove this->, you don't need it. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:108: for (Element* nextElement = ElementTraversal::firstWithin(*menu); nextElement; nextElement = ElementTraversal::next(*nextElement)) { On 2014/05/20 05:51:34, esprehn wrote: > Missing scoping node, this will traverse all the way back up to the Document. > You need ::next(*nextElement, menu) Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:114: m_menuItems.append(toHTMLElement(nextElement)); On 2014/05/20 05:51:34, esprehn wrote: > Does the spec really say that anything is a menu item? This makes <div> and > <span> menu items? > > Also this cast is not safe, what if I put an <svg> in there? Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.cpp:115: contextMenu.appendItem(ContextMenuItem(ActionType, static_cast<ContextMenuAction>(ContextMenuItemBaseCustomTag + m_menuItems.size() - 1), nextElement->fastGetAttribute(HTMLNames::labelAttr))); On 2014/05/20 05:51:34, esprehn wrote: > This enum stuff doesn't look right. I mostly followed the custom contextmenu implementations from inspector, where they specify an Id for the custom menu items. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But we don't have anything similar for menuitem so I am using menuitem index/position as the id. any suggestion for an alternative? https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:41: PassRefPtr<CustomContextMenuProvider> getCustomProvider() { return m_menuProvider; } On 2014/05/20 05:51:34, esprehn wrote: > Getters don't start with "get". Instead you want customProvider() and > ensureCustomProvider() where ensure returns a reference and the regular returns > a pointer. > > This shouldn't return PassRefPtr, you're not returning ownership. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:42: void populateContextMenuItems(HTMLMenuElement* menu, ContextMenu& contextMenu); On 2014/05/20 05:51:34, esprehn wrote: > You don't need the argument names, they don't add value. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:43: HTMLElement* getSelectedMenuItem(unsigned menuId); On 2014/05/20 05:51:34, esprehn wrote: > Getters don't start with "get", selectedMenuItem(menuId) Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:50: class CustomContextMenuProvider FINAL : public ContextMenuProvider { On 2014/05/20 05:51:34, esprehn wrote: > This doesn't need to be all inline. Give it its own file .h and .cpp file Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:57: void disconnect() On 2014/05/20 05:51:34, esprehn wrote: > What is this for? Done. Removed. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:68: virtual ~CustomContextMenuProvider() On 2014/05/20 05:51:34, esprehn wrote: > These don't need to be inline. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuElement.h:92: HTMLMenuElement* m_menu; On 2014/05/20 05:51:34, esprehn wrote: > Does this need to be a ref ptr? What keeps m_menu alive? Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... File Source/core/html/HTMLMenuitemElement.idl (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/html/HTMLMen... Source/core/html/HTMLMenuitemElement.idl:21: [ Reflect ] readonly attribute DOMString type; On 2014/05/20 05:51:34, esprehn wrote: > Remove space around Reflect Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... Source/core/page/ContextMenuController.cpp:90: if (node && node->isElementNode()) { On 2014/05/20 05:51:34, esprehn wrote: > I think you want isHTMLElement Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... Source/core/page/ContextMenuController.cpp:91: const HTMLElement& element = toHTMLElement(*event->target()->toNode()); On 2014/05/20 05:51:34, esprehn wrote: > This is not safe, it could be an SVG element or just a regular element. Changed to isHTMLElement() above. https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... Source/core/page/ContextMenuController.cpp:92: if (HTMLMenuElement* menuElement = element.menuElement()) { On 2014/05/20 05:51:34, esprehn wrote: > This probably needs to be a RefPtr, the script below can blow away menuElement > and the entire page. Done. https://codereview.chromium.org/243403006/diff/60001/Source/core/page/Context... Source/core/page/ContextMenuController.cpp:95: EventDispatcher::dispatchEvent(menuElement, EventDispatchMediator::create(relatedEvent)); On 2014/05/20 05:51:34, esprehn wrote: > Are you sure it's safe to synchronously execute script here? m_menuProvider below which is populated by menu element on receiving this event, so I am sending this event synchronously. I am not sure of the consequences. Do you see any problem here?
On 2014/05/21 07:55:11, sanjoy.pal wrote: > Updated the patch. Tests are pending. Please review. Reviewers are very overloaded. Reviews are for when you actually think the patch is ready to be committed, which means having the tests completed. I'm just saying this so that you know that it's very unlikely anyone will do this review until the patch has tests. There's an exception to this. When you have a patch that you're particularly unsure about, it's OK to post a WIP and make it clear to reviewers that you're just seeing if you're high-level approach is correct. Although, even then, having the tests is nice as the process of writing the tests will show you issues in the approach you're taking. I often discover bugs in my code when I write the tests.
Added both unit tests and browser tests in chromium side https://codereview.chromium.org/296153008/. PTAL.
On 2014/05/23 15:18:17, sanjoy.pal wrote: > Added both unit tests and browser tests in chromium side > https://codereview.chromium.org/296153008/. PTAL. Ping. PTAL.
https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... File Source/core/events/RelatedEvent.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... Source/core/events/RelatedEvent.cpp:11: : relatedTarget(nullptr) You don't need this, RefPtr initializes itself automatically. https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:30: EventTarget* relatedTarget(bool& isNull) const { isNull = !m_relatedTarget; return m_relatedTarget.get(); } I don't know what this method is, but you definitely don't need it. Just check relatedTarget() in callers. https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:35: bool isRelatedEvent() const { return true; } Shouldn't this be virtual? https://codereview.chromium.org/243403006/diff/120001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:38: virtual void contextMenuCleared() OVERRIDE one line. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:912: if (!isHTMLMenuElement(*element)) You can combine all these checks into: return isHTMLMenuElement(element) ? element : 0; you don't even need to null check it then. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:43: addEventListener("show", listener.release(), false); We don't normally add event listeners like this. I'm not sure we want to do this. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:51: CustomContextMenuProvider& HTMLMenuElement::ensureCustomProvider() ensureMenuProvider() https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:63: for (Element* nextElement = ElementTraversal::firstWithin(*menu); nextElement;) { Lets use a while() loop, it'll be more clear. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:80: return m_menuItems[menuId - ContextMenuItemBaseCustomTag]; This unsigned thing is really weird. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:41: void buildCustomProvider(); This method doesn't exist. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:45: HTMLElement* selectedMenuItem(unsigned menuId); I'm not sure I understand the purpose of this. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElementEventListener.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElementEventListener.cpp:13: void HTMLMenuElementEventListener::handleEvent(ExecutionContext*, Event* event) This should be in the defaultEventHandler, we don't add EventListeners in C++ to my knowledge. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElementEventListener.h (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElementEventListener.h:15: class HTMLMenuElementEventListener FINAL : public EventListener { Lets not do this. I see IndexedDB does it, but we really shouldn't be doing this. https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:93: RefPtr<RelatedEvent> relatedEvent = RelatedEvent::create("show"); This should use EventTypeNames. https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:95: EventDispatcher::dispatchEvent(menuElement.get(), EventDispatchMediator::create(relatedEvent.release())); Are you sure it's safe to run script here? Who owns the ContextMenuController? The script here can navigate the page and destroy the current document.
Updated the patch after addressing the review comments. Please take another look. https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... File Source/core/events/RelatedEvent.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... Source/core/events/RelatedEvent.cpp:11: : relatedTarget(nullptr) On 2014/06/06 22:30:41, esprehn wrote: > You don't need this, RefPtr initializes itself automatically. Done. https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:30: EventTarget* relatedTarget(bool& isNull) const { isNull = !m_relatedTarget; return m_relatedTarget.get(); } It is being called by generated files gen/blink/bindings/core/v8/V8RelatedEvent.cpp:53:66: error: no matching function for call to ‘WebCore::RelatedEvent::relatedTarget(bool&)’ https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:35: bool isRelatedEvent() const { return true; } On 2014/06/06 22:30:41, esprehn wrote: > Shouldn't this be virtual? Done. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:38: virtual void contextMenuCleared() OVERRIDE On 2014/06/06 22:30:42, esprehn wrote: > one line. Done. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:912: if (!isHTMLMenuElement(*element)) On 2014/06/06 22:30:42, esprehn wrote: > You can combine all these checks into: > > return isHTMLMenuElement(element) ? element : 0; > > you don't even need to null check it then. Done. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:43: addEventListener("show", listener.release(), false); On 2014/06/06 22:30:42, esprehn wrote: > We don't normally add event listeners like this. I'm not sure we want to do > this. Done. Removed HTMLMenuElementEventListener. Added defaultEventHandler. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:51: CustomContextMenuProvider& HTMLMenuElement::ensureCustomProvider() On 2014/06/06 22:30:42, esprehn wrote: > ensureMenuProvider() Done. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:63: for (Element* nextElement = ElementTraversal::firstWithin(*menu); nextElement;) { On 2014/06/06 22:30:42, esprehn wrote: > Lets use a while() loop, it'll be more clear. Done. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:80: return m_menuItems[menuId - ContextMenuItemBaseCustomTag]; On 2014/06/06 22:30:42, esprehn wrote: > This unsigned thing is really weird. Done. Added checks. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:41: void buildCustomProvider(); On 2014/06/06 22:30:42, esprehn wrote: > This method doesn't exist. Done. Removed. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:45: HTMLElement* selectedMenuItem(unsigned menuId); I am calling this method from Source/core/html/CustomContextMenuProvider.cpp to get the corresponding MenuitemElement for the selected context menu item by the user. Renamed the function as menuItemAt(unsigned menuId) as name was confusing. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElementEventListener.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElementEventListener.cpp:13: void HTMLMenuElementEventListener::handleEvent(ExecutionContext*, Event* event) On 2014/06/06 22:30:42, esprehn wrote: > This should be in the defaultEventHandler, we don't add EventListeners in C++ to > my knowledge. Done. https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElementEventListener.h (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElementEventListener.h:15: class HTMLMenuElementEventListener FINAL : public EventListener { On 2014/06/06 22:30:42, esprehn wrote: > Lets not do this. I see IndexedDB does it, but we really shouldn't be doing > this. Done. Removed. https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:93: RefPtr<RelatedEvent> relatedEvent = RelatedEvent::create("show"); On 2014/06/06 22:30:42, esprehn wrote: > This should use EventTypeNames. Done. https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:95: EventDispatcher::dispatchEvent(menuElement.get(), EventDispatchMediator::create(relatedEvent.release())); On 2014/06/06 22:30:42, esprehn wrote: > Are you sure it's safe to run script here? Who owns the ContextMenuController? > The script here can navigate the page and destroy the current document. Done. The ContextMenuController is owned by Page. If the script navigate the page and destroy the current document, then ContextMenuController does not change. Do we still need async event here because we need to update the already shown context menu in case of async event which makes the existing implementation complex.
On 2014/06/10 10:42:40, sanjoy.pal wrote: > Updated the patch after addressing the review comments. Please take another > look. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... > File Source/core/events/RelatedEvent.cpp (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... > Source/core/events/RelatedEvent.cpp:11: : relatedTarget(nullptr) > On 2014/06/06 22:30:41, esprehn wrote: > > You don't need this, RefPtr initializes itself automatically. > > Done. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... > File Source/core/events/RelatedEvent.h (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... > Source/core/events/RelatedEvent.h:30: EventTarget* relatedTarget(bool& isNull) > const { isNull = !m_relatedTarget; return m_relatedTarget.get(); } > It is being called by generated files > gen/blink/bindings/core/v8/V8RelatedEvent.cpp:53:66: error: no matching function > for call to ‘WebCore::RelatedEvent::relatedTarget(bool&)’ > > https://codereview.chromium.org/243403006/diff/120001/Source/core/events/Rela... > Source/core/events/RelatedEvent.h:35: bool isRelatedEvent() const { return true; > } > On 2014/06/06 22:30:41, esprehn wrote: > > Shouldn't this be virtual? > > Done. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/Custom... > File Source/core/html/CustomContextMenuProvider.h (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/Custom... > Source/core/html/CustomContextMenuProvider.h:38: virtual void > contextMenuCleared() OVERRIDE > On 2014/06/06 22:30:42, esprehn wrote: > > one line. > > Done. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLEl... > File Source/core/html/HTMLElement.cpp (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLEl... > Source/core/html/HTMLElement.cpp:912: if (!isHTMLMenuElement(*element)) > On 2014/06/06 22:30:42, esprehn wrote: > > You can combine all these checks into: > > > > return isHTMLMenuElement(element) ? element : 0; > > > > you don't even need to null check it then. > > Done. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > File Source/core/html/HTMLMenuElement.cpp (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > Source/core/html/HTMLMenuElement.cpp:43: addEventListener("show", > listener.release(), false); > On 2014/06/06 22:30:42, esprehn wrote: > > We don't normally add event listeners like this. I'm not sure we want to do > > this. > > Done. Removed HTMLMenuElementEventListener. Added defaultEventHandler. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > Source/core/html/HTMLMenuElement.cpp:51: CustomContextMenuProvider& > HTMLMenuElement::ensureCustomProvider() > On 2014/06/06 22:30:42, esprehn wrote: > > ensureMenuProvider() > > Done. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > Source/core/html/HTMLMenuElement.cpp:63: for (Element* nextElement = > ElementTraversal::firstWithin(*menu); nextElement;) { > On 2014/06/06 22:30:42, esprehn wrote: > > Lets use a while() loop, it'll be more clear. > > Done. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > Source/core/html/HTMLMenuElement.cpp:80: return m_menuItems[menuId - > ContextMenuItemBaseCustomTag]; > On 2014/06/06 22:30:42, esprehn wrote: > > This unsigned thing is really weird. > > Done. Added checks. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > File Source/core/html/HTMLMenuElement.h (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > Source/core/html/HTMLMenuElement.h:41: void buildCustomProvider(); > On 2014/06/06 22:30:42, esprehn wrote: > > This method doesn't exist. > > Done. Removed. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > Source/core/html/HTMLMenuElement.h:45: HTMLElement* selectedMenuItem(unsigned > menuId); > I am calling this method from > Source/core/html/CustomContextMenuProvider.cpp to get the corresponding > MenuitemElement for the selected context menu item by the user. Renamed the > function as menuItemAt(unsigned menuId) as name was confusing. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > File Source/core/html/HTMLMenuElementEventListener.cpp (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > Source/core/html/HTMLMenuElementEventListener.cpp:13: void > HTMLMenuElementEventListener::handleEvent(ExecutionContext*, Event* event) > On 2014/06/06 22:30:42, esprehn wrote: > > This should be in the defaultEventHandler, we don't add EventListeners in C++ > to > > my knowledge. > > Done. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > File Source/core/html/HTMLMenuElementEventListener.h (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/html/HTMLMe... > Source/core/html/HTMLMenuElementEventListener.h:15: class > HTMLMenuElementEventListener FINAL : public EventListener { > On 2014/06/06 22:30:42, esprehn wrote: > > Lets not do this. I see IndexedDB does it, but we really shouldn't be doing > > this. > > Done. Removed. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... > File Source/core/page/ContextMenuController.cpp (right): > > https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... > Source/core/page/ContextMenuController.cpp:93: RefPtr<RelatedEvent> relatedEvent > = RelatedEvent::create("show"); > On 2014/06/06 22:30:42, esprehn wrote: > > This should use EventTypeNames. > > Done. > > https://codereview.chromium.org/243403006/diff/120001/Source/core/page/Contex... > Source/core/page/ContextMenuController.cpp:95: > EventDispatcher::dispatchEvent(menuElement.get(), > EventDispatchMediator::create(relatedEvent.release())); > On 2014/06/06 22:30:42, esprehn wrote: > > Are you sure it's safe to run script here? Who owns the ContextMenuController? > > The script here can navigate the page and destroy the current document. > > Done. The ContextMenuController is owned by Page. If the script navigate the > page and destroy the current document, then ContextMenuController does not > change. Do we still need async event here because we need to update the already > shown context menu in case of async event which makes the existing > implementation complex. To be clear, I am trying out this test page http://jsbin.com/qujelihu/1. Do you mean the same? The ContextMenuController does not change if we replace the document using window.location.replace() but it changes while navigating to a page using window.location.href. So, in the above example the custom contextmenu is stale but does not crash, this behavior is similar in case of default context menu. If you still feel that this risky, I shall make it an async event.
This is looking better, but you're missing a lot of stuff from the spec. Also you need a RuntimeEnabledFeature flag to hide your feature behind. Lets add that and land this and then you can work on finishing compliance with the spec. https://codereview.chromium.org/243403006/diff/220001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/220001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:22: if (m_menu) { Early return instead. if (!m_menu) return; if (...) ...->click(); Also does the spec really say to use the simulated click API like this? https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:67: } else if (nextElement->hasTagName(HTMLNames::menuitemTag)) { This doesn't seem to implement the <hr>'s as separators feature of the spec. https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuitemElement.cpp (right): https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuitemElement.cpp:14: inline HTMLMenuitemElement::HTMLMenuitemElement(Document& document) Incorrect capitalization per the spec. https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuitemElement.idl (right): https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuitemElement.idl:5: interface HTMLMenuitemElement : HTMLElement { This is not spelled correctly, the spec says the I is capitalized: HTMLMenuItemElement
Updated the patch as per comments. Please check. https://codereview.chromium.org/243403006/diff/220001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/220001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:22: if (m_menu) { On 2014/06/20 08:39:57, esprehn wrote: > Early return instead. > > if (!m_menu) > return; > if (...) > ...->click(); > > Also does the spec really say to use the simulated click API like this? The spec says " If the command's Action is defined as firing a click event, either directly or via the run synthetic click activation steps algorithm, then the relatedTarget attribute of that click event must be initialised to the subject passed to this construct and show a menu algorithm." Done. https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:67: } else if (nextElement->hasTagName(HTMLNames::menuitemTag)) { On 2014/06/20 08:39:57, esprehn wrote: > This doesn't seem to implement the <hr>'s as separators feature of the spec. Yes, I have skipped the whole separators thing while building the menu. I would like to do it a separate CL if that is ok? https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuitemElement.cpp (right): https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuitemElement.cpp:14: inline HTMLMenuitemElement::HTMLMenuitemElement(Document& document) On 2014/06/20 08:39:57, esprehn wrote: > Incorrect capitalization per the spec. Done. https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuitemElement.idl (right): https://codereview.chromium.org/243403006/diff/220001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuitemElement.idl:5: interface HTMLMenuitemElement : HTMLElement { On 2014/06/20 08:39:57, esprehn wrote: > This is not spelled correctly, the spec says the I is capitalized: > HTMLMenuItemElement Done.
Hi Esprehn, Please take another look. It has only one minor change from last patch.
https://codereview.chromium.org/243403006/diff/280001/Source/core/html/HTMLTa... File Source/core/html/HTMLTagNames.in (right): https://codereview.chromium.org/243403006/diff/280001/Source/core/html/HTMLTa... Source/core/html/HTMLTagNames.in:85: menuitem interfaceName=HTMLMenuItemElement This needs runtimeEnabled otherwise the parser will still create them and your code will still dispatch events.
https://codereview.chromium.org/243403006/diff/280001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/280001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:90: if (node && node->isHTMLElement()) { This needs to be guarded by the RuntimeEnabledFeature flag. Your code will still dispatch events below.
Fixed the comments. Please review. https://codereview.chromium.org/243403006/diff/280001/Source/core/html/HTMLTa... File Source/core/html/HTMLTagNames.in (right): https://codereview.chromium.org/243403006/diff/280001/Source/core/html/HTMLTa... Source/core/html/HTMLTagNames.in:85: menuitem interfaceName=HTMLMenuItemElement On 2014/07/14 08:56:48, esprehn wrote: > This needs runtimeEnabled otherwise the parser will still create them and your > code will still dispatch events. Done. https://codereview.chromium.org/243403006/diff/280001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/280001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:90: if (node && node->isHTMLElement()) { On 2014/07/14 08:57:51, esprehn wrote: > This needs to be guarded by the RuntimeEnabledFeature flag. Your code will still > dispatch events below. Done.
Rebased and fixed the review comments. Please review.
The CQ bit was checked by sanjoy.pal@samsung.com
The CQ bit was unchecked by sanjoy.pal@samsung.com
All the builders went red?
On 2014/07/16 19:37:59, esprehn wrote: > All the builders went red? webexposed/global-constructors-listing.html needs rebaselining.
A few drive-by comments. https://codereview.chromium.org/243403006/diff/300001/Source/core/events/Rela... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:27: virtual ~RelatedEvent() { } nit: should probably be in the cpp. https://codereview.chromium.org/243403006/diff/300001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:30: EventTarget* relatedTarget(bool& isNull) const { isNull = !m_relatedTarget; return m_relatedTarget.get(); } nit: This isNull argument doesn't make much sense for pointers. We should probably remove the '?' on IDL side. https://codereview.chromium.org/243403006/diff/300001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:39: RelatedEvent(const AtomicString& type); nit: explicit https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:6: nit: blank line is not needed here. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:24: explicit CustomContextMenuProvider(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items) nit: explicit is not really useful here as this takes 2 mandatory arguments. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:30: virtual ~CustomContextMenuProvider() nit: could be in the cpp https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:48: populateContextMenuItems(this, menu); *this https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:54: void HTMLMenuElement::populateContextMenuItems(HTMLMenuElement* menu, ContextMenu& contextMenu) HTMLMenuElement& https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:56: Element* nextElement = ElementTraversal::firstWithin(*menu); HTMLElement* currentElement = Traversal<HTMLElement>::firstWithin(menu); since you are only interested in HTMLElements (avoids casting below). https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:58: if (nextElement->hasTagName(HTMLNames::menuTag)) { isHTMLMenuElement(*nextElement) https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:62: nextElement = ElementTraversal::nextSibling(*nextElement); Traversal<HTMLElement> https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:63: } else if (nextElement->hasTagName(HTMLNames::menuitemTag)) { isHTMLMenuItemElement(*nextElement) https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:68: nextElement = ElementTraversal::next(*nextElement, menu); Traversal<HTMLElement> https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:76: m_subject = toHTMLElement(toRelatedEvent(event)->relatedTarget()->toNode()); Won't this crash if relatedTarget is an EventTarget other than an HTMLElement? https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:35: class CustomContextMenuProvider; You already include this. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:47: HTMLElement * subjectElement() { return m_subject.get(); } nit: space before star https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:51: RefPtr<CustomContextMenuProvider> m_menuProvider; nit: I would add a blink line to separate the class members from the methods. https://codereview.chromium.org/243403006/diff/300001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:96: const HTMLElement& element = toHTMLElement(*event->target()->toNode()); toHTMLElement(*node)
Fixed the review comments. Please review. https://codereview.chromium.org/243403006/diff/300001/Source/core/events/Rela... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:27: virtual ~RelatedEvent() { } On 2014/07/16 20:07:25, Chris Dumez wrote: > nit: should probably be in the cpp. Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:30: EventTarget* relatedTarget(bool& isNull) const { isNull = !m_relatedTarget; return m_relatedTarget.get(); } On 2014/07/16 20:07:25, Chris Dumez wrote: > nit: This isNull argument doesn't make much sense for pointers. We should > probably remove the '?' on IDL side. Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:39: RelatedEvent(const AtomicString& type); On 2014/07/16 20:07:25, Chris Dumez wrote: > nit: explicit Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:6: On 2014/07/16 20:07:25, Chris Dumez wrote: > nit: blank line is not needed here. Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:24: explicit CustomContextMenuProvider(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items) On 2014/07/16 20:07:26, Chris Dumez wrote: > nit: explicit is not really useful here as this takes 2 mandatory arguments. Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:30: virtual ~CustomContextMenuProvider() On 2014/07/16 20:07:25, Chris Dumez wrote: > nit: could be in the cpp Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:48: populateContextMenuItems(this, menu); On 2014/07/16 20:07:26, Chris Dumez wrote: > *this Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:54: void HTMLMenuElement::populateContextMenuItems(HTMLMenuElement* menu, ContextMenu& contextMenu) On 2014/07/16 20:07:26, Chris Dumez wrote: > HTMLMenuElement& Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:56: Element* nextElement = ElementTraversal::firstWithin(*menu); On 2014/07/16 20:07:26, Chris Dumez wrote: > HTMLElement* currentElement = Traversal<HTMLElement>::firstWithin(menu); > > since you are only interested in HTMLElements (avoids casting below). Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:58: if (nextElement->hasTagName(HTMLNames::menuTag)) { On 2014/07/16 20:07:26, Chris Dumez wrote: > isHTMLMenuElement(*nextElement) Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:62: nextElement = ElementTraversal::nextSibling(*nextElement); On 2014/07/16 20:07:26, Chris Dumez wrote: > Traversal<HTMLElement> Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:63: } else if (nextElement->hasTagName(HTMLNames::menuitemTag)) { On 2014/07/16 20:07:26, Chris Dumez wrote: > isHTMLMenuItemElement(*nextElement) Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:68: nextElement = ElementTraversal::next(*nextElement, menu); On 2014/07/16 20:07:26, Chris Dumez wrote: > Traversal<HTMLElement> Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:76: m_subject = toHTMLElement(toRelatedEvent(event)->relatedTarget()->toNode()); On 2014/07/16 20:07:26, Chris Dumez wrote: > Won't this crash if relatedTarget is an EventTarget other than an HTMLElement? It should not be anything otherthan HTMLElement as we set it to HTMLElement only in ContextMenuController.cpp. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:35: class CustomContextMenuProvider; On 2014/07/16 20:07:26, Chris Dumez wrote: > You already include this. Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:47: HTMLElement * subjectElement() { return m_subject.get(); } On 2014/07/16 20:07:26, Chris Dumez wrote: > nit: space before star Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:51: RefPtr<CustomContextMenuProvider> m_menuProvider; On 2014/07/16 20:07:26, Chris Dumez wrote: > nit: I would add a blink line to separate the class members from the methods. Done. https://codereview.chromium.org/243403006/diff/300001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/300001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:96: const HTMLElement& element = toHTMLElement(*event->target()->toNode()); On 2014/07/16 20:07:26, Chris Dumez wrote: > toHTMLElement(*node) Done.
Esprehn, Chris: Please take another look.
Your bubbles are still all red.
Now all are green.
+tkent (instead of Ojan who is doing code yellow) as reviewer.
https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... File Source/core/events/RelatedEvent.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.cpp:8: namespace WebCore { s/WebCore/blink https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.cpp:18: PassRefPtr<RelatedEvent> RelatedEvent::create(const AtomicString& type) Use oilpan types in the cpp as well https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.cpp:45: } // namespace WebCore blink https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:10: namespace WebCore { s/WebCore/blink https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:14: RefPtr<EventTarget> relatedTarget; oilpan: RefPtrWillBeMember https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:19: static PassRefPtr<RelatedEvent> create() Oilpan: PassRefPtrWillBeRawPtr https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:24: static PassRefPtr<RelatedEvent> create(const AtomicString& eventType); ditto https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:25: static PassRefPtr<RelatedEvent> create(const AtomicString& eventType, const RelatedEventInit&); ditto https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:30: void setRelatedTarget(PassRefPtr<EventTarget> relatedTarget) { m_relatedTarget = relatedTarget; } oilpan: PassRefPtrWillBeRawPtr<EventTarget> https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:42: RefPtr<EventTarget> m_relatedTarget; oilpan: RefPtrWillBeMember<EventTarget> https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:47: } // namespace WebCore blink https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:14: namespace WebCore { blink https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:28: if (!m_menu) Just to confirm, can this happen or not? If it shouldn't happen, then this should be an assertion. Otherwise, it is fine to have an if check. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:31: if (HTMLElement* element = m_menu->menuItemAt(item->action())) { HTMLMenuItemElement* https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:38: } // namespace blink https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:11: namespace WebCore { blink https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:18: static PassRefPtr<CustomContextMenuProvider> create(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items) PassRefPtrWillBeRawPtr https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:24: CustomContextMenuProvider(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items) Why isn't this defined in the cpp? https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:38: RefPtr<HTMLMenuElement> m_menu; RefPtrWillBeMember https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:42: } // namespace blink https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:61: contextMenu.appendItem(ContextMenuItem(SubmenuType, ContextMenuItemCustomTagNoAction, nextElement->fastGetAttribute(HTMLNames::labelAttr), &subMenu)); HTMLNames:: are not needed in this file https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:64: m_menuItems.append(nextElement); Looks like m_menuItems can only HTMLMenuItemElements? If so, please use tighter typing. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:76: m_subject = toHTMLElement(toRelatedEvent(event)->relatedTarget()->toNode()); Is this cast safe? https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:82: HTMLElement* HTMLMenuElement::menuItemAt(unsigned menuId) Could return a HTMLMenuItemElement* https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:86: int itemNumber = menuId - ContextMenuItemBaseCustomTag; itemIndex ? Also, can we reverse this? 1. Compute the index 2. Do the bounds check if (itemIndex < 0 || itemIndex >= m_menuItems.size()) return 0; https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:51: RefPtr<CustomContextMenuProvider> m_menuProvider; RefPtrWillBeMember https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:52: RefPtr<HTMLElement> m_subject; RefPtrWillBeMember https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:53: Vector<HTMLElement*> m_menuItems; Vector<HTMLMenuItemElement*> ? https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.cpp:20: PassRefPtr<HTMLMenuItemElement> HTMLMenuItemElement::create(Document& document) PassRefPtrWillBeRawPtr https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.h (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.h:10: namespace WebCore { blink https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.h:14: static PassRefPtr<HTMLMenuItemElement> create(Document&); PassRefPtrWillBeRawPtr https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.h:20: } // namespace blink https://codereview.chromium.org/243403006/diff/380001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:97: if (RefPtr<HTMLMenuElement> menuElement = element.menuElement()) { oilpan types https://codereview.chromium.org/243403006/diff/380001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:101: m_menuProvider = menuElement->menuProvider(); I did not look much into it but is it normal that the menu provider is initialized *after* the event is fired?
Moved to oilpan types. PTAL. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... File Source/core/events/RelatedEvent.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.cpp:8: namespace WebCore { On 2014/07/23 14:19:07, Chris Dumez wrote: > s/WebCore/blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.cpp:18: PassRefPtr<RelatedEvent> RelatedEvent::create(const AtomicString& type) On 2014/07/23 14:19:07, Chris Dumez wrote: > Use oilpan types in the cpp as well Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.cpp:45: } // namespace WebCore On 2014/07/23 14:19:07, Chris Dumez wrote: > blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:10: namespace WebCore { On 2014/07/23 14:19:07, Chris Dumez wrote: > s/WebCore/blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:14: RefPtr<EventTarget> relatedTarget; On 2014/07/23 14:19:07, Chris Dumez wrote: > oilpan: RefPtrWillBeMember Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:19: static PassRefPtr<RelatedEvent> create() On 2014/07/23 14:19:07, Chris Dumez wrote: > Oilpan: PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:24: static PassRefPtr<RelatedEvent> create(const AtomicString& eventType); On 2014/07/23 14:19:07, Chris Dumez wrote: > ditto Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:25: static PassRefPtr<RelatedEvent> create(const AtomicString& eventType, const RelatedEventInit&); On 2014/07/23 14:19:07, Chris Dumez wrote: > ditto Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:30: void setRelatedTarget(PassRefPtr<EventTarget> relatedTarget) { m_relatedTarget = relatedTarget; } On 2014/07/23 14:19:07, Chris Dumez wrote: > oilpan: PassRefPtrWillBeRawPtr<EventTarget> Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:42: RefPtr<EventTarget> m_relatedTarget; On 2014/07/23 14:19:07, Chris Dumez wrote: > oilpan: RefPtrWillBeMember<EventTarget> Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:47: } // namespace WebCore On 2014/07/23 14:19:07, Chris Dumez wrote: > blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:14: namespace WebCore { On 2014/07/23 14:19:07, Chris Dumez wrote: > blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:28: if (!m_menu) On 2014/07/23 14:19:07, Chris Dumez wrote: > Just to confirm, can this happen or not? If it shouldn't happen, then this > should be an assertion. Otherwise, it is fine to have an if check. This can happen if user has been shown context menu and js has removed the menu from dom before selecting any item. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:31: if (HTMLElement* element = m_menu->menuItemAt(item->action())) { On 2014/07/23 14:19:07, Chris Dumez wrote: > HTMLMenuItemElement* Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:38: } // namespace On 2014/07/23 14:19:07, Chris Dumez wrote: > blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:11: namespace WebCore { On 2014/07/23 14:19:08, Chris Dumez wrote: > blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:18: static PassRefPtr<CustomContextMenuProvider> create(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items) On 2014/07/23 14:19:08, Chris Dumez wrote: > PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:24: CustomContextMenuProvider(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items) On 2014/07/23 14:19:07, Chris Dumez wrote: > Why isn't this defined in the cpp? Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:38: RefPtr<HTMLMenuElement> m_menu; On 2014/07/23 14:19:08, Chris Dumez wrote: > RefPtrWillBeMember Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:42: } // namespace On 2014/07/23 14:19:08, Chris Dumez wrote: > blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:61: contextMenu.appendItem(ContextMenuItem(SubmenuType, ContextMenuItemCustomTagNoAction, nextElement->fastGetAttribute(HTMLNames::labelAttr), &subMenu)); On 2014/07/23 14:19:08, Chris Dumez wrote: > HTMLNames:: are not needed in this file Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:64: m_menuItems.append(nextElement); On 2014/07/23 14:19:08, Chris Dumez wrote: > Looks like m_menuItems can only HTMLMenuItemElements? If so, please use tighter > typing. hr element can also be menuItems. hr elements can be used as separators between menu items. But, in that case we don't need to store hr element in the list. I shall change Vector<HTMLElement*> to Vector<HTMLMenuItemElement*> https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:76: m_subject = toHTMLElement(toRelatedEvent(event)->relatedTarget()->toNode()); On 2014/07/23 14:19:08, Chris Dumez wrote: > Is this cast safe? Yes. we are checking for isRelatedEvent() here and isHTMLElemnt() before setting relatedTarget in ContextMenuController.cpp. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:82: HTMLElement* HTMLMenuElement::menuItemAt(unsigned menuId) On 2014/07/23 14:19:08, Chris Dumez wrote: > Could return a HTMLMenuItemElement* Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:86: int itemNumber = menuId - ContextMenuItemBaseCustomTag; On 2014/07/23 14:19:08, Chris Dumez wrote: > itemIndex ? > Also, can we reverse this? > 1. Compute the index > 2. Do the bounds check if (itemIndex < 0 || itemIndex >= m_menuItems.size()) > return 0; Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:51: RefPtr<CustomContextMenuProvider> m_menuProvider; On 2014/07/23 14:19:08, Chris Dumez wrote: > RefPtrWillBeMember Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:52: RefPtr<HTMLElement> m_subject; On 2014/07/23 14:19:08, Chris Dumez wrote: > RefPtrWillBeMember Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:53: Vector<HTMLElement*> m_menuItems; On 2014/07/23 14:19:08, Chris Dumez wrote: > Vector<HTMLMenuItemElement*> ? Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.cpp:20: PassRefPtr<HTMLMenuItemElement> HTMLMenuItemElement::create(Document& document) On 2014/07/23 14:19:08, Chris Dumez wrote: > PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.h (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.h:10: namespace WebCore { On 2014/07/23 14:19:08, Chris Dumez wrote: > blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.h:14: static PassRefPtr<HTMLMenuItemElement> create(Document&); On 2014/07/23 14:19:08, Chris Dumez wrote: > PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.h:20: } // namespace On 2014/07/23 14:19:08, Chris Dumez wrote: > blink Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/380001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:97: if (RefPtr<HTMLMenuElement> menuElement = element.menuElement()) { On 2014/07/23 14:19:08, Chris Dumez wrote: > oilpan types Done. https://codereview.chromium.org/243403006/diff/380001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:101: m_menuProvider = menuElement->menuProvider(); On 2014/07/23 14:19:08, Chris Dumez wrote: > I did not look much into it but is it normal that the menu provider is > initialized *after* the event is fired? Yes. We are initializing the menu provider in menu element upon receiving the show event synchronously.
What's your plan about |icon| and |command| IDL attributes of HMTLMenuItem element?
On 2014/07/25 00:58:29, tkent wrote: > What's your plan about |icon| and |command| IDL attributes of HMTLMenuItem > element? and |radiogroup|
On 2014/07/25 01:03:47, tkent wrote: > On 2014/07/25 00:58:29, tkent wrote: > > What's your plan about |icon| and |command| IDL attributes of HMTLMenuItem > > element? > > and |radiogroup| I'm planning to implement the rest of specification as multiple follow up patches. Esprehn also suggested to land this cl and implement the rest as smaller and separate CLs as its being difficult to review.
On 2014/07/25 01:51:24, sanjoy.pal wrote: > On 2014/07/25 01:03:47, tkent wrote: > > On 2014/07/25 00:58:29, tkent wrote: > > > What's your plan about |icon| and |command| IDL attributes of HMTLMenuItem > > > element? > > > > and |radiogroup| > > I'm planning to implement the rest of specification as multiple follow up > patches. Esprehn also suggested to land this cl and implement the rest as > smaller and separate CLs as its being difficult to review. Yeah, It's a good plan. Please mention it in the CL description. Do you think |icon| is implementable in all platforms?
On 2014/07/25 02:16:38, tkent wrote: > On 2014/07/25 01:51:24, sanjoy.pal wrote: > > On 2014/07/25 01:03:47, tkent wrote: > > > On 2014/07/25 00:58:29, tkent wrote: > > > > What's your plan about |icon| and |command| IDL attributes of HMTLMenuItem > > > > element? > > > > > > and |radiogroup| > > > > I'm planning to implement the rest of specification as multiple follow up > > patches. Esprehn also suggested to land this cl and implement the rest as > > smaller and separate CLs as its being difficult to review. > > Yeah, It's a good plan. Please mention it in the CL description. > Do you think |icon| is implementable in all platforms? Updated the description and showing icons in context menu are supported on all platforms from platform side.
> Updated the description and showing icons in context menu are supported on all > platforms from platform side. ok, I believe you about |icon| implementability. I'll do code review.
https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:20: { The function implementation should be moved to RelatedEvent.cpp for consistency with other |create| functions. https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:21: return adoptRef(new RelatedEvent); adoptRef -> adoptRefWillBeNoop https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:36: protected: This should be private. https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:42: RefPtrWillBeMember<EventTarget> m_relatedTarget; Because this class has Member<>, we need to have trace() function. Please refer to FocusEvent. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:35: if (!m_menu) This check is unnecessary. m_menu is always non-null. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:39: RefPtrWillBeRawPtr<SimulatedMouseEvent> click = SimulatedMouseEvent::create(EventTypeNames::click, m_menu->document().domWindow(), Event::create()); Why do you dispatch 'click' event even though the selection was done by keyboard or something? Do we need to pass Event::create()? Is passing null acceptable? https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:41: EventDispatcher::dispatchEvent(element, EventDispatchMediator::create(click.release())); Why don't you use element->dispatchEvent(click.release())? https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:19: static PassRefPtrWillBeRawPtr<CustomContextMenuProvider> create(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items) PassRefPtrWIllBeRawPtr -> PassRefPtr because ContextMenuProvider is not garbage-collected. The first argument should be HTMLMenuElement&. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:21: return adoptRefWillBeNoop(new CustomContextMenuProvider(menu, items)); adoptRefWIllBeNoop -> adoptRef https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:25: CustomContextMenuProvider(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items); The first argument should be HTMLMenuElement&. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:26: nit: Need no blank line https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:30: nit: Need no blank line https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:32: nit: Need no blank line https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:35: RefPtrWillBeMember<HTMLMenuElement> m_menu; RefPtrWillBeMember -> RefPtrWillBePersistent https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.h:99: HTMLMenuElement* menuElement() const; HTMLMenuElement dependency from HTMLElement is weird. ContextMenuProvider should have this. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:69: } if nextElement is neither <menu> nor <menuitem>, this loop won't stop. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:34: class HTMLMenuElement; This is unnecessary. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:51: RefPtrWillBeMember<CustomContextMenuProvider> m_menuProvider; HTMLMenuElement should not keep CustomContextMenuProvider. See the comment on ContextMenuController.cpp. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:52: RefPtrWillBeMember<HTMLElement> m_subject; This member is not referred. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:53: Vector<HTMLMenuItemElement*> m_menuItems; should be WillBeHeapVector<RawPtrWillBeMember<HTMLMenuItemElement> >, and we need trace() override. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.cpp:20: PassRefPtrWillBeRawPtr<HTMLMenuItemElement> HTMLMenuItemElement::create(Document& document) Use DEFINE_NODE_FACTORY. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.h:14: static PassRefPtrWillBeRawPtr<HTMLMenuItemElement> create(Document&); Use DECLARE_NODE_FACTORY. https://codereview.chromium.org/243403006/diff/440001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:95: EventDispatcher::dispatchEvent(menuElement.get(), EventDispatchMediator::create(relatedEvent.release())); Why don't you use menuElement->dispatchEvent(relatedEvent.release())? Also, this even this event is cancelable, and we need to check the return value. https://codereview.chromium.org/243403006/diff/440001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:96: m_menuProvider = menuElement->menuProvider(); We should construct menu items here, not in HTMLMenuElement::dispatchEvent. The current code is complex unnecessarily. https://codereview.chromium.org/243403006/diff/440001/Source/platform/Runtime... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/243403006/diff/440001/Source/platform/Runtime... Source/platform/RuntimeEnabledFeatures.in:89: MenuElement status=experimental The name |MenuElement| is not good because we already support <menu>. ContextMenu? https://codereview.chromium.org/243403006/diff/440001/Source/web/tests/Contex... File Source/web/tests/ContextMenuTest.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/web/tests/Contex... Source/web/tests/ContextMenuTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. We should remove web/ dependency from the test, and should move this to core/.
https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.h:99: HTMLMenuElement* menuElement() const; On 2014/07/25 04:42:38, tkent wrote: > HTMLMenuElement dependency from HTMLElement is weird. > ContextMenuProvider should have this. Please ignore this comment. Instead, this function should be renamed to |contextMenu|, and exposed via IDL.
Fixed the review comments as suggested. PTAL. https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... File Source/core/events/RelatedEvent.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:20: { On 2014/07/25 04:42:37, tkent wrote: > The function implementation should be moved to RelatedEvent.cpp for consistency > with other |create| functions. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:21: return adoptRef(new RelatedEvent); On 2014/07/25 04:42:37, tkent wrote: > adoptRef -> adoptRefWillBeNoop Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:36: protected: On 2014/07/25 04:42:37, tkent wrote: > This should be private. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/events/Rela... Source/core/events/RelatedEvent.h:42: RefPtrWillBeMember<EventTarget> m_relatedTarget; On 2014/07/25 04:42:37, tkent wrote: > Because this class has Member<>, we need to have trace() function. Please refer > to FocusEvent. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:35: if (!m_menu) On 2014/07/25 04:42:37, tkent wrote: > This check is unnecessary. m_menu is always non-null. This can happen if user has been shown context menu and js has removed the menu from dom before selecting any item. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:39: RefPtrWillBeRawPtr<SimulatedMouseEvent> click = SimulatedMouseEvent::create(EventTypeNames::click, m_menu->document().domWindow(), Event::create()); On 2014/07/25 04:42:37, tkent wrote: > Why do you dispatch 'click' event even though the selection was done by keyboard > or something? Selection is done on menu shown by platform context menu, so we need to forward this event by dispatching 'click'. > > Do we need to pass Event::create()? Is passing null acceptable? Yes, we need to pass valid Event*. > https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:41: EventDispatcher::dispatchEvent(element, EventDispatchMediator::create(click.release())); On 2014/07/25 04:42:37, tkent wrote: > Why don't you use element->dispatchEvent(click.release())? Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:19: static PassRefPtrWillBeRawPtr<CustomContextMenuProvider> create(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items) On 2014/07/25 04:42:38, tkent wrote: > PassRefPtrWIllBeRawPtr -> PassRefPtr > because ContextMenuProvider is not garbage-collected. > > The first argument should be HTMLMenuElement&. > > Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:21: return adoptRefWillBeNoop(new CustomContextMenuProvider(menu, items)); On 2014/07/25 04:42:38, tkent wrote: > adoptRefWIllBeNoop -> adoptRef Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:25: CustomContextMenuProvider(HTMLMenuElement* menu, const Vector<ContextMenuItem>& items); On 2014/07/25 04:42:37, tkent wrote: > The first argument should be HTMLMenuElement&. Changed to PassRefPtr. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:26: On 2014/07/25 04:42:37, tkent wrote: > nit: Need no blank line Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:30: On 2014/07/25 04:42:37, tkent wrote: > nit: Need no blank line Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:32: On 2014/07/25 04:42:37, tkent wrote: > nit: Need no blank line Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.h:35: RefPtrWillBeMember<HTMLMenuElement> m_menu; On 2014/07/25 04:42:38, tkent wrote: > RefPtrWillBeMember -> RefPtrWillBePersistent Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.h:99: HTMLMenuElement* menuElement() const; On 2014/07/25 04:46:58, tkent wrote: > On 2014/07/25 04:42:38, tkent wrote: > > HTMLMenuElement dependency from HTMLElement is weird. > > ContextMenuProvider should have this. > > Please ignore this comment. > > Instead, this function should be renamed to |contextMenu|, and exposed via IDL. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:69: } On 2014/07/25 04:42:38, tkent wrote: > if nextElement is neither <menu> nor <menuitem>, this loop won't stop. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:34: class HTMLMenuElement; On 2014/07/25 04:42:38, tkent wrote: > This is unnecessary. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:51: RefPtrWillBeMember<CustomContextMenuProvider> m_menuProvider; On 2014/07/25 04:42:38, tkent wrote: > HTMLMenuElement should not keep CustomContextMenuProvider. See the comment on > ContextMenuController.cpp. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:52: RefPtrWillBeMember<HTMLElement> m_subject; On 2014/07/25 04:42:38, tkent wrote: > This member is not referred. This is referred in ContextMenuController.cpp. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:53: Vector<HTMLMenuItemElement*> m_menuItems; On 2014/07/25 04:42:38, tkent wrote: > should be WillBeHeapVector<RawPtrWillBeMember<HTMLMenuItemElement> >, and we > need trace() override. > Moved this code to ContextMenuController.cpp. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.cpp:20: PassRefPtrWillBeRawPtr<HTMLMenuItemElement> HTMLMenuItemElement::create(Document& document) On 2014/07/25 04:42:38, tkent wrote: > Use DEFINE_NODE_FACTORY. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.h (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.h:14: static PassRefPtrWillBeRawPtr<HTMLMenuItemElement> create(Document&); On 2014/07/25 04:42:38, tkent wrote: > Use DECLARE_NODE_FACTORY. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:95: EventDispatcher::dispatchEvent(menuElement.get(), EventDispatchMediator::create(relatedEvent.release())); On 2014/07/25 04:42:38, tkent wrote: > Why don't you use menuElement->dispatchEvent(relatedEvent.release())? > > Also, this even this event is cancelable, and we need to check the return value. Done. https://codereview.chromium.org/243403006/diff/440001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:96: m_menuProvider = menuElement->menuProvider(); On 2014/07/25 04:42:38, tkent wrote: > We should construct menu items here, not in HTMLMenuElement::dispatchEvent. The > current code is complex unnecessarily. Done. https://codereview.chromium.org/243403006/diff/440001/Source/platform/Runtime... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/243403006/diff/440001/Source/platform/Runtime... Source/platform/RuntimeEnabledFeatures.in:89: MenuElement status=experimental On 2014/07/25 04:42:39, tkent wrote: > The name |MenuElement| is not good because we already support <menu>. > ContextMenu? Done. https://codereview.chromium.org/243403006/diff/440001/Source/web/tests/Contex... File Source/web/tests/ContextMenuTest.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/web/tests/Contex... Source/web/tests/ContextMenuTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/07/25 04:42:39, tkent wrote: > We should remove web/ dependency from the test, and should move this to core/. Done.
It looks you're working with old checkout. Please rebase. https://codereview.chromium.org/243403006/diff/470001/Source/core/events/Rela... File Source/core/events/RelatedEvent.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/events/Rela... Source/core/events/RelatedEvent.idl:6: EventConstructor, Need a layout test for constructor. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:912: setAttribute(contextmenuAttr, contextMenu->elementData()->idForStyleResolution()); Does this behavior conform to the standard? https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.h (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.h:99: PassRefPtrWillBeRawPtr<HTMLMenuElement> contextMenu() const; The return value should be HTMLMenuElement* because this function should not move ownership. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.h:100: void setContextMenu(PassRefPtrWillBeRawPtr<HTMLMenuElement>); The argument type should be HTMLMenuElement*. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.idl:43: [RuntimeEnabled=ContextMenu] attribute HTMLMenuElement? contextMenu; This CL should have a layout test for HTMLElement::contextMenu. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:39: } // namespace blink You don't need to touch this file in this CL. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:38: } // namespace blink You don't need to touch this file in this CL. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.idl:21: [RuntimeEnabled=ContextMenu, Reflect] attribute DOMString type; Add a test to fast/dom/domstring-attribute-reflection.html. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.idl:22: [RuntimeEnabled=ContextMenu, Reflect] attribute DOMString label; Ditto. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.cpp:12: using namespace HTMLNames; This is used only once. Please remove this line, and replace menuitemTag with HTMLNames::menuitemTag. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:8: [Reflect] readonly attribute DOMString type; Add a test to fast/dom/domstring-attribute-reflection.html https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:9: [Reflect] readonly attribute DOMString label; Ditto. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:10: [Reflect] readonly attribute boolean disabled; Add a test to fast/dom/boolean-attribute-reflection.html https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:11: [Reflect] readonly attribute boolean checked; Ditto. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:12: [Reflect] readonly attribute boolean default; Ditto. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:13: }; Add a FIXME comment about unimplemented IDL attributes. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:120: Node* node = event->target()->toNode(); I recommend to make new function for this block of new code. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:126: m_subject = toHTMLElement(node); toHTMLElement is unnecessary we already have |element|. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:127: bool notCanceled = menuElement->dispatchEvent(relatedEvent.release()); |notCanceled| is unnecessary. if (menuElement->dispatchEvet...) { If you'd like to add a variable for code readability, the name should not contain 'not'. It should be |shouldProceed| or something. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:129: ContextMenu menu; Because dispatchEvent runs arbitrary JavaScript code, |menuElement| might be disassociated with the subject element. We need to check HTMLElement::contextMenu() again. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:130: populateContextMenuItems(*menuElement, menu); I'm sorry I changed my mind. populateContextMenuItems() should be a member function of CustomContextMenuProvider. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... File Source/core/page/ContextMenuController.h (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.h:80: RefPtr<HTMLElement> m_subject; Someone needs to clear m_subject and m_menuItems after closing a context menu. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... File Source/core/page/ContextMenuControllerTest.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuControllerTest.cpp:16: using namespace WebCore; This is unnecessary. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuControllerTest.cpp:18: namespace { We should use |namespace blink| instead of anonymous namespace. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Custom... File Source/core/page/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Custom... Source/core/page/CustomContextMenuProvider.h:19: static PassRefPtr<CustomContextMenuProvider> create(PassRefPtr<Document> document, const Vector<ContextMenuItem>& items) PassRefPtr<Document> -> Document& https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Custom... Source/core/page/CustomContextMenuProvider.h:25: CustomContextMenuProvider(PassRefPtr<Document>, const Vector<ContextMenuItem>& items); Ditto.
https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... File Source/core/html/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... Source/core/html/CustomContextMenuProvider.cpp:39: RefPtrWillBeRawPtr<SimulatedMouseEvent> click = SimulatedMouseEvent::create(EventTypeNames::click, m_menu->document().domWindow(), Event::create()); On 2014/07/30 09:47:43, sanjoy.pal wrote: > On 2014/07/25 04:42:37, tkent wrote: > > Why do you dispatch 'click' event even though the selection was done by > keyboard > > or something? > Selection is done on menu shown by platform context menu, so we need to forward > this event by dispatching 'click'. I'm asking why 'click'. We should not dispatch 'click' event for keyboard operation. Does the standard ask to dispatch 'click' event?
On 2014/07/31 06:10:47, tkent wrote: > https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... > File Source/core/html/CustomContextMenuProvider.cpp (right): > > https://codereview.chromium.org/243403006/diff/440001/Source/core/html/Custom... > Source/core/html/CustomContextMenuProvider.cpp:39: > RefPtrWillBeRawPtr<SimulatedMouseEvent> click = > SimulatedMouseEvent::create(EventTypeNames::click, > m_menu->document().domWindow(), Event::create()); > On 2014/07/30 09:47:43, sanjoy.pal wrote: > > On 2014/07/25 04:42:37, tkent wrote: > > > Why do you dispatch 'click' event even though the selection was done by > > keyboard > > > or something? > > Selection is done on menu shown by platform context menu, so we need to > forward > > this event by dispatching 'click'. > > I'm asking why 'click'. We should not dispatch 'click' event for keyboard > operation. Does the standard ask to dispatch 'click' event? The spec says " If the command's Action is defined as firing a click event, either directly or via the run synthetic click activation steps algorithm, then the relatedTarget attribute of that click event must be initialised to the subject passed to this construct and show menu algorithm." There is nothing mentioned specific to keyboard event.
On 2014/07/31 09:29:21, sanjoy.pal wrote: > > I'm asking why 'click'. We should not dispatch 'click' event for keyboard > > operation. Does the standard ask to dispatch 'click' event? > > The spec says " If the command's Action is defined as firing a click event, > either directly or via the run synthetic click activation steps algorithm, then > the relatedTarget attribute of that click event must be initialised to the > subject passed to this construct and show menu algorithm." There is nothing > mentioned > specific to keyboard event. Please file a spec bug to W3C to clarify this. We can proceed the code review by adding a FIXME comment.
On 2014/07/31 12:51:30, tkent wrote: > On 2014/07/31 09:29:21, sanjoy.pal wrote: > > > I'm asking why 'click'. We should not dispatch 'click' event for keyboard > > > operation. Does the standard ask to dispatch 'click' event? > > > > The spec says " If the command's Action is defined as firing a click event, > > either directly or via the run synthetic click activation steps algorithm, > then > > the relatedTarget attribute of that click event must be initialised to the > > subject passed to this construct and show menu algorithm." There is nothing > > mentioned > > specific to keyboard event. > > Please file a spec bug to W3C to clarify this. > We can proceed the code review by adding a FIXME comment. Filed a bug https://www.w3.org/Bugs/Public/show_bug.cgi?id=26481 From the discussion, it seems we should dispatch 'click' event only.
Fixed the comments and added few more test cases. Please review. https://codereview.chromium.org/243403006/diff/470001/Source/core/events/Rela... File Source/core/events/RelatedEvent.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/events/Rela... Source/core/events/RelatedEvent.idl:6: EventConstructor, On 2014/07/31 06:07:55, tkent wrote: > Need a layout test for constructor. Rebased LayoutTests/webexposed/global-constructors-listing.html. Do you mean the same? https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:912: setAttribute(contextmenuAttr, contextMenu->elementData()->idForStyleResolution()); On 2014/07/31 06:07:55, tkent wrote: > Does this behavior conform to the standard? Specification says that the contextmenu attribute value must be the ID of a menu element in the same home subtree. So, added the treescope check. Do you mean the same? https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.h (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.h:99: PassRefPtrWillBeRawPtr<HTMLMenuElement> contextMenu() const; On 2014/07/31 06:07:55, tkent wrote: > The return value should be HTMLMenuElement* because this function should not > move ownership. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.h:100: void setContextMenu(PassRefPtrWillBeRawPtr<HTMLMenuElement>); On 2014/07/31 06:07:55, tkent wrote: > The argument type should be HTMLMenuElement*. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.idl:43: [RuntimeEnabled=ContextMenu] attribute HTMLMenuElement? contextMenu; On 2014/07/31 06:07:55, tkent wrote: > This CL should have a layout test for HTMLElement::contextMenu. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.cpp:39: } // namespace blink On 2014/07/31 06:07:55, tkent wrote: > You don't need to touch this file in this CL. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.h (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.h:38: } // namespace blink On 2014/07/31 06:07:55, tkent wrote: > You don't need to touch this file in this CL. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuElement.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.idl:21: [RuntimeEnabled=ContextMenu, Reflect] attribute DOMString type; On 2014/07/31 06:07:55, tkent wrote: > Add a test to fast/dom/domstring-attribute-reflection.html. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuElement.idl:22: [RuntimeEnabled=ContextMenu, Reflect] attribute DOMString label; On 2014/07/31 06:07:55, tkent wrote: > Ditto. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.cpp:12: using namespace HTMLNames; On 2014/07/31 06:07:55, tkent wrote: > This is used only once. > Please remove this line, and replace menuitemTag with HTMLNames::menuitemTag. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... File Source/core/html/HTMLMenuItemElement.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:8: [Reflect] readonly attribute DOMString type; On 2014/07/31 06:07:55, tkent wrote: > Add a test to fast/dom/domstring-attribute-reflection.html Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:9: [Reflect] readonly attribute DOMString label; On 2014/07/31 06:07:55, tkent wrote: > Ditto. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:10: [Reflect] readonly attribute boolean disabled; On 2014/07/31 06:07:55, tkent wrote: > Add a test to fast/dom/boolean-attribute-reflection.html Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:11: [Reflect] readonly attribute boolean checked; On 2014/07/31 06:07:56, tkent wrote: > Ditto. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:12: [Reflect] readonly attribute boolean default; On 2014/07/31 06:07:56, tkent wrote: > Ditto. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLMe... Source/core/html/HTMLMenuItemElement.idl:13: }; On 2014/07/31 06:07:56, tkent wrote: > Add a FIXME comment about unimplemented IDL attributes. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:120: Node* node = event->target()->toNode(); On 2014/07/31 06:07:56, tkent wrote: > I recommend to make new function for this block of new code. > Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:126: m_subject = toHTMLElement(node); On 2014/07/31 06:07:56, tkent wrote: > toHTMLElement is unnecessary we already have |element|. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:127: bool notCanceled = menuElement->dispatchEvent(relatedEvent.release()); On 2014/07/31 06:07:56, tkent wrote: > |notCanceled| is unnecessary. > if (menuElement->dispatchEvet...) { > > If you'd like to add a variable for code readability, the name should not > contain 'not'. It should be |shouldProceed| or something. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:129: ContextMenu menu; On 2014/07/31 06:07:56, tkent wrote: > Because dispatchEvent runs arbitrary JavaScript code, |menuElement| might be > disassociated with the subject element. We need to check > HTMLElement::contextMenu() again. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:130: populateContextMenuItems(*menuElement, menu); On 2014/07/31 06:07:56, tkent wrote: > I'm sorry I changed my mind. populateContextMenuItems() should be a member > function of CustomContextMenuProvider. Done. Moved to CustomContextMenuProvider. After this move the code looks more logically organised. Thanks. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... File Source/core/page/ContextMenuController.h (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuController.h:80: RefPtr<HTMLElement> m_subject; On 2014/07/31 06:07:56, tkent wrote: > Someone needs to clear m_subject and m_menuItems after closing a context menu. Done. Clearing at CustomContextMenuProvider::contextMenuCleared(). https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... File Source/core/page/ContextMenuControllerTest.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuControllerTest.cpp:16: using namespace WebCore; On 2014/07/31 06:07:56, tkent wrote: > This is unnecessary. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Contex... Source/core/page/ContextMenuControllerTest.cpp:18: namespace { On 2014/07/31 06:07:56, tkent wrote: > We should use |namespace blink| instead of anonymous namespace. Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Custom... File Source/core/page/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Custom... Source/core/page/CustomContextMenuProvider.h:19: static PassRefPtr<CustomContextMenuProvider> create(PassRefPtr<Document> document, const Vector<ContextMenuItem>& items) On 2014/07/31 06:07:56, tkent wrote: > PassRefPtr<Document> -> Document& Done. https://codereview.chromium.org/243403006/diff/470001/Source/core/page/Custom... Source/core/page/CustomContextMenuProvider.h:25: CustomContextMenuProvider(PassRefPtr<Document>, const Vector<ContextMenuItem>& items); On 2014/07/31 06:07:56, tkent wrote: > Ditto. Done.
https://codereview.chromium.org/243403006/diff/470001/Source/core/events/Rela... File Source/core/events/RelatedEvent.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/events/Rela... Source/core/events/RelatedEvent.idl:6: EventConstructor, On 2014/08/01 10:03:07, sanjoy.pal wrote: > On 2014/07/31 06:07:55, tkent wrote: > > Need a layout test for constructor. > Rebased LayoutTests/webexposed/global-constructors-listing.html. > Do you mean the same? No. global-constructors-list.html just lists constructors. We need a test of RelatedEvent constructor behavior. See LayoutTests/fast/event/constructors/. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:912: setAttribute(contextmenuAttr, contextMenu->elementData()->idForStyleResolution()); On 2014/08/01 10:03:07, sanjoy.pal wrote: > On 2014/07/31 06:07:55, tkent wrote: > > Does this behavior conform to the standard? > Specification says that the contextmenu attribute value must be the ID of a menu > element in the same home subtree. So, added the treescope check. > Do you mean the same? The HTML specification must have a detailed algorithm to reflect HTMLElement object to ID content attribute. Does your code conform to the algorithm? I don't think your code is correct. e.g. HTMLElement without ID, multiple HTMLElement with simple ID.
I am not sure if it still conforms to spec. Can you please point me to the specification link which you are referring to? or some existing blink code which implements similar spec? or some test code which should fails with the current code? https://codereview.chromium.org/243403006/diff/470001/Source/core/events/Rela... File Source/core/events/RelatedEvent.idl (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/events/Rela... Source/core/events/RelatedEvent.idl:6: EventConstructor, On 2014/08/04 02:26:12, tkent wrote: > On 2014/08/01 10:03:07, sanjoy.pal wrote: > > On 2014/07/31 06:07:55, tkent wrote: > > > Need a layout test for constructor. > > Rebased LayoutTests/webexposed/global-constructors-listing.html. > > Do you mean the same? > > No. global-constructors-list.html just lists constructors. We need a test of > RelatedEvent constructor behavior. See LayoutTests/fast/event/constructors/. Added tests. https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/243403006/diff/470001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:912: setAttribute(contextmenuAttr, contextMenu->elementData()->idForStyleResolution()); On 2014/08/04 02:26:12, tkent wrote: > On 2014/08/01 10:03:07, sanjoy.pal wrote: > > On 2014/07/31 06:07:55, tkent wrote: > > > Does this behavior conform to the standard? > > Specification says that the contextmenu attribute value must be the ID of a > menu > > element in the same home subtree. So, added the treescope check. > > Do you mean the same? > > The HTML specification must have a detailed algorithm to reflect HTMLElement > object to ID content attribute. Does your code conform to the algorithm? > > I don't think your code is correct. e.g. HTMLElement without ID, multiple > HTMLElement with simple ID. Now I am checking if id is null and I have added tests with HTMLMenuElement without ID, multiple HTMLMenuElement with same ID. I am not sure if it still conforms to spec. Can you please point me to the specification link which you are referring to? or some existing blink code which implements similar spec? or some test code which should fails with the current code?
On 2014/08/04 08:58:27, sanjoy.pal wrote: > I am not sure if it still conforms to spec. Can you please point me to the > specification link which you are referring to? or some existing blink code which > implements similar spec? or some test code which should fails with the current > code? You're responsible to find the specification. Not me. AFAIK, we have never had HTMLElement-to-id reflection.
On 2014/08/04 09:02:06, tkent wrote: > On 2014/08/04 08:58:27, sanjoy.pal wrote: > > I am not sure if it still conforms to spec. Can you please point me to the > > specification link which you are referring to? or some existing blink code > which > > implements similar spec? or some test code which should fails with the current > > code? > > You're responsible to find the specification. Not me. > > AFAIK, we have never had HTMLElement-to-id reflection. Ok. I'll try to find out the specification. I have added test for HTMLMenuElement without ID and multiple HTMLMenuElement with same ID, which seems working as expected. Can you please tell me some test which would fail with the current implementation?
Before this CL, I recommend to update MakeMenuItemStringsFor() in event_sender.cc so that it appends actual menu items. It makes tests easier. https://codereview.chromium.org/243403006/diff/610001/LayoutTests/fast/dom/HT... File LayoutTests/fast/dom/HTMLElement/script-tests/contextmenu.js (right): https://codereview.chromium.org/243403006/diff/610001/LayoutTests/fast/dom/HT... LayoutTests/fast/dom/HTMLElement/script-tests/contextmenu.js:1: description('Tests the contextmenu attribute.'); Please do not make new script-test. This file should be merged into contextmenu.html. https://codereview.chromium.org/243403006/diff/610001/LayoutTests/fast/dom/HT... LayoutTests/fast/dom/HTMLElement/script-tests/contextmenu.js:12: Need a test for a case where an element without contextmenu attribute. What should happen if the referred <menu> has no type=popup? What does the specification say about it? https://codereview.chromium.org/243403006/diff/610001/LayoutTests/fast/dom/HT... LayoutTests/fast/dom/HTMLElement/script-tests/contextmenu.js:22: shouldBe("container3.contextMenu.id", "'menu3'"); Use shouldBeEqualToString(). https://codereview.chromium.org/243403006/diff/610001/LayoutTests/fast/dom/HT... LayoutTests/fast/dom/HTMLElement/script-tests/contextmenu.js:34: shouldBe("container4.contextMenu", "null"); Use shouldBeNull(). https://codereview.chromium.org/243403006/diff/610001/LayoutTests/fast/dom/HT... LayoutTests/fast/dom/HTMLElement/script-tests/contextmenu.js:49: container5.contextMenu = menu2; Set menu3 instead of menu2. What should happen in such case? https://codereview.chromium.org/243403006/diff/610001/LayoutTests/fast/dom/HT... LayoutTests/fast/dom/HTMLElement/script-tests/contextmenu.js:52: shouldBe("container5.contextMenu.label", "'menu 5'"); What should happen if - element.contextMenu = null - element.contextMenu = "foobar" - element.contextMenu = document.createComment('foo') - element.contextMenu = menu-element-with-id-without-type - element.contextMenu = menu-element-with-id-in-ShadowRoot https://codereview.chromium.org/243403006/diff/610001/Source/core/page/Contex... File Source/core/page/ContextMenuController.cpp (right): https://codereview.chromium.org/243403006/diff/610001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:92: if (node && node->isHTMLElement() && RuntimeEnabledFeatures::contextMenuEnabled()) { We prefer early return, and let's check the flag first. if (!RuntimeEnabelFeatures::...) return; Node* node = ... if (!node || !node->isHTMLElement()) return; https://codereview.chromium.org/243403006/diff/610001/Source/core/page/Contex... Source/core/page/ContextMenuController.cpp:96: if (menuElement->dispatchEvent(relatedEvent.release())) { We prefer early return. if (!menuElement->dispatchEvent...) return; if (menuElement != element.contextMenu()) return; https://codereview.chromium.org/243403006/diff/610001/Source/core/page/Contex... File Source/core/page/ContextMenuController.h (right): https://codereview.chromium.org/243403006/diff/610001/Source/core/page/Contex... Source/core/page/ContextMenuController.h:43: class HTMLElement; This is unnecessary. https://codereview.chromium.org/243403006/diff/610001/Source/core/page/Contex... Source/core/page/ContextMenuController.h:44: class HTMLMenuElement; Ditto. https://codereview.chromium.org/243403006/diff/610001/Source/core/page/Custom... File Source/core/page/CustomContextMenuProvider.h (right): https://codereview.chromium.org/243403006/diff/610001/Source/core/page/Custom... Source/core/page/CustomContextMenuProvider.h:37: Vector<HTMLElement*> m_menuItems; Vector<HTMLElement*> -> WillBePersistentHeapVector<RefPtrWillBeMember<HTMLElement> >
> Can you please tell me some test which would fail with the current > implementation? You should refer to the specification, and test behaviors defined by the specification.
This CL still needs a lot of work. I recommend to split this to multiple CLs to proceed smoothly. 1. RuntimeEnabledFeatures.in 2. RelatedEvent and its tests 3. Addition of <menuitem>, HTMLMenuItemElement, new HTMLMenuElement IDL attributes, and their tests 4. Remaining |
