Bug 121079 – structure of frame tree is not correct in WINCE port.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Zhuang Zhigang]

We are working on WINCE port.When we open some contents that contain frames, sometimes we can't get expected results.
We checked the code of WebView in WINCE port. We think the method of WebView::createFrame is not correct.
The frame that used as parent frame is the main frame. So, when a child frame is created, it is created as child of main frame. In the end, all the children frames (even children of children) are created as child of main frame.

This can be resolved by passing the correct parent frame from FrameLoaderClientWinCE::createFrame to WebView::createFrame.

[Patrick R. Gansterer]

Can you post a patch for that to this bug? See http://www.webkit.org/coding/contributing.html

[Zan Dobersek]

This might have regressed with bugs #119964 and #120092.

[Zhuang Zhigang]

Created attachment 212460 [details]
Change structure of frame tree in WINCE port.

[Patrick R. Gansterer]

Comment on attachment 212460 [details]
Change structure of frame tree in WINCE port.

Please set the r? flag to get the patch reviewed and into the tree.

[Brent Fulgham]

This patch seems to have a bunch of improper line-endings.  Can you reupload the patch with matching EOL encoding as the source files in the repository.

The EWS bots cannot apply the patch, and so we cannot confirm that there are no regressions when adding this to the archive.

Thanks!

[Brent Fulgham]

Comment on attachment 212460 [details]
Change structure of frame tree in WINCE port.

This patch looks fine to me, but it cannot be applied by our bot system. I'm marking it r- to correct the line-endings, otherwise it looks good.

[Zhuang Zhigang]

Created attachment 215704 [details]
A new patch

[Zhuang Zhigang]

(In reply to comment #5)
> This patch seems to have a bunch of improper line-endings.  Can you reupload the patch with matching EOL encoding as the source files in the repository.
> 
> The EWS bots cannot apply the patch, and so we cannot confirm that there are no regressions when adding this to the archive.
> 
> Thanks!
A new patch was uploaded, and I have checked it with Tools/Scripts/check-webkit-style. Please check it.

[Patrick R. Gansterer]

Comment on attachment 215704 [details]
A new patch

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

> Source/WebKit/wince/WebView.cpp:161
> +    Frame* parentFrame = frame;

Why do this additional assignment? Just rename the parameter name of the function.

Otherwise looks ok to me.

[Zhuang Zhigang]

(In reply to comment #9)
> (From update of attachment 215704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215704&action=review
> 
> > Source/WebKit/wince/WebView.cpp:161
> > +    Frame* parentFrame = frame;
> 
> Why do this additional assignment? Just rename the parameter name of the function.
> 
> Otherwise looks ok to me.
I changed the name because I think parentFrame is a suitable name for this variable. Anyway I will change it back and upload a new patch.

[Zhuang Zhigang]

Created attachment 215725 [details]
updated patch

[Patrick R. Gansterer]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 215704 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=215704&action=review
> > 
> > > Source/WebKit/wince/WebView.cpp:161
> > > +    Frame* parentFrame = frame;
> > 
> > Why do this additional assignment? Just rename the parameter name of the function.
> > 
> > Otherwise looks ok to me.
> I changed the name because I think parentFrame is a suitable name for this variable. Anyway I will change it back and upload a new patch.

Renaming it to parentFrame is ok, but the assignment of "parentFrame = frame" is not necessary. Just rename frame (last parameter of the method) to parentFrame.

[Darin Adler]

Comment on attachment 215725 [details]
updated patch

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

> Source/WebKit/wince/WebView.cpp:159
>  PassRefPtr<Frame> WebView::createFrame(const URL& url, const String& name, HTMLFrameOwnerElement* ownerElement, const String& referrer,
> -                                       bool /*allowsScrolling*/, int /*marginWidth*/, int /*marginHeight*/)
> +    bool /*allowsScrolling*/, int /*marginWidth*/, int /*marginHeight*/, Frame* frame)

Argument should be named parentFrame.

> Source/WebKit/wince/WebView.h:72
> +    PassRefPtr<WebCore::Frame> createFrame(const WebCore::URL&, const WTF::String&, WebCore::HTMLFrameOwnerElement*, const WTF::String&, bool, int, int, WebCore::Frame*);

Should give the arguments names here, except the ones that are obvious from the type along. The meanings of the strings, bool, ints, and frame are not obvious.

[Zhuang Zhigang]

Created attachment 215886 [details]
updated patch according to opinion of review results

[Zhuang Zhigang]

(In reply to comment #13)
> (From update of attachment 215725 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215725&action=review
> 
> > Source/WebKit/wince/WebView.cpp:159
> >  PassRefPtr<Frame> WebView::createFrame(const URL& url, const String& name, HTMLFrameOwnerElement* ownerElement, const String& referrer,
> > -                                       bool /*allowsScrolling*/, int /*marginWidth*/, int /*marginHeight*/)
> > +    bool /*allowsScrolling*/, int /*marginWidth*/, int /*marginHeight*/, Frame* frame)
> 
> Argument should be named parentFrame.
> 
Yes, I see.
Argument name was changed.
Then removed assignment for "coreFrame" and changed remaining of "coreFrame" to "parentFrame".

> > Source/WebKit/wince/WebView.h:72
> > +    PassRefPtr<WebCore::Frame> createFrame(const WebCore::URL&, const WTF::String&, WebCore::HTMLFrameOwnerElement*, const WTF::String&, bool, int, int, WebCore::Frame*);
> 
> Should give the arguments names here, except the ones that are obvious from the type along. The meanings of the strings, bool, ints, and frame are not obvious.
OK, I added the arguments names for these strings, bool, ints, and frame.

[WebKit Commit Bot]

Comment on attachment 215886 [details]
updated patch according to opinion of review results

Clearing flags on attachment: 215886

Committed r158616: <http://trac.webkit.org/changeset/158616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.