Bug 37759 - [Qt] need an implementation of LayoutTestController::computedStyleIncludingVisitedInfo
Summary: [Qt] need an implementation of LayoutTestController::computedStyleIncludingVi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Yi Shen
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-04-17 15:21 PDT by Yi Shen
Modified: 2010-05-24 08:27 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch (4.94 KB, patch)
2010-04-17 15:27 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
fix style error (4.93 KB, patch)
2010-04-17 15:36 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
proposed patch (4.99 KB, patch)
2010-04-19 09:27 PDT, Yi Shen
hausmann: review-
Details | Formatted Diff | Diff
returns the qvariantmap as return value in DumpRenderTreeSupportQt (4.97 KB, patch)
2010-04-19 13:01 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
unskip the tests (6.39 KB, patch)
2010-04-20 07:26 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
patch v6 (10.88 KB, patch)
2010-05-13 09:03 PDT, Antonio Gomes
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yi Shen 2010-04-17 15:21:55 PDT
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.
Comment 1 Yi Shen 2010-04-17 15:27:47 PDT
Created attachment 53614 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-04-17 15:30:59 PDT
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.
Comment 3 Yi Shen 2010-04-17 15:36:51 PDT
Created attachment 53615 [details]
fix style error
Comment 4 Yi Shen 2010-04-19 09:27:31 PDT
Created attachment 53684 [details]
proposed patch
Comment 5 Simon Hausmann 2010-04-19 12:04:12 PDT
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?
Comment 6 Yi Shen 2010-04-19 12:17:55 PDT
I tried to make this function more efficient by avoiding the copy construction of QVariantMap. Please let me know your thoughts :)
Comment 7 Yi Shen 2010-04-19 13:01:31 PDT
Created attachment 53709 [details]
returns the qvariantmap as return value in DumpRenderTreeSupportQt
Comment 8 Simon Hausmann 2010-04-19 13:21:27 PDT
(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 9 Yi Shen 2010-04-19 13:23:51 PDT
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 10 Simon Hausmann 2010-04-19 13:25:25 PDT
Comment on attachment 53709 [details]
returns the qvariantmap as return value in DumpRenderTreeSupportQt

Doesn't this also unskip some tests?
Comment 11 Yi Shen 2010-04-19 13:31:25 PDT
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?
Comment 12 Yi Shen 2010-04-20 07:26:49 PDT
Created attachment 53815 [details]
unskip the tests
Comment 13 WebKit Commit Bot 2010-04-21 03:54:55 PDT
Comment on attachment 53815 [details]
unskip the tests

Clearing flags on attachment: 53815

Committed r57963: <http://trac.webkit.org/changeset/57963>
Comment 14 WebKit Commit Bot 2010-04-21 03:55:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Review Bot 2010-04-21 04:14:24 PDT
http://trac.webkit.org/changeset/57963 might have broken Qt Linux Release
Comment 16 Eric Seidel (no email) 2010-04-21 04:15:10 PDT
Reverted r57963 for reason:

Three tests started crashing on the Qt bot.

Committed r57966: <http://trac.webkit.org/changeset/57966>
Comment 17 Yi Shen 2010-04-21 11:19:47 PDT
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"...
Comment 18 Yi Shen 2010-04-21 11:28:08 PDT
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 19 Eric Seidel (no email) 2010-04-21 18:13:41 PDT
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.
Comment 20 Yi Shen 2010-04-22 09:00:37 PDT
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.
Comment 21 Antonio Gomes 2010-05-09 21:53:10 PDT
yi, maybe use the same approach we did for bug 37323?
Comment 22 Yi Shen 2010-05-10 06:14:34 PDT
(In reply to comment #21)
> yi, maybe use the same approach we did for bug 37323?

Sure, I am fine with it. Thanks
Comment 23 Antonio Gomes 2010-05-11 14:17:01 PDT
(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.
Comment 24 Yi Shen 2010-05-11 14:20:36 PDT
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.
Comment 25 Antonio Gomes 2010-05-13 09:03:33 PDT
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 26 Kenneth Rohde Christiansen 2010-05-13 09:38:25 PDT
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.
Comment 27 Antonio Gomes 2010-05-13 11:09:18 PDT
Committed r59373: <http://trac.webkit.org/changeset/59373>
Comment 28 Csaba Osztrogonác 2010-05-21 15:33:36 PDT
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
Comment 29 Dave Hyatt 2010-05-21 15:36:36 PDT
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?
Comment 30 Antonio Gomes 2010-05-21 21:45:21 PDT
(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?
Comment 31 Antonio Gomes 2010-05-24 06:51:24 PDT
(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.
Comment 32 Antonio Gomes 2010-05-24 08:27:56 PDT
(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