Bug 63857 - [DRT] implement createPopupMenu for Chromium DRT port
Summary: [DRT] implement createPopupMenu for Chromium DRT port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 61515 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-07-01 18:16 PDT by Johnny(Jianning) Ding
Modified: 2011-07-07 10:17 PDT (History)
4 users (show)

See Also:


Attachments
patch v1 (5.82 KB, patch)
2011-07-01 18:31 PDT, Johnny(Jianning) Ding
tony: review-
Details | Formatted Diff | Diff
patch v1 (fix style complain) (5.78 KB, patch)
2011-07-06 09:45 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2 (5.71 KB, patch)
2011-07-06 14:23 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johnny(Jianning) Ding 2011-07-01 18:16:54 PDT
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.
Comment 1 Johnny(Jianning) Ding 2011-07-01 18:31:50 PDT
Created attachment 99545 [details]
patch v1
Comment 2 WebKit Review Bot 2011-07-01 18:33:10 PDT
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.
Comment 3 Satish Sampath 2011-07-04 04:27:09 PDT
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?
Comment 4 Tony Chang 2011-07-06 09:44:13 PDT
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?
Comment 5 Johnny(Jianning) Ding 2011-07-06 09:45:46 PDT
Created attachment 99845 [details]
patch v1 (fix style complain)
Comment 6 WebKit Review Bot 2011-07-06 09:48:10 PDT
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.
Comment 7 Johnny(Jianning) Ding 2011-07-06 14:23:14 PDT
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.
Comment 8 WebKit Review Bot 2011-07-06 16:14:41 PDT
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
Comment 9 Tony Chang 2011-07-06 16:46:42 PDT
Comment on attachment 99880 [details]
patch v2

trying again
Comment 10 WebKit Review Bot 2011-07-06 17:09:49 PDT
Comment on attachment 99880 [details]
patch v2

Clearing flags on attachment: 99880

Committed r90518: <http://trac.webkit.org/changeset/90518>
Comment 11 WebKit Review Bot 2011-07-06 17:09:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Eric Seidel (no email) 2011-07-07 10:17:17 PDT
*** Bug 61515 has been marked as a duplicate of this bug. ***