Bug 37759 – [Qt] need an implementation of LayoutTestController::computedStyleIncludingVisitedInfo

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

[Yi Shen]

To fix Bug 37323 - [Qt] tst_QWebHistoryInterface::visitedLinks() fails (https://bugs.webkit.org/show_bug.cgi?id=37323), which is because of the new :visited patch (http://trac.webkit.org/changeset/57292). We can have an implementation of LayoutTestController::computedStyleIncludingVisitedInfo to know the real visited link info.

[Yi Shen]

Created attachment 53614 [details]
proposed patch

[WebKit Review Bot]

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.

[Yi Shen]

Created attachment 53615 [details]
fix style error

[Yi Shen]

Created attachment 53684 [details]
proposed patch

[Simon Hausmann]

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?

[Yi Shen]

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

[Yi Shen]

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

[Simon Hausmann]

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

[Yi Shen]

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.

[Simon Hausmann]

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

Doesn't this also unskip some tests?

[Yi Shen]

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?

[Yi Shen]

Created attachment 53815 [details]
unskip the tests

[WebKit Commit Bot]

Comment on attachment 53815 [details]
unskip the tests

Clearing flags on attachment: 53815

Committed r57963: <http://trac.webkit.org/changeset/57963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Eric Seidel (no email)]

Reverted r57963 for reason:

Three tests started crashing on the Qt bot.

Committed r57966: <http://trac.webkit.org/changeset/57966>

[Yi Shen]

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"...

[Yi Shen]

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!

[Eric Seidel (no email)]

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.

[Yi Shen]

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.

[Antonio Gomes]

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

[Yi Shen]

(In reply to comment #21)
> yi, maybe use the same approach we did for bug 37323?

Sure, I am fine with it. Thanks

[Antonio Gomes]

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

[Yi Shen]

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.

[Antonio Gomes]

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.

[Kenneth Rohde Christiansen]

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.

[Antonio Gomes]

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

[Csaba Osztrogonác]

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

[Dave Hyatt]

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?

[Antonio Gomes]

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

[Antonio Gomes]

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

[Antonio Gomes]

(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