Bug 154067 – [GTK][Threaded Compositor] The web inspector doesn't work when using the threaded compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Try to open the inspector in MiniBrowser and see that it's not rendered at all.

[ChangSeok Oh]

Let me pick this up.

[ChangSeok Oh]

Created attachment 271640 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 271640 [details]
Patch

Thanks for the patch! I think it would be better to do this in a single place then, so I think we can move this to WebPreferences::platformInitializeStore() since it will be common, and remove it from _WebKitSettingsPrivate().

[ChangSeok Oh]

Created attachment 271658 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[ChangSeok Oh]

Comment on attachment 271658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271658&action=review

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:75
>      if (webPage.corePage()->settings().acceleratedDrawingEnabled() || webPage.corePage()->settings().forceCompositingMode())

Or else we can move whole lines in this constructor into DrawingAreaImpl::updatePreferences(). What do you think?

[Carlos Garcia Campos]

Comment on attachment 271658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271658&action=review

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:73
> -
>  #if USE(COORDINATED_GRAPHICS_THREADED)
> -    webPage.corePage()->settings().setForceCompositingMode(true);
> +    webPage.corePage()->settings().setForceCompositingMode(parameters.store.getBoolValueForKey(WebPreferencesKey::forceCompositingModeKey()));
>  #endif

I think we don't even need this anymore. The drawing area is created by WebPage in its constructor and after that it calls updatePreferences that ends up calling DrawinArea::updatePreferences that already sets this setting.

[Carlos Garcia Campos]

Comment on attachment 271658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271658&action=review

>> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:75
>>      if (webPage.corePage()->settings().acceleratedDrawingEnabled() || webPage.corePage()->settings().forceCompositingMode())
> 
> Or else we can move whole lines in this constructor into DrawingAreaImpl::updatePreferences(). What do you think?

Actually, I don't see how this can work, because this happens before settings have been set in WebPage.

[ChangSeok Oh]

Comment on attachment 271658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271658&action=review

>> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:73
>>  #endif
> 
> I think we don't even need this anymore. The drawing area is created by WebPage in its constructor and after that it calls updatePreferences that ends up calling DrawinArea::updatePreferences that already sets this setting.

Why this line is needed here is because of following lines. If we just remove this line only over leaving below lines. We will see a crash just after launching the mini browser. So I asked how about moving all lines in the constructor into DrawingAreaImpl::updatePreferences(). =)

[Carlos Garcia Campos]

(In reply to comment #9)
> Comment on attachment 271658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271658&action=review
> 
> >> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:73
> >>  #endif
> > 
> > I think we don't even need this anymore. The drawing area is created by WebPage in its constructor and after that it calls updatePreferences that ends up calling DrawinArea::updatePreferences that already sets this setting.
> 
> Why this line is needed here is because of following lines. If we just
> remove this line only over leaving below lines. We will see a crash just
> after launching the mini browser. So I asked how about moving all lines in
> the constructor into DrawingAreaImpl::updatePreferences(). =)

Sounds good to me, I guess we would also need to enter AC mdoe only when there's no layer tree host yet

[Build Bot]

Comment on attachment 271658 [details]
Patch

Attachment 271658 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/849874

New failing tests:
storage/indexeddb/odd-strings-private.html

[Build Bot]

Created attachment 271660 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[ChangSeok Oh]

Comment on attachment 271658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271658&action=review

>>>> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:73
>>>>  #endif
>>> 
>>> I think we don't even need this anymore. The drawing area is created by WebPage in its constructor and after that it calls updatePreferences that ends up calling DrawinArea::updatePreferences that already sets this setting.
>> 
>> Why this line is needed here is because of following lines. If we just remove this line only over leaving below lines. We will see a crash just after launching the mini browser. So I asked how about moving all lines in the constructor into DrawingAreaImpl::updatePreferences(). =)
> 
> Sounds good to me, I guess we would also need to enter AC mdoe only when there's no layer tree host yet

I found another issue after moving the lines into DrawingAreaImpl::updatePreferences(). It messes up rendering when not using threaded compositor. While webPage.corePage()->settings().acceleratedDrawingEnabled() is false in the constructor, it is true in DrawingAreaImpl::updatePreferences. So it results in making m_alwaysUseCompositing true and calling enterAcceleratedCompositingMode(0) even when not using threaded compositor. This is because the default value of the acceleratedDrawingEnabled of WebPreference is true so that it changes settings().acceleratedDrawingEnabled() in WebPage::updatePreferences. I assume this is not intended. When not using threaded compositor, m_alwaysUseCompositing should be false, right? If so, we move the lines into DrawingAreaImpl::updatePreferences, but with a guard COORDINATED_GRAPHICS_THREADED. Or we can leave it as is. Any idea?

