Bug 70507 - [Chromium] Refactor WebFrameImpl::createFrameView() to use Frame:createView
Summary: [Chromium] Refactor WebFrameImpl::createFrameView() to use Frame:createView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
: 70555 (view as bug list)
Depends on:
Blocks: 70559 70940
  Show dependency treegraph
 
Reported: 2011-10-20 09:11 PDT by Fady Samuel
Modified: 2011-11-03 12:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.39 KB, patch)
2011-10-25 09:19 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (3.39 KB, patch)
2011-10-25 09:52 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (3.40 KB, patch)
2011-11-02 11:28 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (20.50 KB, patch)
2011-11-03 12:11 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 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.
Comment 1 Darin Fisher (:fishd, Google) 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
Comment 2 Fady Samuel 2011-10-25 09:19:17 PDT
Created attachment 112345 [details]
Patch
Comment 3 Fady Samuel 2011-10-25 09:52:59 PDT
Created attachment 112348 [details]
Patch
Comment 4 Fady Samuel 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.
Comment 5 Darin Fisher (:fishd, Google) 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.
Comment 6 Fady Samuel 2011-11-02 11:28:47 PDT
Created attachment 113341 [details]
Patch
Comment 7 Fady Samuel 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.
Comment 8 Fady Samuel 2011-11-03 08:38:17 PDT
*** Bug 70555 has been marked as a duplicate of this bug. ***
Comment 9 Fady Samuel 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>
Comment 10 Fady Samuel 2011-11-03 10:07:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Adrienne Walker 2011-11-03 12:11:29 PDT
Created attachment 113537 [details]
Patch
Comment 12 Adrienne Walker 2011-11-03 12:12:48 PDT
Comment on attachment 113537 [details]
Patch

Oops.  Sorry.  Copied a bug line, but forgot to edit it.