Bug 47849

Summary: Dump the gesture status of frame in frame load callbacks in DumpRenderTree by adding a new method dumpUserGestureInFrameLoadCallbacks()
Product: WebKit Reporter: Johnny(Jianning) Ding <jnd>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, commit-queue, dglazkov, jnd, pierre.lafayette, tony, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch V1
none
patch v1 (fixed compilation error and style lint)
none
patch v2 (passed chromium compilation)
none
patch V2 (fixed Tony's nit)
abarth: review+, webkit.review.bot: commit-queue-
landing patch none

Description Johnny(Jianning) Ding 2010-10-18 14:19:31 PDT
We want to get the gesture status of main frame without JavaScript on the stack. Please refer to bug 47817 for more details.
Comment 1 Johnny(Jianning) Ding 2010-10-20 12:44:41 PDT
I'd like to add a new method called dumpUserGestureInFrameLoadCallbacks() to dump the gesture status of frame in frame load callbacks in DumpRenderTree. With this change we can know the gesture status in the frame load callbacks. 

CC to tony@chromium since he is working on the Chromium DumpRenderTree port
Comment 2 Johnny(Jianning) Ding 2010-10-20 12:46:56 PDT
Created attachment 71321 [details]
patch V1

Add the dumpUserGestureInFrameLoadCallbacks method in DumpRenderTree's mac/qt/chromium ports.
Comment 3 WebKit Review Bot 2010-10-20 12:50:39 PDT
Attachment 71321 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:109:  _frame is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:111:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Early Warning System Bot 2010-10-20 12:55:10 PDT
Attachment 71321 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4639001
Comment 5 Johnny(Jianning) Ding 2010-10-20 13:13:07 PDT
Created attachment 71323 [details]
patch v1 (fixed compilation error and style lint)
Comment 6 Adam Barth 2010-10-20 13:25:20 PDT
Comment on attachment 71323 [details]
patch v1 (fixed compilation error and style lint)

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

> LayoutTests/ChangeLog:6
> +        Set the right referent sourceURL for the script with force user gesture.
> +        https://bugs.webkit.org/show_bug.cgi?id=37138

This ChangeLog entry seems unrelated to this patch.  :)
Comment 7 Tony Chang 2010-10-20 13:45:38 PDT
Comment on attachment 71323 [details]
patch v1 (fixed compilation error and style lint)

Do we need to dump the user gesture status for every callback during loading?  Would it be sufficient to just dump it once or twice during the frame load?
Comment 8 Johnny(Jianning) Ding 2010-10-20 13:52:19 PDT
(In reply to comment #7)
> (From update of attachment 71323 [details])
> Do we need to dump the user gesture status for every callback during loading?  Would it be sufficient to just dump it once or twice during the frame load?

I agree, AFAIK only the status in didStartProvisionalLoad is what we care. Maybe we can only dump gesture status in this callback. If someone is interested in the status in other frame-load callbacks, he/she can add the logic at that time.

@Adam, what's your suggestion?
Comment 9 Johnny(Jianning) Ding 2010-10-20 13:52:51 PDT
(In reply to comment #6)
> This ChangeLog entry seems unrelated to this patch.  :)
Thanks:-) will drop it in new patch.
Comment 10 Adam Barth 2010-10-20 14:01:52 PDT
> @Adam, what's your suggestion?

Makes sense to me.
Comment 11 WebKit Review Bot 2010-10-20 14:02:36 PDT
Attachment 71321 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4627005
Comment 12 WebKit Review Bot 2010-10-20 14:58:28 PDT
Attachment 71323 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4705001
Comment 13 Johnny(Jianning) Ding 2010-10-20 17:32:52 PDT
Created attachment 71372 [details]
patch v2 (passed chromium compilation)
Comment 14 Tony Chang 2010-10-20 17:39:57 PDT
Comment on attachment 71372 [details]
patch v2 (passed chromium compilation)

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

This looks OK to me.  Do we need to have entries for GTK+ and win in their Skipped files?

> WebKitTools/DumpRenderTree/chromium/LayoutTestController.h:471
> +    // If true, the test_shell will output a line of the user gesture status
> +    // text for each frame load callback.

Nit: I don't think this comment is accurate anymore.
Comment 15 Johnny(Jianning) Ding 2010-10-20 17:50:48 PDT
(In reply to comment #14)
Thanks, Tony!
> This looks OK to me.  Do we need to have entries for GTK+ and win in their Skipped files?
Yes, I guess all tests which use the dumpUserGestureInFrameLoadCallbacks need to add them in the skip list for GTK/win ports.
 
> Nit: I don't think this comment is accurate anymore.
Will change before landing
Comment 16 Johnny(Jianning) Ding 2010-10-20 22:51:22 PDT
Created attachment 71392 [details]
patch V2 (fixed Tony's nit)
Comment 17 WebKit Review Bot 2010-10-21 18:56:26 PDT
Comment on attachment 71392 [details]
patch V2 (fixed Tony's nit)

Rejecting patch 71392 from commit-queue.

jnd@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 18 WebKit Review Bot 2010-10-23 01:56:40 PDT
Attachment 71392 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4657038
Comment 19 Johnny(Jianning) Ding 2010-10-23 03:27:32 PDT
Created attachment 71633 [details]
landing patch

(In reply to comment #18)
> Build output: http://queues.webkit.org/results/4657038
fix it.  (pass compilation on mac via build-webkit --release --chromium), Since no logic changed, not to add review flag.
Comment 20 WebKit Commit Bot 2010-10-25 11:11:45 PDT
The commit-queue encountered the following flaky tests while processing attachment 71633 [details]:

http/tests/websocket/tests/close-on-unload.html
http/tests/appcache/origin-quota.html

Please file bugs against the tests.  The author(s) of the test(s) are abarth@webkit.org and joepeck@webkit.org.  The commit-queue is continuing to process your patch.
Comment 21 WebKit Commit Bot 2010-10-25 11:28:12 PDT
Comment on attachment 71633 [details]
landing patch

Rejecting patch 71633 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 71633]" exit_code: 1
Last 500 characters of output:
dit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=47849&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Updating working directory
Processing patch 71633 from bug 47849.
NOBODY (OOPS!) found in /Users/abarth/git/webkit-queue/WebKit/qt/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/abarth/git/webkit-queue/WebKit/qt/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/4617081
Comment 22 Johnny(Jianning) Ding 2010-10-25 11:34:02 PDT
> ERROR: /Users/abarth/git/webkit-queue/WebKit/qt/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/4617081

Oops, apparently my fault. The landing patch should have a review+ flag.
Hi Tony, Adam, would any of you please add the review+ and commit‑queue+ flag to the landing patch again?
Thanks!
Comment 23 Tony Chang 2010-10-25 11:34:21 PDT
Comment on attachment 71633 [details]
landing patch

Trying again.
Comment 24 WebKit Commit Bot 2010-10-25 12:03:28 PDT
Comment on attachment 71633 [details]
landing patch

Clearing flags on attachment: 71633

Committed r70470: <http://trac.webkit.org/changeset/70470>
Comment 25 Johnny(Jianning) Ding 2010-10-26 00:04:23 PDT
Patch has been landed, closing this bug.
Thanks, Tony!