Bug 103229 – [EFL][WK2] Duplicated WebPageGroup initialization

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jinwoo Song]

WebPageGroup does not need to be created in creating EwkView when the pageGroupRef is 0
because the default WebPageGroup(m_defaultPageGroup) is created in WebContext constructor.

As a side effect on the duplicated initialization, pageGroupID increases
even when the EwkView is created with default context.

The issue can be easily detected with the following test case. 
(Even the window name is same, the new window is created.)
http://www.w3schools.com/js/tryit.asp?filename=try_win_name

[Jinwoo Song]

Created attachment 175942 [details]
Patch

[Kangil Han]

LGTM, thanks. :)

[Ryuan Choi]

LGTM too

[WebKit Review Bot]

Comment on attachment 175942 [details]
Patch

Clearing flags on attachment: 175942

Committed r135973: <http://trac.webkit.org/changeset/135973>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 103604

[Jinwoo Song]

ewk_setting API test cases should be fixed to land this patch. (bug 103692)

[Chris Dumez]

Kenneth, could you please take a look at this?

This patch changes the behavior a bit. All the views from the default context will be in the same PageGroup, meaning that they will share the settings.
Previously, we were creating a new PageGroup for each view and therefore, each view had its own settings.

[Kenneth Rohde Christiansen]

Comment on attachment 175942 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:512
>      ASSERT(!smartData->priv);
> -    RefPtr<WebPageGroup> pageGroup = pageGroupRef ? toImpl(pageGroupRef) : WebPageGroup::create();
> -    smartData->priv = new EwkViewImpl(ewkView, context, pageGroup, behavior);
> +    smartData->priv = new EwkViewImpl(ewkView, context, toImpl(pageGroupRef), behavior);
>      return ewkView;

I wonder whether we should add a comment?

[Yael]

(In reply to comment #8)
> Kenneth, could you please take a look at this?
> 
> This patch changes the behavior a bit. All the views from the default context will be in the same PageGroup, meaning that they will share the settings.
> Previously, we were creating a new PageGroup for each view and therefore, each view had its own settings.
That behavior was wrong. If a page creates a named popup, and then tries to create it again with the same name, we were creating a new popup instead of replacing the existing one.

[Jinwoo Song]

(In reply to comment #9)
> (From update of attachment 175942 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175942&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:512
> >      ASSERT(!smartData->priv);
> > -    RefPtr<WebPageGroup> pageGroup = pageGroupRef ? toImpl(pageGroupRef) : WebPageGroup::create();
> > -    smartData->priv = new EwkViewImpl(ewkView, context, pageGroup, behavior);
> > +    smartData->priv = new EwkViewImpl(ewkView, context, toImpl(pageGroupRef), behavior);
> >      return ewkView;
> 
> I wonder whether we should add a comment?

I added the comment here.

[Jinwoo Song]

Created attachment 177086 [details]
patch

Patch with more comments.

[WebKit Review Bot]

Comment on attachment 177086 [details]
patch

Clearing flags on attachment: 177086

Committed r137423: <http://trac.webkit.org/changeset/137423>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.