Bug 70507

Summary: [Chromium] Refactor WebFrameImpl::createFrameView() to use Frame:createView
Product: WebKit Reporter: Fady Samuel <fsamuel>
Component: Layout and RenderingAssignee: Fady Samuel <fsamuel>
Status: RESOLVED FIXED    
Severity: Normal CC: fishd, fsamuel, rjkroege
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 70559, 70940    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Fady Samuel
Reported 2011-10-20 09:11:57 PDT
These two pieces of code seem to have a lot in common. It's probably a good idea to use Frame::createView to avoid needlessly diverging in behavior over time resulting in port specific bugs.
Attachments
Patch (3.39 KB, patch)
2011-10-25 09:19 PDT, Fady Samuel 		no flags
DetailsFormatted DiffDiff
Patch (3.39 KB, patch)
2011-10-25 09:52 PDT, Fady Samuel 		no flags
DetailsFormatted DiffDiff
Patch (3.40 KB, patch)
2011-11-02 11:28 PDT, Fady Samuel 		no flags
DetailsFormatted DiffDiff
Patch (20.50 KB, patch)
2011-11-03 12:11 PDT, Adrienne Walker 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Darin Fisher (:fishd, Google)
Comment 1 2011-10-20 22:08:58 PDT
Yes, please! I never noticed when Frame::createView was added. Wow, 3 years ago: http://trac.webkit.org/changeset/40435
Fady Samuel
Comment 2 2011-10-25 09:19:17 PDT
Created attachment 112345 [details] Patch
Fady Samuel
Comment 3 2011-10-25 09:52:59 PDT
Created attachment 112348 [details] Patch
Fady Samuel
Comment 4 2011-10-26 11:26:59 PDT
Please note: Fixed layout mode probably needs to be disabled for subframes or else you end up with scrollbars on iframe content that was meant to fit the the iframe. How the viewport meta tag and fixed layout should interact with frames and iframes is a complex question, and I'll leave that for another bug report.
Darin Fisher (:fishd, Google)
Comment 5 2011-11-02 10:52:32 PDT
Comment on attachment 112348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112348&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:-2023 > - if (webView->isTransparent()) It looks like you are losing this step, which I think matters for chrome extensions.
Fady Samuel
Comment 6 2011-11-02 11:28:47 PDT
Created attachment 113341 [details] Patch
Fady Samuel
Comment 7 2011-11-02 13:21:25 PDT
Comment on attachment 112348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112348&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:-2023 >> - if (webView->isTransparent()) > > It looks like you are losing this step, which I think matters for chrome extensions. Oops. I totally missed those two lines. Thanks! I've uploaded a new patch to fix this.
Fady Samuel
Comment 8 2011-11-03 08:38:17 PDT
*** Bug 70555 has been marked as a duplicate of this bug. ***
Fady Samuel
Comment 9 2011-11-03 10:07:10 PDT
Comment on attachment 113341 [details] Patch Clearing flags on attachment: 113341 Committed r99208: <http://trac.webkit.org/changeset/99208>
Fady Samuel
Comment 10 2011-11-03 10:07:16 PDT
All reviewed patches have been landed. Closing bug.
Adrienne Walker
Comment 11 2011-11-03 12:11:29 PDT
Created attachment 113537 [details] Patch
Adrienne Walker
Comment 12 2011-11-03 12:12:48 PDT
Comment on attachment 113537 [details] Patch Oops. Sorry. Copied a bug line, but forgot to edit it.
Note You need to log in before you can comment on or make changes to this bug.