Bug 154067 - [GTK][Threaded Compositor] The web inspector doesn't work when using the threaded compositor
Summary: [GTK][Threaded Compositor] The web inspector doesn't work when using the thre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: ChangSeok Oh
URL:
Keywords: Gtk
Depends on:
Blocks: 154066
  Show dependency treegraph
 
Reported: 2016-02-10 03:13 PST by Carlos Garcia Campos
Modified: 2016-02-18 11:06 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.75 KB, patch)
2016-02-17 23:57 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (4.02 KB, patch)
2016-02-18 06:11 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (851.23 KB, application/zip)
2016-02-18 07:08 PST, Build Bot
no flags Details
Patch (2.54 KB, patch)
2016-02-18 08:46 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-02-10 03:13:43 PST
Try to open the inspector in MiniBrowser and see that it's not rendered at all.
Comment 1 ChangSeok Oh 2016-02-15 22:52:18 PST
Let me pick this up.
Comment 2 ChangSeok Oh 2016-02-17 23:57:29 PST
Created attachment 271640 [details]
Patch
Comment 3 Carlos Garcia Campos 2016-02-18 00:49:11 PST
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().
Comment 4 ChangSeok Oh 2016-02-18 06:11:45 PST
Created attachment 271658 [details]
Patch
Comment 5 WebKit Commit Bot 2016-02-18 06:14:31 PST
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
Comment 6 ChangSeok Oh 2016-02-18 06:22:30 PST
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?
Comment 7 Carlos Garcia Campos 2016-02-18 06:24:44 PST
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.
Comment 8 Carlos Garcia Campos 2016-02-18 06:36:19 PST
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.
Comment 9 ChangSeok Oh 2016-02-18 06:36:34 PST
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(). =)
Comment 10 Carlos Garcia Campos 2016-02-18 06:40:27 PST
(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
Comment 11 Build Bot 2016-02-18 07:08:35 PST
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
Comment 12 Build Bot 2016-02-18 07:08:40 PST
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
Comment 13 ChangSeok Oh 2016-02-18 07:53:14 PST
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?
Comment 14 Carlos Garcia Campos 2016-02-18 08:17:08 PST
(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 :-)
Comment 15 ChangSeok Oh 2016-02-18 08:38:35 PST
(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.
Comment 16 ChangSeok Oh 2016-02-18 08:46:01 PST
Created attachment 271662 [details]
Patch
Comment 17 WebKit Commit Bot 2016-02-18 11:06:49 PST
Comment on attachment 271662 [details]
Patch

Clearing flags on attachment: 271662

Committed r196767: <http://trac.webkit.org/changeset/196767>
Comment 18 WebKit Commit Bot 2016-02-18 11:06:59 PST
All reviewed patches have been landed.  Closing bug.