Bug 37684 – [Qt] JSValue QtClass::fallbackObject can be optimized

• Status: CLOSED
• Resolution: FIXED
• Component: WebKit Qt

[Anders Bakken]

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.

[Jesus Sanchez-Palencia]

(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. ;)

[Kenneth Rohde Christiansen]

I guess you wanted to put this up for review

[Anders Bakken]

I did want to put it up for review. Sorry about that :-)

[Kenneth Rohde Christiansen]

Comment on attachment 53489 [details]
patch that reduces memcpy'ing

Looks good Anders!

[Jesus Sanchez-Palencia]

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

[Jesus Sanchez-Palencia]

Created attachment 55837 [details]
Patch

Added the bug number to the Changelog.

[Jesus Sanchez-Palencia]

Comment on attachment 53489 [details]
patch that reduces memcpy'ing

Clearing flags of old patch.

[WebKit Commit Bot]

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

patching file WebCore/bridge/qt/qt_class.cpp

Full output: http://webkit-commit-queue.appspot.com/results/2315058

[Anders Bakken]

Created attachment 56062 [details]
New version of the patch

[WebKit Commit Bot]

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

[Kenneth Rohde Christiansen]

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.

[Adam Barth]

Comment on attachment 56062 [details]
New version of the patch

I think you were just bit by a flaky test.

[WebKit Commit Bot]

Comment on attachment 56062 [details]
New version of the patch

Clearing flags on attachment: 56062

Committed r59563: <http://trac.webkit.org/changeset/59563>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r59563 cherry-picked into qtwebkit-2.0 with commit 4bb49bb59d4f763069ae6bb84b9648d6ed0bfd1d

[Kimmo Kinnunen]

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);

[Kenneth Rohde Christiansen]

Uh, I wasn't able to see that from the diff. I will fix it later today

[Kenneth Rohde Christiansen]

Patch shadowed a variable.

[Kenneth Rohde Christiansen]

Fixed regression in r59606

[Anders Bakken]

Oops. My bad.

[Simon Hausmann]

<cherr-pick-for-backport: r59606>

[Simon Hausmann]

<cherry-pick-for-backport: r59606>

[Simon Hausmann]

Revision r59606 cherry-picked into qtwebkit-2.0 with commit 807157e42add842605ec67d9363dd3f1861748ca