Bug 47849 – Dump the gesture status of frame in frame load callbacks in DumpRenderTree by adding a new method dumpUserGestureInFrameLoadCallbacks()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Johnny(Jianning) Ding]

We want to get the gesture status of main frame without JavaScript on the stack. Please refer to bug 47817 for more details.

[Johnny(Jianning) Ding]

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

[Johnny(Jianning) Ding]

Created attachment 71321 [details]
patch V1

Add the dumpUserGestureInFrameLoadCallbacks method in DumpRenderTree's mac/qt/chromium ports.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 71321 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4639001

[Johnny(Jianning) Ding]

Created attachment 71323 [details]
patch v1 (fixed compilation error and style lint)

[Adam Barth]

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.  :)

[Tony Chang]

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?

[Johnny(Jianning) Ding]

(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?

[Johnny(Jianning) Ding]

(In reply to comment #6)
> This ChangeLog entry seems unrelated to this patch.  :)
Thanks:-) will drop it in new patch.

[Adam Barth]

> @Adam, what's your suggestion?

Makes sense to me.

[WebKit Review Bot]

Attachment 71321 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4627005

[WebKit Review Bot]

Attachment 71323 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4705001

[Johnny(Jianning) Ding]

Created attachment 71372 [details]
patch v2 (passed chromium compilation)

[Tony Chang]

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.

[Johnny(Jianning) Ding]

(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

[Johnny(Jianning) Ding]

Created attachment 71392 [details]
patch V2 (fixed Tony's nit)

[WebKit Review Bot]

Comment on attachment 71392 [details]
patch V2 (fixed Tony's nit)

Rejecting patch 71392 from commit-queue.

[e-mail] 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.

[WebKit Review Bot]

Attachment 71392 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4657038

[Johnny(Jianning) Ding]

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.

[WebKit Commit Bot]

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 [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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

[Johnny(Jianning) Ding]

> 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!

[Tony Chang]

Comment on attachment 71633 [details]
landing patch

Trying again.

[WebKit Commit Bot]

Comment on attachment 71633 [details]
landing patch

Clearing flags on attachment: 71633

Committed r70470: <http://trac.webkit.org/changeset/70470>

[Johnny(Jianning) Ding]

Patch has been landed, closing this bug.
Thanks, Tony!