Bug 112364

Summary: [WK2][EFL] Fix code wrapping WKPageGroupRef
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, gyuyoung.kim, jinwoo7.song, kenneth, kling, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 111543, 111591    
Attachments:
Description Flags
patch
none
patch v2 ap: review+

Mikhail Pozdnyakov
Reported 2013-03-14 10:45:31 PDT
SSIA. EwkPageGroup is just a wrapper around WKPageGroupRef, hence there is no reason in keeping several different EwkPageGroup instances for the same WKPageGroupRef. Secondly EwkPageGroup should take after EwkContext::createOrFindWrapper API to keep consistency.
Attachments
patch (6.57 KB, patch)
2013-03-14 14:14 PDT, Mikhail Pozdnyakov 		no flags
DetailsFormatted DiffDiff
patch v2 (9.51 KB, patch)
2013-03-15 08:09 PDT, Mikhail Pozdnyakov 		ap: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mikhail Pozdnyakov
Comment 1 2013-03-14 14:14:18 PDT
Created attachment 193183 [details] patch
Kenneth Rohde Christiansen
Comment 2 2013-03-14 15:49:38 PDT
Comment on attachment 193183 [details] patch LGTM
Jinwoo Song
Comment 3 2013-03-14 23:18:11 PDT
Looks fine to me, too.
Mikhail Pozdnyakov
Comment 4 2013-03-15 06:00:36 PDT
Comment on attachment 193183 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=193183&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:35 > + RefPtr<EwkPageGroup> pageGroup = pageGroupRef ? EwkPageGroup::findOrCreateWrapper(pageGroupRef) : EwkPageGroup::create(); argh! here is a mistake: We should let page group to be null (at the moment we have the same mistake inside EwkPageGroup::create() :( ).
Mikhail Pozdnyakov
Comment 5 2013-03-15 08:09:27 PDT
Created attachment 193311 [details] patch v2 Fixes also default page group usage problem.
Jinwoo Song
Comment 6 2013-03-15 20:12:36 PDT
(In reply to comment #5) > Created an attachment (id=193311) [details] > patch v2 > > Fixes also default page group usage problem. LGTM. This is more correct way to use the default page group created by WebContext when we pass page group as '0'. My first though was to create the default page group with the 'defaultPageGroupIdentifier' in EwkPageGroup but it may lead to create two WebPageGroup in this case. (one is by WebContext, and the other is by EwkPageGroup::create().)
Kenneth Rohde Christiansen
Comment 7 2013-03-18 02:59:39 PDT
Comment on attachment 193311 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=193311&action=review LGTM > Source/WebKit2/UIProcess/API/efl/EwkView.h:259 > RefPtr<EwkContext> m_context; > + RefPtr<WebKit::WebView> m_webView; > RefPtr<EwkPageGroup> m_pageGroup; > OwnPtr<Evas_GL> m_evasGL; Why is this move not explained in the changelog?
Mikhail Pozdnyakov
Comment 8 2013-03-18 10:04:27 PDT
Committed r146075: <http://trac.webkit.org/changeset/146075>
Note You need to log in before you can comment on or make changes to this bug.