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.
Can you post a patch for that to this bug? See http://www.webkit.org/coding/contributing.html
This might have regressed with bugs #119964 and #120092.
Created attachment 212460 [details] Change structure of frame tree in WINCE port.
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.
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 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.
Created attachment 215704 [details] A new patch
(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 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.
(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.
Created attachment 215725 [details] updated patch
(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 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.
Created attachment 215886 [details] updated patch according to opinion of review results
(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 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>
All reviewed patches have been landed. Closing bug.