Bug 121079 - structure of frame tree is not correct in WINCE port.
Summary: structure of frame tree is not correct in WINCE port.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-09 23:26 PDT by Zhuang Zhigang
Modified: 2013-11-04 16:45 PST (History)
4 users (show)

See Also:


Attachments
Change structure of frame tree in WINCE port. (3.68 KB, patch)
2013-09-24 06:25 PDT, Zhuang Zhigang
bfulgham: review-
Details | Formatted Diff | Diff
A new patch (3.62 KB, patch)
2013-10-31 18:02 PDT, Zhuang Zhigang
paroga: review-
Details | Formatted Diff | Diff
updated patch (3.07 KB, patch)
2013-11-01 02:35 PDT, Zhuang Zhigang
darin: review+
Details | Formatted Diff | Diff
updated patch according to opinion of review results (3.65 KB, patch)
2013-11-03 20:12 PST, Zhuang Zhigang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhuang Zhigang 2013-09-09 23:26:56 PDT
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.
Comment 1 Patrick R. Gansterer 2013-09-22 13:06:14 PDT
Can you post a patch for that to this bug? See http://www.webkit.org/coding/contributing.html
Comment 2 Zan Dobersek 2013-09-22 22:42:48 PDT
This might have regressed with bugs #119964 and #120092.
Comment 3 Zhuang Zhigang 2013-09-24 06:25:43 PDT
Created attachment 212460 [details]
Change structure of frame tree in WINCE port.
Comment 4 Patrick R. Gansterer 2013-10-31 01:45:07 PDT
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.
Comment 5 Brent Fulgham 2013-10-31 09:45:58 PDT
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!
Comment 6 Brent Fulgham 2013-10-31 09:52:27 PDT
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.
Comment 7 Zhuang Zhigang 2013-10-31 18:02:42 PDT
Created attachment 215704 [details]
A new patch
Comment 8 Zhuang Zhigang 2013-10-31 18:04:31 PDT
(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.
Comment 9 Patrick R. Gansterer 2013-11-01 01:18:58 PDT
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.
Comment 10 Zhuang Zhigang 2013-11-01 02:33:29 PDT
(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.
Comment 11 Zhuang Zhigang 2013-11-01 02:35:09 PDT
Created attachment 215725 [details]
updated patch
Comment 12 Patrick R. Gansterer 2013-11-01 03:33:51 PDT
(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.
Comment 13 Darin Adler 2013-11-01 11:49:54 PDT
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.
Comment 14 Zhuang Zhigang 2013-11-03 20:12:30 PST
Created attachment 215886 [details]
updated patch according to opinion of review results
Comment 15 Zhuang Zhigang 2013-11-03 20:21:08 PST
(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.
Comment 16 WebKit Commit Bot 2013-11-04 16:45:01 PST
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>
Comment 17 WebKit Commit Bot 2013-11-04 16:45:03 PST
All reviewed patches have been landed.  Closing bug.