Bug 59638 – [Qt][WK2] Support PageGroup in QGraphicsWKView/QWKPage constructors

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

[Csaba Osztrogonác]

r84979: http://webkit.sed.hu/buildbot/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r84979%20%286279%29/results.html
r84980: http://webkit.sed.hu/buildbot/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r84980%20%286243%29/results.html

[Chang Shu]

The root cause of the problem is actually the Qt implementation is missing the WKPageGroup parameter in QWKView constructors, etc and the defaultPageGroup is used. As a result, the preference settings are not able to be transferred to WebProcess side. r84980 exposed the problem.

I am working on a patch.

[Chang Shu]

Original title: REGRESSION(r84980): It made 11 tests fail
The WKPageGroupRef parameter needs to be passed down to QWKPage.

[Chang Shu]

Created attachment 91555 [details]
fix patch

[Benjamin Poulain]

Comment on attachment 91555 [details]
fix patch

You should not use the opaque type from the C API in the C++ Qt API. So at least you should write a nice wrapper for PageGroup to make this feature.

But I actually don't see a use case for exposing the page group so I think it should be handled internally for our APIs. If you don't agree, please add a relevant test for the use case of multiple page group.

[Chang Shu]

(In reply to comment #4)
> (From update of attachment 91555 [details])
> You should not use the opaque type from the C API in the C++ Qt API. So at least you should write a nice wrapper for PageGroup to make this feature.
> 
> But I actually don't see a use case for exposing the page group so I think it should be handled internally for our APIs. If you don't agree, please add a relevant test for the use case of multiple page group.

I don't understand your 2nd part. The use case is in the patch itself.

[Benjamin Poulain]

(In reply to comment #5)
> > But I actually don't see a use case for exposing the page group so I think it should be handled internally for our APIs. If you don't agree, please add a relevant test for the use case of multiple page group.
> 
> I don't understand your 2nd part. The use case is in the patch itself.

Trying again :)
1) What is the use case for _users_ (Qt developers) to use PageGroup. I never use page group so for me it looks like a obscure feature.
2) If there is such use case, it should be expressed clearly in the tests.

Adding API has a huge cost. It increase complexity for the users, for us engineers and for the support organization, and it generally limits the direction of future development of the code. So unless something is really useful for lots of people, we tend not to create public API.

[Chang Shu]

> Trying again :)
> 1) What is the use case for _users_ (Qt developers) to use PageGroup. I never use page group so for me it looks like a obscure feature.
> 2) If there is such use case, it should be expressed clearly in the tests.
> 
> Adding API has a huge cost. It increase complexity for the users, for us engineers and for the support organization, and it generally limits the direction of future development of the code. So unless something is really useful for lots of people, we tend not to create public API.

I was thinking what I was missing after I replied. Yes, interestingly, the patch doesn't explicitly tell where exactly the use case is, my bad. Actually, it starts from WebKitTestRunner/TestController. See links below:
http://trac.webkit.org/browser/trunk/Tools/WebKitTestRunner/TestController.cpp#L372.
All the WKPreferencesSetXYZs are failing(seriously!) because UIProcess fails to send the preference change to WebProcess. And the reason is in WebPreferences::update(), m_pageGroups is not the right one. The original page group in TestController didn't set to WebPage because of lack of this API.
http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/WebPreferences.cpp#L58

Hopefully, that explains well. With this patch, all the regressed tests reported by Ossy should be fixed.

[Benjamin Poulain]

(In reply to comment #7)
> Hopefully, that explains well. With this patch, all the regressed tests reported by Ossy should be fixed.

I understand this fixes the test, that is not what I am talking about. You can add special private API for the test runner, look at how the other tests are written.

I ask about use case for _USER_ of Qt, the developers. You are adding _PUBLIC APIs_ with this patch.

[Chang Shu]

> I understand this fixes the test, that is not what I am talking about. You can add special private API for the test runner, look at how the other tests are written.
> 
> I ask about use case for _USER_ of Qt, the developers. You are adding _PUBLIC APIs_ with this patch.

Ok. I will have to come back to this on next Monday as I am taking Friday off. It will save me some time if you can show me some private API examples. Thanks!

[Chang Shu]

Created attachment 91947 [details]
fix patch 2

added wrapper class

[Csaba Osztrogonác]

(In reply to comment #10)
> Created an attachment (id=91947) [details]
> fix patch 2
> 
> added wrapper class

I tested this patch, it fixed the 11 tests, but broke another 87. :-/

[Chang Shu]

> > added wrapper class
> 
> I tested this patch, it fixed the 11 tests, but broke another 87. :-/

Can you show me a couple of failed tests?

[Csaba Osztrogonác]

Created attachment 92062 [details]
fails with "fix patch 2"

[Chang Shu]

Created attachment 92070 [details]
fix patch 3

Because the patch fixed set WKPreference issue, WKPreferenceSetFontFamily started to work and messed up the fonts. I had to disable this code in WTR.
Ossy, can you try again? Thanks!

[Csaba Osztrogonác]

(In reply to comment #14)
> Created an attachment (id=92070) [details]
> fix patch 3
> 
> Because the patch fixed set WKPreference issue, WKPreferenceSetFontFamily started to work and messed up the fonts. I had to disable this code in WTR.
> Ossy, can you try again? Thanks!

I tested it and I got only 4 fails, but they are absolutely unrelated to this patch, they are very old fails. ;)

[Chang Shu]

> > Because the patch fixed set WKPreference issue, WKPreferenceSetFontFamily started to work and messed up the fonts. I had to disable this code in WTR.
> > Ossy, can you try again? Thanks!
> 
> I tested it and I got only 4 fails, but they are absolutely unrelated to this patch, they are very old fails. ;)

Great! ping reviewer.

[Csaba Osztrogonác]

I skipped the tests because I hate red bots by heart: http://trac.webkit.org/changeset/85664

Please don't forget unskip them after the fix landed.

[Andreas Kling]

What is the use-case for exposing QWKPageGroup as public Qt API?

If this is only needed for WTR, couldn't we simply have a private QGraphicsWKView constructor that takes the opaque type and percolates the PG down to WebContext::createWebPage()?

[Chang Shu]

Created attachment 92253 [details]
fix patch 4

[Early Warning System Bot]

Attachment 92253 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8558536

[Chang Shu]

Created attachment 92271 [details]
fix patch 5

[Eric Seidel (no email)]

Attachment 92271 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qgraphicswkview.h:97:  The parameter name "backingStoreType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chang Shu]

Created attachment 92275 [details]
fix patch 5

[Andreas Kling]

Comment on attachment 92275 [details]
fix patch 5

Sure, r=me.
Please open a bug about the fonts issue if there isn't one already. :)

[WebKit Commit Bot]

Comment on attachment 92275 [details]
fix patch 5

Clearing flags on attachment: 92275

Committed r85867: <http://trac.webkit.org/changeset/85867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

*** Bug 59446 has been marked as a duplicate of this bug. ***