Bug 59638

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.

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

Created attachment 91555 [details] fix patch

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.

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

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

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

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

> 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!

Created attachment 91947 [details] fix patch 2 added wrapper class

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

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

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

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!

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

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

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.

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

Created attachment 92253 [details] fix patch 4

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

Created attachment 92271 [details] fix patch 5

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.

Created attachment 92275 [details] fix patch 5

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

Comment on attachment 92275 [details] fix patch 5 Clearing flags on attachment: 92275 Committed r85867: <http://trac.webkit.org/changeset/85867>

All reviewed patches have been landed. Closing bug.

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