Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7831028:
Compute pageScaleFactor on page so that fixed layout page fits width of window. (Closed)
|
Created:
9 years, 3 months ago by Fady Samuel Modified:
8 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, varunjain, klobag.chromium Visibility:
Public. |
DescriptionCompute pageScaleFactor on page so that fixed layout page fits width of window.
Fixed layout mode can now be enabled with the command line
flag --enable-fixed-layout.
This mode does not interact well with zoom at this point in time. This will be fixed shortly.
This bug replaces http://codereview.chromium.org/7764006/ which is no longer relevant.
BUG=none
TEST=manually
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123126
Patch Set 1 #
Total comments: 12
Patch Set 2 : Updated according to jamesr and jam #Patch Set 3 : Can only be enabled for TOUCH_UI builds. #Patch Set 4 : Implement Viewport and Fixed Layout #Patch Set 5 : Compile time flag to enable viewport tag. Runtime flag to enable fixed layout. #Patch Set 6 : Refreshed patch to fix position:fixed elements #Patch Set 7 : Small fix (workaround) to work on Aura #
Total comments: 1
Patch Set 8 : Removed unnecsssary changes in build/common.gypi #
Total comments: 10
Patch Set 9 : Fixed based on comments (missing fishd's first comment) #Patch Set 10 : Minimizes render_view_impl changes and introduces a defaultDeviceScaleFactor argument #
Total comments: 9
Patch Set 11 : Updated according to Darin #Patch Set 12 : Updated according to discussion with rjkroege #
Total comments: 6
Patch Set 13 : Fixed some comments according to rjkroege #
Total comments: 1
Patch Set 14 : Removed unnecessary getter #Patch Set 15 : Fixed common.gypi to have default value for enable_viewport #
Messages
Total messages: 40 (0 generated)
I'm only able to give superficial review comments. someone more familiar with this should review. James: you perhaps? http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:4060: ? http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h File content/renderer/render_view.h (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h#... content/renderer/render_view.h:949: // Keeps Keeps the anchor (in window coordinates) in a stable position on the nit Keeps Keeps http://codereview.chromium.org/7831028/diff/1/content/renderer/render_widget.h File content/renderer/render_widget.h (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_widget.... content/renderer/render_widget.h:250: virtual gfx::Size GetContentSize(); what's the point of adding these virtual methods to RenderWidget if they're only useful for RenderView, and only RenderView calls these methods? http://codereview.chromium.org/7831028/diff/1/content/renderer/render_widget.... content/renderer/render_widget.h:257: ?
Is there a test plan for this? Should any of this be guarded by TOUCH_UI macros? http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1241: frame_url.SchemeIs(chrome::kHttpsScheme)) { I really doubt this is the best way to test for web content, what about extension pages or file:// URLs? http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h File content/renderer/render_view.h (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h#... content/renderer/render_view.h:1216: float maximum_scale_; why are these member variables? from what I can tell they are always set to the defaults
On 2011/09/02 17:43:09, jamesr wrote: > Is there a test plan for this? > > Should any of this be guarded by TOUCH_UI macros? > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc > File content/renderer/render_view.cc (right): > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc... > content/renderer/render_view.cc:1241: frame_url.SchemeIs(chrome::kHttpsScheme)) > { > I really doubt this is the best way to test for web content, what about > extension pages or file:// URLs? > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h > File content/renderer/render_view.h (right): > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h#... > content/renderer/render_view.h:1216: float maximum_scale_; > why are these member variables? from what I can tell they are always set to the > defaults The major concern I have about testing at the browser-level (according to John, a renderer testing framework is not yet ready, am I understanding this correctly?), is that it'll capture bugs as well. There are still a number of scaling related bugs/functionality in WebKit that need to be addressed before this feature is ready for prime time, regardless of what I do in the browser/renderer. So my concern is tests will break as fixes make their way into webkit. How about this for a test plan: #1 I add fixed layout support to DumpRenderTree #2 I add some layout tests that test fixed layout mode in webkit (for chromium). #3 I clean up this patch according to suggestions here but leave larger scale testing when this feature is a bit more mature. Does this sound reasonable? Thanks.
On 2011/09/07 14:17:19, Fady Samuel wrote: > On 2011/09/02 17:43:09, jamesr wrote: > > Is there a test plan for this? > > > > Should any of this be guarded by TOUCH_UI macros? > > > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc > > File content/renderer/render_view.cc (right): > > > > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc... > > content/renderer/render_view.cc:1241: > frame_url.SchemeIs(chrome::kHttpsScheme)) > > { > > I really doubt this is the best way to test for web content, what about > > extension pages or file:// URLs? > > > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h > > File content/renderer/render_view.h (right): > > > > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h#... > > content/renderer/render_view.h:1216: float maximum_scale_; > > why are these member variables? from what I can tell they are always set to > the > > defaults > > The major concern I have about testing at the browser-level (according to John, > a renderer testing framework is not yet ready, am I understanding this > correctly?), To be clear: we don't have browser tests running against a browser built on content only. But we do have browser_tests which test chrome, and that's where we have all of our integration tests now. > is that it'll capture bugs as well. There are still a number of > scaling related bugs/functionality in WebKit that need to be addressed before > this feature is ready for prime time, regardless of what I do in the > browser/renderer. So my concern is tests will break as fixes make their way into > webkit. > > How about this for a test plan: > > #1 I add fixed layout support to DumpRenderTree > #2 I add some layout tests that test fixed layout mode in webkit (for chromium). > #3 I clean up this patch according to suggestions here but leave larger scale > testing when this feature is a bit more mature. > > Does this sound reasonable? > > Thanks.
FYI, my intention is to test fixed layout in WebKit. Link: https://bugs.webkit.org/show_bug.cgi?id=67723
On 2011/09/07 18:55:58, Fady Samuel wrote: > FYI, my intention is to test fixed layout in WebKit. > > Link: > > https://bugs.webkit.org/show_bug.cgi?id=67723 Patch for Bug 67723 has landed in WebKit so testing for fixed layout will be done through webkit layout tests. I will address the other comments now however.
http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1241: frame_url.SchemeIs(chrome::kHttpsScheme)) { On 2011/09/02 17:43:09, jamesr wrote: > I really doubt this is the best way to test for web content, what about > extension pages or file:// URLs? This is intentional. WebUI is broken with fixed layout currently. Fixing this will take a while so this "hack" is in place until (if) we decide to fix WebUI. http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h File content/renderer/render_view.h (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h#... content/renderer/render_view.h:949: // Keeps Keeps the anchor (in window coordinates) in a stable position on the On 2011/09/02 17:11:41, John Abd-El-Malek wrote: > nit Keeps Keeps Done. http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.h#... content/renderer/render_view.h:1216: float maximum_scale_; On 2011/09/02 17:43:09, jamesr wrote: > why are these member variables? from what I can tell they are always set to the > defaults These will be used in an upcoming patch once the viewport meta tag is enabled. http://codereview.chromium.org/7831028/diff/1/content/renderer/render_widget.h File content/renderer/render_widget.h (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_widget.... content/renderer/render_widget.h:250: virtual gfx::Size GetContentSize(); On 2011/09/02 17:11:41, John Abd-El-Malek wrote: > what's the point of adding these virtual methods to RenderWidget if they're only > useful for RenderView, and only RenderView calls these methods? Done. http://codereview.chromium.org/7831028/diff/1/content/renderer/render_widget.... content/renderer/render_widget.h:257: On 2011/09/02 17:11:41, John Abd-El-Malek wrote: > ? Done.
http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:1241: frame_url.SchemeIs(chrome::kHttpsScheme)) { On 2011/09/08 23:46:39, Fady Samuel wrote: > On 2011/09/02 17:43:09, jamesr wrote: > > I really doubt this is the best way to test for web content, what about > > extension pages or file:// URLs? > > This is intentional. WebUI is broken with fixed layout currently. Fixing this > will take a while so this "hack" is in place until (if) we decide to fix WebUI. Good grief! That is horrible. What is wrong with WebUI? Is it the actual content of WebUI pages that is broken (as in the html/css we use on these pages) or something else? If it is just the html/css can't you fix it by proper use of viewport tags? If you really want to exclude WebUI, then do it by checking for the WebUI scheme explicitly, file a bug, and reference the bug in a comment. This code also breaks chrome extension pages, file:// urls, etc etc. I'm having serious doubts about this codepath if it can't handle WebUI....
On 2011/09/09 00:11:40, jamesr wrote: > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc > File content/renderer/render_view.cc (right): > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc... > content/renderer/render_view.cc:1241: frame_url.SchemeIs(chrome::kHttpsScheme)) > { > On 2011/09/08 23:46:39, Fady Samuel wrote: > > On 2011/09/02 17:43:09, jamesr wrote: > > > I really doubt this is the best way to test for web content, what about > > > extension pages or file:// URLs? > > > > This is intentional. WebUI is broken with fixed layout currently. Fixing this > > will take a while so this "hack" is in place until (if) we decide to fix > WebUI. > > Good grief! That is horrible. What is wrong with WebUI? Is it the actual > content of WebUI pages that is broken (as in the html/css we use on these pages) > or something else? If it is just the html/css can't you fix it by proper use of > viewport tags? > > If you really want to exclude WebUI, then do it by checking for the WebUI scheme > explicitly, file a bug, and reference the bug in a comment. This code also > breaks chrome extension pages, file:// urls, etc etc. > > I'm having serious doubts about this codepath if it can't handle WebUI.... Hi James, Yes, the issue is WebUI needs the viewport meta tag to look right. Also, the TOUCHUI soft keyboard is not laid out correctly without the viewport meta tag. The viewport tag has not yet been upstreamed for Chromium. I'm in the process of getting that working. I'll upload a work-in-progress patch tonight to bugzilla and link it here. There are still some bugs for that tag that I'm working out locally but I'll keep updating bugzilla so that you and others can monitor my progress. Does that sound ok to you?
On 2011/09/09 18:21:13, Fady Samuel wrote: > On 2011/09/09 00:11:40, jamesr wrote: > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc > > File content/renderer/render_view.cc (right): > > > > > http://codereview.chromium.org/7831028/diff/1/content/renderer/render_view.cc... > > content/renderer/render_view.cc:1241: > frame_url.SchemeIs(chrome::kHttpsScheme)) > > { > > On 2011/09/08 23:46:39, Fady Samuel wrote: > > > On 2011/09/02 17:43:09, jamesr wrote: > > > > I really doubt this is the best way to test for web content, what about > > > > extension pages or file:// URLs? > > > > > > This is intentional. WebUI is broken with fixed layout currently. Fixing > this > > > will take a while so this "hack" is in place until (if) we decide to fix > > WebUI. > > > > Good grief! That is horrible. What is wrong with WebUI? Is it the actual > > content of WebUI pages that is broken (as in the html/css we use on these > pages) > > or something else? If it is just the html/css can't you fix it by proper use > of > > viewport tags? > > > > If you really want to exclude WebUI, then do it by checking for the WebUI > scheme > > explicitly, file a bug, and reference the bug in a comment. This code also > > breaks chrome extension pages, file:// urls, etc etc. > > > > I'm having serious doubts about this codepath if it can't handle WebUI.... > > Hi James, > > Yes, the issue is WebUI needs the viewport meta tag to look right. Also, the > TOUCHUI soft keyboard is not laid out correctly without the viewport meta tag. > > The viewport tag has not yet been upstreamed for Chromium. I'm in the process of > getting that working. I'll upload a work-in-progress patch tonight to bugzilla > and link it here. There are still some bugs for that tag that I'm working out > locally but I'll keep updating bugzilla so that you and others can monitor my > progress. Does that sound ok to you? It is now only possible to enable this feature if it's a TOUCH_UI.
James, could you please advise me as to what kind of testing you'd like to see? I've already introduced a mechanism to test fixed layout in WebKit, and included a basic layout test.
James, I'd like to get unblocked on this because this is a prerequisite to a whole bunch of changes that I'm queueing up locally. I'd appreciate discussing this over VC or figuring out what types of tests must be done to clear this code for upstreaming. Thanks. On 2011/09/16 13:19:07, fsamuel wrote: > James, could you please advise me as to what kind of testing you'd like to see? > I've already introduced a mechanism to test fixed layout in WebKit, and included > a basic layout test.
On Tue, Oct 11, 2011 at 3:54 PM, <fsamuel@google.com> wrote: > James, I'd like to get unblocked on this because this is a prerequisite to > a > whole bunch of changes that I'm queueing up locally. I'd appreciate > discussing > this over VC or figuring out what types of tests must be done to clear this > code > for upstreaming. Thanks. > I have two pieces of feedback about this patch: 1.) If you are adding logic (especially to shared code), it should have tests. I don't know what the appropriate way to test this sort of code is but I know it needs something - right now, the logic this adds to render_view/render_widget is not tested at all (the layout tests don't use this codepath). Otherwise, it's highly likely that this code will break. 2.) the http/https check is clearly wrong and not acceptable to land Beyond that I don't have much insight on the patch itself. - James > On 2011/09/16 13:19:07, fsamuel wrote: > >> James, could you please advise me as to what kind of testing you'd like to >> > see? > >> I've already introduced a mechanism to test fixed layout in WebKit, and >> > included > >> a basic layout test. >> > > > > http://codereview.chromium.**org/7831028/<http://codereview.chromium.org/7831... >
1. I agree about the testing issue but I don't know how to test this. How is renderer code usually tested? I'm just unfamiliar with testing in this part of chromium. 2. I believe the issue here is complicated. WebUI is broken because it needs a viewport meta tag but the viewport meatag implementation needs this code (so we have a chicken and the egg situation here). Extensions must not use this code path by default because existing extensions assume they'll fit the window they're in and so if we enable fixed layout on existing extensions, they will break. Fady On Tue, Oct 11, 2011 at 7:21 PM, James Robinson <jamesr@google.com> wrote: > On Tue, Oct 11, 2011 at 3:54 PM, <fsamuel@google.com> wrote: > >> James, I'd like to get unblocked on this because this is a prerequisite to >> a >> whole bunch of changes that I'm queueing up locally. I'd appreciate >> discussing >> this over VC or figuring out what types of tests must be done to clear >> this code >> for upstreaming. Thanks. >> > > I have two pieces of feedback about this patch: > > 1.) If you are adding logic (especially to shared code), it should have > tests. I don't know what the appropriate way to test this sort of code is > but I know it needs something - right now, the logic this adds to > render_view/render_widget is not tested at all (the layout tests don't use > this codepath). Otherwise, it's highly likely that this code will break. > 2.) the http/https check is clearly wrong and not acceptable to land > > Beyond that I don't have much insight on the patch itself. > > - James > > > >> On 2011/09/16 13:19:07, fsamuel wrote: >> >>> James, could you please advise me as to what kind of testing you'd like >>> to >>> >> see? >> >>> I've already introduced a mechanism to test fixed layout in WebKit, and >>> >> included >> >>> a basic layout test. >>> >> >> >> >> http://codereview.chromium.**org/7831028/<http://codereview.chromium.org/7831... >> > >
On 2011/10/12 17:58:17, fsamuel wrote: > 1. I agree about the testing issue but I don't know how to test this. How is > renderer code usually tested? I'm just unfamiliar with testing in this part > of chromium. > > 2. I believe the issue here is complicated. WebUI is broken because it needs > a viewport meta tag but the viewport meatag implementation needs this code > (so we have a chicken and the egg situation here). Extensions must not use > this code path by default because existing extensions assume they'll fit the > window they're in and so if we enable fixed layout on existing extensions, > they will break. > > Fady > > On Tue, Oct 11, 2011 at 7:21 PM, James Robinson <mailto:jamesr@google.com> wrote: > > > On Tue, Oct 11, 2011 at 3:54 PM, <mailto:fsamuel@google.com> wrote: > > > >> James, I'd like to get unblocked on this because this is a prerequisite to > >> a > >> whole bunch of changes that I'm queueing up locally. I'd appreciate > >> discussing > >> this over VC or figuring out what types of tests must be done to clear > >> this code > >> for upstreaming. Thanks. > >> > > > > I have two pieces of feedback about this patch: > > > > 1.) If you are adding logic (especially to shared code), it should have > > tests. I don't know what the appropriate way to test this sort of code is > > but I know it needs something - right now, the logic this adds to > > render_view/render_widget is not tested at all (the layout tests don't use > > this codepath). Otherwise, it's highly likely that this code will break. > > 2.) the http/https check is clearly wrong and not acceptable to land > > > > Beyond that I don't have much insight on the patch itself. > > > > - James > > > > > > > >> On 2011/09/16 13:19:07, fsamuel wrote: > >> > >>> James, could you please advise me as to what kind of testing you'd like > >>> to > >>> > >> see? > >> > >>> I've already introduced a mechanism to test fixed layout in WebKit, and > >>> > >> included > >> > >>> a basic layout test. > >>> > >> > >> > >> > >> > http://codereview.chromium.**org/7831028/%3Chttp://codereview.chromium.org/78...> > >> > > > > I've resumed work on this on both the chromium and webkit side. Will continue work on the various renderer-side changes when various discussions reach a conclusion. The master webkit bug can be found here: https://bugs.webkit.org/show_bug.cgi?id=70559
Now that most of the viewport logic has been moved into webkit, there's very little that needs to be done on the chromium side, except disable fixed layout if the view is not tab contents. This might need some additional iteration but I think this should do. Darin, could you please look at the gyp file. This basically makes it possible to specify a compile time flag to build with viewport code in WebKit. Aaron, the code to enable fixed layout has been moved to ChromeRenderViewObserver and uses the extension helper to decide whether the view is tab contents or not.
http://codereview.chromium.org/7831028/diff/36001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/7831028/diff/36001/build/common.gypi#newcode49 build/common.gypi:49: 'enable_viewport%': 0, I may not be the best reviewer for this change. It is conspicuous that no other enable_XXX options are expressed here. Why does this need to be handled differently than other WebKit ENABLE_XXX options?
http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_view_observer.cc (right): http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:381: ExtensionHelper* extension_helper = ExtensionHelper::Get(render_view()); What are you trying to do here? It looks to me like ExtensionHelper exists for all render views, so this code will run for all tab contents - web or extension. Is that what you want?
On 2012/01/20 18:49:23, Aaron Boodman wrote: > http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... > File chrome/renderer/chrome_render_view_observer.cc (right): > > http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... > chrome/renderer/chrome_render_view_observer.cc:381: ExtensionHelper* > extension_helper = ExtensionHelper::Get(render_view()); > What are you trying to do here? It looks to me like ExtensionHelper exists for > all render views, so this code will run for all tab contents - web or extension. > > Is that what you want? Per offline discussion, this is what we agreed to. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/7831028/40001
Presubmit check for 7831028-40001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/browser/renderer_host/render_process_host_impl.cc,content/public/common/content_switches.cc,content/public/common/content_switches.h,content/browser/renderer_host/render_widget_host_view_aura.cc Presubmit checks took 1.8s to calculate.
http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_view_observer.cc (right): http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:383: extension_helper->view_type() == content::VIEW_TYPE_TAB_CONTENTS; it seems a little hacky to be using something from the extension system for this. it seems like you are trying to test if the RenderView belongs to a TabContents. surely, there must be or could be a more direct way of determining that via the Content API. if not, then perhaps we should add one. it seems wrong to entangle the extension system here when this code really has nothing to do with extensions. http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:387: WebSize viewSize = render_view()->GetWebView()->size(); nit: view_size http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:388: int layoutHeight = nit: layout_height http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:396: setShouldLayoutFixedElementsRelativeToFrame(true); isn't this done through WebSettings now?
Added jam to address a comment Darin made. I'm not sure if there is a way to access the container type from the content API currently? http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_view_observer.cc (right): http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:383: extension_helper->view_type() == content::VIEW_TYPE_TAB_CONTENTS; On 2012/01/20 19:10:20, darin wrote: > it seems a little hacky to be using something from the extension system for > this. it seems like you are trying to test if the RenderView belongs to a > TabContents. surely, there must be or could be a more direct way of determining > that via the Content API. if not, then perhaps we should add one. it seems > wrong to entangle the extension system here when this code really has nothing to > do with extensions. I don't know what the answer to that question is...perhaps jam might know? I'll ask him. http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:387: WebSize viewSize = render_view()->GetWebView()->size(); On 2012/01/20 19:10:20, darin wrote: > nit: view_size Done. http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:388: int layoutHeight = On 2012/01/20 19:10:20, darin wrote: > nit: layout_height Done. http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:396: setShouldLayoutFixedElementsRelativeToFrame(true); On 2012/01/20 19:10:20, darin wrote: > isn't this done through WebSettings now? Patch hasn't landed yet. Will fix now. But once it does, this isn't necessary, so I've removed those lines.
http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_view_observer.cc (right): http://codereview.chromium.org/7831028/diff/40001/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_view_observer.cc:383: extension_helper->view_type() == content::VIEW_TYPE_TAB_CONTENTS; On 2012/01/20 21:05:04, Fady Samuel wrote: > On 2012/01/20 19:10:20, darin wrote: > > it seems a little hacky to be using something from the extension system for > > this. it seems like you are trying to test if the RenderView belongs to a > > TabContents. surely, there must be or could be a more direct way of > determining > > that via the Content API. if not, then perhaps we should add one. it seems > > wrong to entangle the extension system here when this code really has nothing > to > > do with extensions. > > I don't know what the answer to that question is...perhaps jam might know? I'll > ask him. code in the renderer doesn't generally know about TabContents, by design. can you have the browser process (TabContents?) to send relevant ipcs to enable/disable ? it also seems that this isn't chrome related, but something that completely belongs in content (since all the other code in this cl is there)
Darin, could you please take a look at this patch. It minimizes changes to render_view_impl, adds a new method to the content renderer client for RenderViewResized, and introduces a default device scale factor for debugging purposes. This flag is only used in fixed layout mode.
https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... File chrome/renderer/chrome_content_renderer_client.cc (right): https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:267: if (CommandLine::ForCurrentProcess()->HasSwitch( nit: probably should cache a pointer to the CommandLine here. https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:283: int defaultDeviceScaleFactor = 0; nit: use google_style_for_variable_names https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:285: switches::kDefaultDeviceScaleFactor), nit: indentation https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:289: render_view->GetWebView()->settings()->setLayoutFallbackWidth( it seems a bit unusual to update WebSettings on resize. also, are you sure it makes sense to be doing this from src/chrome and not src/content? shouldn't other src/content embedders code similar logic? https://chromiumcodereview.appspot.com/7831028/diff/47001/content/renderer/re... File content/renderer/render_view_impl.cc (right): https://chromiumcodereview.appspot.com/7831028/diff/47001/content/renderer/re... content/renderer/render_view_impl.cc:4365: content::GetContentClient()->renderer()->RenderViewResized( this is really a "before"-resize event. notice how the WebView's size is still the old size when RenderViewResized is called. is that intentional?
Updated according to your comments, Darin. I believe it should be in Chrome not content because "default device scale factor" seems like a a Chrome specific thing. A flag to enable fixed layout also seems like an embedder specific thing because many embedders might not want fixed layout. Thanks. https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... File chrome/renderer/chrome_content_renderer_client.cc (right): https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:283: int defaultDeviceScaleFactor = 0; On 2012/02/15 06:13:23, darin wrote: > nit: use google_style_for_variable_names Done. I always forget when switching between chromium and webkit changes :) https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:285: switches::kDefaultDeviceScaleFactor), On 2012/02/15 06:13:23, darin wrote: > nit: indentation I think I have matched the style guide now. https://chromiumcodereview.appspot.com/7831028/diff/47001/chrome/renderer/chr... chrome/renderer/chrome_content_renderer_client.cc:289: render_view->GetWebView()->settings()->setLayoutFallbackWidth( On 2012/02/15 06:13:23, darin wrote: > it seems a bit unusual to update WebSettings on resize. also, are you sure it > makes sense to be doing this from src/chrome and not src/content? shouldn't > other src/content embedders code similar logic? In my humble opinion, no. The default device scale factor is a property of chrome as a whole and not just WebKit (it is only used by WebKit currently but that will change shortly). Setting the layout fallback width to be equal to the new width of the render view is also something that feels Chrome specific to me. https://chromiumcodereview.appspot.com/7831028/diff/47001/content/renderer/re... File content/renderer/render_view_impl.cc (right): https://chromiumcodereview.appspot.com/7831028/diff/47001/content/renderer/re... content/renderer/render_view_impl.cc:4365: content::GetContentClient()->renderer()->RenderViewResized( On 2012/02/15 06:13:23, darin wrote: > this is really a "before"-resize event. notice how the WebView's size is still > the old size when RenderViewResized is called. is that intentional? For the purposes of the layout fallback width yes, because the viewport is computed after this. I have renamed the method to RenderViewWillResize for clarity.
Sorry for the drive-by. I was talking to Fady about this CL. darin@: I wasn't sure what you meant in your comment. Did you mean that the pageScaleFactor computation is something that we'd want in content shell too? My intuition would have been that this is functionality that belongs in content. Is this reasonable?
Updated according to discussion with rjkroege.
On 2012/02/16 23:21:35, Fady Samuel wrote: > Updated according to discussion with rjkroege. Seems to put all the pieces where my intuition suggests they belong so lgtm. darin@ perhaps you could have a look? (I note in passing that this CL is important for making progress on high density rendering.)
http://codereview.chromium.org/7831028/diff/56001/content/public/common/conte... File content/public/common/content_switches.cc (right): http://codereview.chromium.org/7831028/diff/56001/content/public/common/conte... content/public/common/content_switches.cc:39: // The default device scale factor to apply to contents in the absense of nit: absence http://codereview.chromium.org/7831028/diff/56001/content/public/common/conte... content/public/common/content_switches.cc:243: // This flag fixes the layout of the page to a default of 980px. these are css pixels. http://codereview.chromium.org/7831028/diff/56001/content/public/renderer/ren... File content/public/renderer/render_view.h (right): http://codereview.chromium.org/7831028/diff/56001/content/public/renderer/ren... content/public/renderer/render_view.h:71: // Gets the default device scale factor of the render view in absense absence
Fixed nits. http://codereview.chromium.org/7831028/diff/56001/content/public/common/conte... File content/public/common/content_switches.cc (right): http://codereview.chromium.org/7831028/diff/56001/content/public/common/conte... content/public/common/content_switches.cc:39: // The default device scale factor to apply to contents in the absense of On 2012/02/16 23:35:05, rjkroege wrote: > nit: absence Done. http://codereview.chromium.org/7831028/diff/56001/content/public/common/conte... content/public/common/content_switches.cc:243: // This flag fixes the layout of the page to a default of 980px. On 2012/02/16 23:35:05, rjkroege wrote: > these are css pixels. Done. http://codereview.chromium.org/7831028/diff/56001/content/public/renderer/ren... File content/public/renderer/render_view.h (right): http://codereview.chromium.org/7831028/diff/56001/content/public/renderer/ren... content/public/renderer/render_view.h:71: // Gets the default device scale factor of the render view in absense On 2012/02/16 23:35:05, rjkroege wrote: > absence Done.
LGTM http://codereview.chromium.org/7831028/diff/56003/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/7831028/diff/56003/content/renderer/render_vie... content/renderer/render_view_impl.cc:3390: int RenderViewImpl::GetDefaultDeviceScaleFactor() const { until we have a consumer for this method, how about we leave this out? maybe in the future, we'll find that we actually need to IPC this up to the RenderViewHost instead. or something.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/7831028/58008
Presubmit check for 7831028-58008 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/public/common/content_switches.cc,content/public/common/content_switches.h Presubmit checks took 2.6s to calculate.
content/public lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/7831028/58008
Try job failure for 7831028-58008 (retry) on android for step "update". It's a second try, previously, step "update" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/7831028/63001
Change committed as 123126 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
