Bug 63857 – [DRT] implement createPopupMenu for Chromium DRT port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Johnny(Jianning) Ding]

In WebKit, the keystroke configurations of selection list below (see dom/SelectElement.cpp)

// Configure platform-specific behavior when focused pop-up receives arrow/space/return keystroke.
// (PLATFORM(MAC) and PLATFORM(GTK) are always false in Chromium, hence the extra tests.)
#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN))
#define ARROW_KEYS_POP_MENU 1
#define SPACE_OR_RETURN_POP_MENU 0
#elif PLATFORM(GTK) || (PLATFORM(CHROMIUM) && OS(UNIX))
#define ARROW_KEYS_POP_MENU 0
#define SPACE_OR_RETURN_POP_MENU 1
#else
#define ARROW_KEYS_POP_MENU 0
#define SPACE_OR_RETURN_POP_MENU 0
#endif

According to the configuration, On Linux and Chromium unix-like platforms, with configuration of SPACE_OR_RETURN_POP_MENU, pressing '\r' or space on a <select> which acts as menuList render object is to trigger showing popup menu. (instead of doing implicit submit, see SelectElement::menuListDefaultEventHandler.). However the behavior causes crash on Chromium's layout tests. It's because showing popup calls WebViewHost::createPopupMenu() which is not implemented by Chromium DRT. Like test LayoutTest/fast/forms/select-script-onchange.html. See bug 61515.

I am working on a patch to fix this issue, which simply creating WebPopupMenu with a empty WidgetClient delegate. It should be enough for layout test.

[Johnny(Jianning) Ding]

Created attachment 99545 [details]
patch v1

[WebKit Review Bot]

Attachment 99545 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Tools/DumpRenderTree/chromium/WebViewHost.cpp:244:  One space before end of line comments  [whitespace/comments] [5]
Tools/DumpRenderTree/chromium/WebViewHost.cpp:248:  Use 0 instead of NULL.  [readability/null] [5]
Tools/DumpRenderTree/chromium/WebViewHost.cpp:256:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Satish Sampath]

Comment on attachment 99545 [details]
patch v1

Looks good to me, but someone else has to approve as I'm not a reviewer.
Please fix all reported style issues as well.

In file Tools/DumpRenderTree/chromium/WebViewHost.cpp line 250:
  Indent to right by 2 more spaces?

[Tony Chang]

Comment on attachment 99545 [details]
patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=99545&action=review

Just some small style nits.

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:249
> +        if (widget)

Nit: Do we really need this check?  Shouldn't WebPopupMenu::create always succeed?

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:254
> +    default:
> +        ASSERT_NOT_REACHED();

I would remove the default case.  That way if someone adds another WebPopupType, there will be a compiler error and the person can update this code.

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:261
> +    // In Chromium, we do not this method.

Nit: This comment isn't that useful.  Can you either explain why we don't need this method or just remove the comment?

[Johnny(Jianning) Ding]

Created attachment 99845 [details]
patch v1 (fix style complain)

[WebKit Review Bot]

Attachment 99845 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/platform/chromium/test_expectations.txt:3990:  Can't specify both modifier 'snowleopard' and macro 'mac' fast/events/click-focus-anchor.html  [test/expectations] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Johnny(Jianning) Ding]

Created attachment 99880 [details]
patch v2

> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:249
> > +        if (widget)
> 
> Nit: Do we really need this check?  Shouldn't WebPopupMenu::create always succeed?

If not considering OOM situation, we need not to check the pointer. I will drop it since WebKit will eventually crash if OOM.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:254
> > +    default:
> > +        ASSERT_NOT_REACHED();
> 
> I would remove the default case.  That way if someone adds another WebPopupType, there will be a compiler error and the person can update this code.

done.

> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:261
> > +    // In Chromium, we do not this method.
> 
> Nit: This comment isn't that useful.  Can you either explain why we don't need this method or just remove the comment?

done.

[WebKit Review Bot]

Comment on attachment 99880 [details]
patch v2

Rejecting attachment 99880 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
yer request failed: MERGE of '/repository/webkit/trunk': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 572


Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/
Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/8984925

[Tony Chang]

Comment on attachment 99880 [details]
patch v2

trying again

[WebKit Review Bot]

Comment on attachment 99880 [details]
patch v2

Clearing flags on attachment: 99880

Committed r90518: <http://trac.webkit.org/changeset/90518>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

*** Bug 61515 has been marked as a duplicate of this bug. ***