[Carlos Garcia Campos]

(In reply to comment #13)
> Comment on attachment 271658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271658&action=review
> 
> >>>> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:73
> >>>>  #endif
> >>> 
> >>> I think we don't even need this anymore. The drawing area is created by WebPage in its constructor and after that it calls updatePreferences that ends up calling DrawinArea::updatePreferences that already sets this setting.
> >> 
> >> Why this line is needed here is because of following lines. If we just remove this line only over leaving below lines. We will see a crash just after launching the mini browser. So I asked how about moving all lines in the constructor into DrawingAreaImpl::updatePreferences(). =)
> > 
> > Sounds good to me, I guess we would also need to enter AC mdoe only when there's no layer tree host yet
> 
> I found another issue after moving the lines into
> DrawingAreaImpl::updatePreferences(). It messes up rendering when not using
> threaded compositor. While
> webPage.corePage()->settings().acceleratedDrawingEnabled() is false in the
> constructor, it is true in DrawingAreaImpl::updatePreferences. So it results
> in making m_alwaysUseCompositing true and calling
> enterAcceleratedCompositingMode(0) even when not using threaded compositor.
> This is because the default value of the acceleratedDrawingEnabled of
> WebPreference is true so that it changes
> settings().acceleratedDrawingEnabled() in WebPage::updatePreferences. I
> assume this is not intended. When not using threaded compositor,
> m_alwaysUseCompositing should be false, right? If so, we move the lines into
> DrawingAreaImpl::updatePreferences, but with a guard
> COORDINATED_GRAPHICS_THREADED. Or we can leave it as is. Any idea?

It seems to me that the check has always been wrong and it actually meant if AC enabled *and* force compositing not *or*, but because it was done before settings were updated, it always used the default values. We don't want to force compositing always when AC is enabled. Maybe we can split this patch since we are fixing too many things in the end :-)

[ChangSeok Oh]

(In reply to comment #14)
> (In reply to comment #13)
> > Comment on attachment 271658 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=271658&action=review
> > 
> > >>>> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:73
> > >>>>  #endif
> > >>> 
> > >>> I think we don't even need this anymore. The drawing area is created by WebPage in its constructor and after that it calls updatePreferences that ends up calling DrawinArea::updatePreferences that already sets this setting.
> > >> 
> > >> Why this line is needed here is because of following lines. If we just remove this line only over leaving below lines. We will see a crash just after launching the mini browser. So I asked how about moving all lines in the constructor into DrawingAreaImpl::updatePreferences(). =)
> > > 
> > > Sounds good to me, I guess we would also need to enter AC mdoe only when there's no layer tree host yet
> > 
> > I found another issue after moving the lines into
> > DrawingAreaImpl::updatePreferences(). It messes up rendering when not using
> > threaded compositor. While
> > webPage.corePage()->settings().acceleratedDrawingEnabled() is false in the
> > constructor, it is true in DrawingAreaImpl::updatePreferences. So it results
> > in making m_alwaysUseCompositing true and calling
> > enterAcceleratedCompositingMode(0) even when not using threaded compositor.
> > This is because the default value of the acceleratedDrawingEnabled of
> > WebPreference is true so that it changes
> > settings().acceleratedDrawingEnabled() in WebPage::updatePreferences. I
> > assume this is not intended. When not using threaded compositor,
> > m_alwaysUseCompositing should be false, right? If so, we move the lines into
> > DrawingAreaImpl::updatePreferences, but with a guard
> > COORDINATED_GRAPHICS_THREADED. Or we can leave it as is. Any idea?
> 
> It seems to me that the check has always been wrong and it actually meant if
> AC enabled *and* force compositing not *or*, but because it was done before
> settings were updated, it always used the default values. We don't want to
> force compositing always when AC is enabled. Maybe we can split this patch
> since we are fixing too many things in the end :-)

You are right. Let's go step by step.

[ChangSeok Oh]

Created attachment 271662 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 271662 [details]
Patch

Clearing flags on attachment: 271662

Committed r196767: <http://trac.webkit.org/changeset/196767>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.