| 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 / Tests | Assignee: | 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
Johnny(Jianning) Ding
2010-10-18 14:19:31 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 Created attachment 71321 [details]
patch V1
Add the dumpUserGestureInFrameLoadCallbacks method in DumpRenderTree's mac/qt/chromium ports.
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.
Attachment 71321 [details] did not build on qt: Build output: http://queues.webkit.org/results/4639001 Created attachment 71323 [details]
patch v1 (fixed compilation error and style lint)
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 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?
(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? (In reply to comment #6) > This ChangeLog entry seems unrelated to this patch. :) Thanks:-) will drop it in new patch. > @Adam, what's your suggestion?
Makes sense to me.
Attachment 71321 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4627005 Attachment 71323 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4705001 Created attachment 71372 [details]
patch v2 (passed chromium compilation)
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. (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 Created attachment 71392 [details]
patch V2 (fixed Tony's nit)
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. Attachment 71392 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4657038 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. 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 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 > 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 on attachment 71633 [details]
landing patch
Trying again.
Comment on attachment 71633 [details] landing patch Clearing flags on attachment: 71633 Committed r70470: <http://trac.webkit.org/changeset/70470> Patch has been landed, closing this bug. Thanks, Tony! |