Bug 152048 – [CoordinatedGraphics][EFL] Fix unhandled web process message when launching MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

When launched MiniBrowser, there is below error message.

ERROR: Unhandled web process message 'WebPage:PreferencesDidChange'

It's because EwkView update preferences before initializing WebPage in WebView constructor.

[Ryuan Choi]

Created attachment 266978 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 266978 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL] Fix unhandled web process message when launching MiniBrowser

Please add [CoordinatedGrahics] for more clarity as well.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:334
> +    WKViewSetIsActive(wkView(), true);

If we don't need to initialize view here, it seems to me "WKViewSetIsActive()" is also unnecessary work. I think this work should be done in Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:60 line. Why can't we do it in WebView::WebView() ?

[Ryuan Choi]

(In reply to comment #2)
> Comment on attachment 266978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266978&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL] Fix unhandled web process message when launching MiniBrowser
> 
> Please add [CoordinatedGrahics] for more clarity as well.
> 
OK.

> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:334
> > +    WKViewSetIsActive(wkView(), true);
> 
> If we don't need to initialize view here, it seems to me
> "WKViewSetIsActive()" is also unnecessary work. I think this work should be
> done in Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:60 line.
> Why can't we do it in WebView::WebView() ?

No. WebPage can be initialized without active state in CoordinatedGraphics system although EFL port initializes WebView with active state.

[Gyuyoung Kim]

Comment on attachment 266978 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:334
>>> +    WKViewSetIsActive(wkView(), true);
>> 
>> If we don't need to initialize view here, it seems to me "WKViewSetIsActive()" is also unnecessary work. I think this work should be done in Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:60 line. Why can't we do it in WebView::WebView() ?
> 
> No. WebPage can be initialized without active state in CoordinatedGraphics system although EFL port initializes WebView with active state.

I mean why we should call WKViewSetIsActive() here even though you remove WKViewInitialize(). Your patch delegates the initialization to the WebView ctor. I wonder why we still need to call WKViewSetIsActive() here. Can't we delegate this to the WebView ctor as well ?

[Gyuyoung Kim]

Comment on attachment 266978 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:334
>>>> +    WKViewSetIsActive(wkView(), true);
>>> 
>>> If we don't need to initialize view here, it seems to me "WKViewSetIsActive()" is also unnecessary work. I think this work should be done in Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:60 line. Why can't we do it in WebView::WebView() ?
>> 
>> No. WebPage can be initialized without active state in CoordinatedGraphics system although EFL port initializes WebView with active state.
> 
> I mean why we should call WKViewSetIsActive() here even though you remove WKViewInitialize(). Your patch delegates the initialization to the WebView ctor. I wonder why we still need to call WKViewSetIsActive() here. Can't we delegate this to the WebView ctor as well ?

Though I still don't like to call WKViewSetIsActive() again, I don't object to call it to go ahead to refactor. But please mention why we still have to call WKviewSetIsActive() in ChangeLog.

[Gyuyoung Kim]

rs=me.

[Ryuan Choi]

Created attachment 266990 [details]
Patch

[Ryuan Choi]

(In reply to comment #5)
> Comment on attachment 266978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266978&action=review
> 
> >>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:334
> >>>> +    WKViewSetIsActive(wkView(), true);
> >>> 
> >>> If we don't need to initialize view here, it seems to me "WKViewSetIsActive()" is also unnecessary work. I think this work should be done in Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:60 line. Why can't we do it in WebView::WebView() ?
> >> 
> >> No. WebPage can be initialized without active state in CoordinatedGraphics system although EFL port initializes WebView with active state.
> > 
> > I mean why we should call WKViewSetIsActive() here even though you remove WKViewInitialize(). Your patch delegates the initialization to the WebView ctor. I wonder why we still need to call WKViewSetIsActive() here. Can't we delegate this to the WebView ctor as well ?
> 
> Though I still don't like to call WKViewSetIsActive() again, I don't object
> to call it to go ahead to refactor. But please mention why we still have to
> call WKviewSetIsActive() in ChangeLog.

I don't like it too.
Because of it, we can't make an inactive ewk_view.
Then, how about adding FIXME to move it to some better place in the different bug?

Anyway, I think that initializing ewk_view without active state is different issue from this bug.

[Gyuyoung Kim]

(In reply to comment #8)
> (In reply to comment #5)
> > Comment on attachment 266978 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=266978&action=review
> > 
> > >>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:334
> > >>>> +    WKViewSetIsActive(wkView(), true);
> > >>> 
> > >>> If we don't need to initialize view here, it seems to me "WKViewSetIsActive()" is also unnecessary work. I think this work should be done in Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:60 line. Why can't we do it in WebView::WebView() ?
> > >> 
> > >> No. WebPage can be initialized without active state in CoordinatedGraphics system although EFL port initializes WebView with active state.
> > > 
> > > I mean why we should call WKViewSetIsActive() here even though you remove WKViewInitialize(). Your patch delegates the initialization to the WebView ctor. I wonder why we still need to call WKViewSetIsActive() here. Can't we delegate this to the WebView ctor as well ?
> > 
> > Though I still don't like to call WKViewSetIsActive() again, I don't object
> > to call it to go ahead to refactor. But please mention why we still have to
> > call WKviewSetIsActive() in ChangeLog.
> 
> I don't like it too.
> Because of it, we can't make an inactive ewk_view.
> Then, how about adding FIXME to move it to some better place in the
> different bug?
> 
> Anyway, I think that initializing ewk_view without active state is different
> issue from this bug.

Ok, I see.

[WebKit Commit Bot]

Comment on attachment 266990 [details]
Patch

Clearing flags on attachment: 266990

Committed r193827: <http://trac.webkit.org/changeset/193827>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.