Bug 37759

Created attachment 53614 [details] proposed patch

Attachment 53614 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 53615 [details] fix style error

Created attachment 53684 [details] proposed patch

Comment on attachment 53684 [details] proposed patch > +void DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo(QWebFrame* frame, const QString& id, QVariantMap& infoMap) Most of the patch looks fine to me, but why not simply return the QVariantMap as return value of the function instead of using an out argument?

I tried to make this function more efficient by avoiding the copy construction of QVariantMap. Please let me know your thoughts :)

Created attachment 53709 [details] returns the qvariantmap as return value in DumpRenderTreeSupportQt

(In reply to comment #6) > I tried to make this function more efficient by avoiding the copy construction > of QVariantMap. Please let me know your thoughts :) QVariantMap is implicitly shared, so returning a copy is very efficient.

Oh, thanks :) I have updated the patch. (In reply to comment #8) > (In reply to comment #6) > > I tried to make this function more efficient by avoiding the copy construction > > of QVariantMap. Please let me know your thoughts :) > > QVariantMap is implicitly shared, so returning a copy is very efficient.

Comment on attachment 53709 [details] returns the qvariantmap as return value in DumpRenderTreeSupportQt Doesn't this also unskip some tests?

it doesn't unskip tests. Do you want me to add them in this patch? (In reply to comment #10) > (From update of attachment 53709 [details]) > Doesn't this also unskip some tests?

Created attachment 53815 [details] unskip the tests

Comment on attachment 53815 [details] unskip the tests Clearing flags on attachment: 53815 Committed r57963: <http://trac.webkit.org/changeset/57963>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/57963 might have broken Qt Linux Release

Reverted r57963 for reason: Three tests started crashing on the Qt bot. Committed r57966: <http://trac.webkit.org/changeset/57966>

The "crash" only happens when running all four tests at one time, or one test with multiple times of calling the new interface (for example), var ele = document.getElementById('one'); window.layoutTestController.computedStyleIncludingVisitedInfo(ele); window.layoutTestController.computedStyleIncludingVisitedInfo(ele); window.layoutTestController.computedStyleIncludingVisitedInfo(ele); window.layoutTestController.computedStyleIncludingVisitedInfo(ele); window.layoutTestController.computedStyleIncludingVisitedInfo(ele); Plus, the "crash" is only made by the run-webkit-test script, I mean, the DumpRenderTree doesn't crash at all. Also, if I change the parameter type of computedStyleIncludingVisitedInfo(), from QVariant to others, e.g. QString, everything works fine -- no crash, no error. I believe the root cause of the crash is from run-webkit-test script and I do find there is something wrong with it, for example, the script reads the output of DumpRenderTree to determine whether there is a crash or not, sub readFromDumpToolWithTimer(**) { ... while (1) { ... my $lineIn = readline($fhIn) unless $haveSeenEofIn; my $lineError = readline($fhError) unless $haveSeenEofError; if (!defined($lineIn) && !defined($lineError)) { ... if ( $! != EAGAIN) { $status = "crashed"; last; } $! is the error no. and EAGAIN is 11, which is a recoverable error. While I found the $! equals to 0 in our case which sets the status to "crashed"...

I have also tried to use a slow test by setting a large timeout, because the new interface may need spending more time on javascript binding (convert element to QVarient). However, it doesn't help. Any help and suggestion is super welcome!

Comment on attachment 53709 [details] returns the qvariantmap as return value in DumpRenderTreeSupportQt Cleared Simon Hausmann's review+ from obsolete attachment 53709 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Made more test for the run-webkit-test with the new interface and found the readFromDumpToolWithTimer() receives the error code 0, when calling the new interface more than 5 times. More detailly, I add some logs in qt_runtime.cpp, static int findMethodIndex( ... ) { fprintf(stderr, "call my new interface!\n"); ... // convert arguments for (unsigned i = 0; ...; ++i) { fprintf(stderr, "convert start ....\n"); QVariant v = convertValueToQVariant(exec, arg, ...); fprintf(stderr, "convert end ....\n"); } Then I checked the error output of the run-webkit-test after it crashed. There is no "convert end ..." for the fifth calling. Note that, if I run the DRT directly, there is no crash and I can see the "convert end ..." for all the function calls. The convertValueToQVariant is a recursive function and it might run many many times to deal with a big jsobject like the element obj. Somehow it doesn't work well with the run-webkit-test, which is using IPC::open3() to run the DRT.

yi, maybe use the same approach we did for bug 37323?

(In reply to comment #21) > yi, maybe use the same approach we did for bug 37323? Sure, I am fine with it. Thanks

(In reply to comment #22) > (In reply to comment #21) > > yi, maybe use the same approach we did for bug 37323? > > Sure, I am fine with it. Thanks great! yi, feel like doing this? if not, I can get to it tonight.

You can work on it. I am kind of busy with other stuff so ... Thanks (In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > yi, maybe use the same approach we did for bug 37323? > > > > Sure, I am fine with it. Thanks > > great! yi, feel like doing this? if not, I can get to it tonight.

Created attachment 55986 [details] patch v6 Patch is essentially similar to https://bugs.webkit.org/attachment.cgi?id=53815 but instead of Instead of receiving a QVariant as parameter of computedStyleIncludingVisitedInfo it is now getting a QWebElement . The parameters of DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo also changed accordingly , which makes the code simpler. > The "crash" only happens when running all four tests at one time, or one test with multiple times of calling the new interface (for > example), > var ele = document.getElementById('one'); > window.layoutTestController.computedStyleIncludingVisitedInfo(ele); > window.layoutTestController.computedStyleIncludingVisitedInfo(ele); > window.layoutTestController.computedStyleIncludingVisitedInfo(ele); > window.layoutTestController.computedStyleIncludingVisitedInfo(ele); > window.layoutTestController.computedStyleIncludingVisitedInfo(ele); I tested it and it works fine now. > Also, if I change the parameter type of computedStyleIncludingVisitedInfo(), from QVariant to others, e.g. QString, everything > works fine -- no crash, no error. I think that is the case of QWebElement as well. ps: Yi is still an author in the ChangeLog, since my work is based on his original work.

Comment on attachment 55986 [details] patch v6 WebKit/qt/ChangeLog:8 + why two new lines WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:384 + QVariantMap ret; ret? res would make more sense. computedStyle even more so.

Committed r59373: <http://trac.webkit.org/changeset/59373>

This implementation is incorrect and it caused a fail. I skipped fast/history/self-is-visited.html until fix: http://trac.webkit.org/changeset/59373

I don't think copying the properties works because of shorthands. You have to be able to query shorthands on the computed style object (e.g., background vs. background-color). Is there no DOM binding you can use to wrap the actual style object?

(In reply to comment #28) > This implementation is incorrect and it caused a fail. > I skipped fast/history/self-is-visited.html until fix: > http://trac.webkit.org/changeset/59373 (In reply to comment #29) > I don't think copying the properties works because of shorthands. You have to be able to query shorthands on the computed style object (e.g., background vs. background-color). Is there no DOM binding you can use to wrap the actual style object? hum, I think the implementation was correct when it landed. could you please tell if anything changed in the background, and needs changing here as well?

(In reply to comment #28) > This implementation is incorrect and it caused a fail. > I skipped fast/history/self-is-visited.html until fix: > http://trac.webkit.org/changeset/59373 It is not wrong. http://trac.webkit.org/changeset/59956 "broke" it. In fact, it changed the implementation in WebCore, and requires change in Qt side. This bug is fixed, and I will open another one to handle this specific failing test.

(In reply to comment #31) > (In reply to comment #28) > > This implementation is incorrect and it caused a fail. > > I skipped fast/history/self-is-visited.html until fix: > > http://trac.webkit.org/changeset/59373 > > It is not wrong. http://trac.webkit.org/changeset/59956 "broke" it. In fact, it changed the implementation in WebCore, and requires change in Qt side. > > This bug is fixed, and I will open another one to handle this specific failing test. fyi, filed bug 39597 about it