Summary: | [Qt] JSValue QtClass::fallbackObject can be optimized | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Anders Bakken <agbakken> | ||||||||
Component: | WebKit Qt | Assignee: | Anders Bakken <agbakken> | ||||||||
Status: | CLOSED FIXED | ||||||||||
Severity: | Enhancement | CC: | commit-queue, hausmann, jesus, kenneth, kimmo.t.kinnunen | ||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 35784 | ||||||||||
Attachments: |
|
(In reply to comment #0) > The attached patch improves this. It is necessary to mention this bug report on the Changelog. Also, do you want it to be reviewed? If so you have to change it to r? . Kenneth, can you take a look at this? LGTM. ;) I guess you wanted to put this up for review I did want to put it up for review. Sorry about that :-) Comment on attachment 53489 [details]
patch that reduces memcpy'ing
Looks good Anders!
Comment on attachment 53489 [details]
patch that reduces memcpy'ing
I'll add the bug number to the changelog and upload a new patch. Just following the guidelines... ;)
Created attachment 55837 [details]
Patch
Added the bug number to the Changelog.
Comment on attachment 53489 [details]
patch that reduces memcpy'ing
Clearing flags of old patch.
Comment on attachment 55837 [details] Patch Rejecting patch 55837 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 2 Parsed 2 diffs from patch file(s). patching file WebCore/ChangeLog patch: **** malformed patch at line 18: 2010-04-15 Adam Roben <aroben@apple.com> patching file WebCore/bridge/qt/qt_class.cpp Full output: http://webkit-commit-queue.appspot.com/results/2315058 Created attachment 56062 [details]
New version of the patch
Comment on attachment 56062 [details] New version of the patch Rejecting patch 56062 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing/iframes', '--quiet']" exit_code: 1 Last 500 characters of output: ng Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 18341 test cases. fast/loader/recursive-before-unload-crash.html -> failed Exiting early after 1 failures. 13417 tests run. 198.65s total testing time 13416 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/2265108 Anders, you cannot remove the "Reviewed by NOBODY (OOPS!!)". If you do that we cannot land using the commit-queue. It has to be exactly like the prepare-ChangeLog scripts writes it. Now you can reupload the patch with this fixed and reset r? and cq? -- or -- You can upload the patch with "Reviewed by Kenneth Rohde Christiansen" and just set cq? The latter is preferred as anyone with committer-rights can change the cq? to cq+ Another option would be to get someone to land it manually using webkit-patch or similar. Comment on attachment 56062 [details]
New version of the patch
I think you were just bit by a flaky test.
Comment on attachment 56062 [details] New version of the patch Clearing flags on attachment: 56062 Committed r59563: <http://trac.webkit.org/changeset/59563> All reviewed patches have been landed. Closing bug. Revision r59563 cherry-picked into qtwebkit-2.0 with commit 4bb49bb59d4f763069ae6bb84b9648d6ed0bfd1d This commit causes a bug which prevents signals from being matched. It introduces second local variable named "index", which then overrides the proper method number variable (also called "index") this causes signals from being found in some cases. The affected line is: QtRuntimeMetaMethod* val = new (exec) QtRuntimeMetaMethod(exec, identifier, static_cast<QtInstance*>(inst), index, normal, false); Uh, I wasn't able to see that from the diff. I will fix it later today Patch shadowed a variable. Fixed regression in r59606 Oops. My bad. <cherr-pick-for-backport: r59606> <cherry-pick-for-backport: r59606> Revision r59606 cherry-picked into qtwebkit-2.0 with commit 807157e42add842605ec67d9363dd3f1861748ca |
Created attachment 53489 [details] patch that reduces memcpy'ing JSValue QtClass::fallbackObject(ExecState* exec, Instance* inst, const Identifier& identifier) copies and modifies more string data than it needs to. The attached patch improves this.