Make shouldLayoutFixedElementsRelativeToFrame a setting
Created attachment 122773 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Will this setting ever be changed at runtime?
(In reply to comment #3) > Will this setting ever be changed at runtime? For layout tests, certainly. However, we probably want this behavior always turned on for the chromium WebKit port. I'm not sure what the best way is to specify that on the WebKit side.
Comment on attachment 122773 [details] Patch Attachment 122773 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11219013
Comment on attachment 122773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122773&action=review > Source/WebKit/chromium/public/WebSettings.h:88 > + virtual void setShouldLayoutFixedElementsRelativeToFrame(bool) = 0; nit: while you are making these changes, is it really necessary to have the word "should" in here? it seems to read well without "should" too.
(In reply to comment #6) > (From update of attachment 122773 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122773&action=review > > > Source/WebKit/chromium/public/WebSettings.h:88 > > + virtual void setShouldLayoutFixedElementsRelativeToFrame(bool) = 0; > > nit: while you are making these changes, is it really necessary to have the word "should" in here? it seems to read well without "should" too. The naming was at the request of Simon Fraser in WebCore. I left it the same in WebSettings for consistency.
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 122773 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=122773&action=review > > > > > Source/WebKit/chromium/public/WebSettings.h:88 > > > + virtual void setShouldLayoutFixedElementsRelativeToFrame(bool) = 0; > > > > nit: while you are making these changes, is it really necessary to have the word "should" in here? it seems to read well without "should" too. > > The naming was at the request of Simon Fraser in WebCore. I left it the same in WebSettings for consistency. The naming was suggested by smfr in WebCore. I kept it in WebSettings for consistency.
Created attachment 123369 [details] Patch
Comment on attachment 123369 [details] Patch Attachment 123369 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11246203
Comment on attachment 123369 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123369&action=review Please fix the build breakage on gtk and win. It looks a real failure. > Source/WebCore/page/Settings.h:631 > + bool m_shouldLayoutFixedElementsRelativeToFrame : 1; Please add an initializer for this value in the constructor.
Created attachment 123690 [details] Patch
Created attachment 123820 [details] Patch
(In reply to comment #7) > > > Source/WebKit/chromium/public/WebSettings.h:88 > > > + virtual void setShouldLayoutFixedElementsRelativeToFrame(bool) = 0; > > > > nit: while you are making these changes, is it really necessary to have the word "should" in here? it seems to read well without "should" too. > > The naming was at the request of Simon Fraser in WebCore. I left it the same in WebSettings for consistency. OK, but what is the point of including the "should" word? Is this an optional request? Can WebKit ignore this setting? The word "should" suggests some variability. Not to be overly pedantic, but for reference, see how RFCs define it: SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course.
(In reply to comment #14) > (In reply to comment #7) > > > > Source/WebKit/chromium/public/WebSettings.h:88 > > > > + virtual void setShouldLayoutFixedElementsRelativeToFrame(bool) = 0; > > > > > > nit: while you are making these changes, is it really necessary to have the word "should" in here? it seems to read well without "should" too. > > > > The naming was at the request of Simon Fraser in WebCore. I left it the same in WebSettings for consistency. > > OK, but what is the point of including the "should" word? Is this an optional request? Can WebKit ignore this setting? The word "should" suggests some variability. > > Not to be overly pedantic, but for reference, see how RFCs define it: > > SHOULD This word, or the adjective "RECOMMENDED", mean that there > may exist valid reasons in particular circumstances to ignore a > particular item, but the full implications must be understood and > carefully weighed before choosing a different course. I'll get rid of all instances of the word "should" in the the method name, and member variables. Thanks.
Created attachment 123876 [details] Patch
(In reply to comment #16) > Created an attachment (id=123876) [details] > Patch This might fail some EWS bots due to missing symbols, I'm waiting for the bots to report what symbols are missing and I will place them in the appropriate files. Please review independent of this. I will ensure bots are green before I land this patch.
Comment on attachment 123876 [details] Patch Attachment 123876 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11242770
Comment on attachment 123876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123876&action=review > Source/WebCore/page/FrameView.cpp:1419 > + float dragFactor = layoutFixedElementsRelativeToFrame() ? 1 : (contentsWidth() - visibleContentWidth * frameScaleFactor) / maxX; Oh, seeing the getter without the "should" prefix, now raises another issue. layoutFixedElementsRelativeToFrame() sounds like it is going to layout the fixed elements now. That's not what you mean at all! Taking that into account, a re-wording like laysOutFixedElementsRelativeToFrame / setLaysOutFixedElementsRelativeToFrame would work, but looking around WebKit more, I see that the "should" prefix is actually quite common, and if a getter is prefixed with should, then the setter should be named correspondingly as setShould. I'm torn between saying use laysOut versus stick with should because it is commonly used. Sorry for not considering the getter naming when I argued against "should"!
(In reply to comment #19) > (From update of attachment 123876 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123876&action=review > > > Source/WebCore/page/FrameView.cpp:1419 > > + float dragFactor = layoutFixedElementsRelativeToFrame() ? 1 : (contentsWidth() - visibleContentWidth * frameScaleFactor) / maxX; > > Oh, seeing the getter without the "should" prefix, now raises another issue. layoutFixedElementsRelativeToFrame() sounds like it is going to layout the fixed elements now. That's not what you mean at all! > > Taking that into account, a re-wording like laysOutFixedElementsRelativeToFrame / setLaysOutFixedElementsRelativeToFrame would work, but looking around WebKit more, I see that the "should" prefix is actually quite common, and if a getter is prefixed with should, then the setter should be named correspondingly as setShould. I'm torn between saying use laysOut versus stick with should because it is commonly used. > > Sorry for not considering the getter naming when I argued against "should"! Not a problem. Are you strongly for "setLaysOutFixedElementsRelativeToFrame" or should I go back to the previously submitted patch?
Comment on attachment 123876 [details] Patch Shouldn't the setter function invalidate something (style, layout, or at least pixels)?
fixedElementsLayoutRelativeToFrame()?
(In reply to comment #22) > fixedElementsLayoutRelativeToFrame()? That sounds good to me: fixedElementsLayoutRelativeToFrame() and setFixedElementsLayoutRelativeToFrame(...) Thanks, Simon.
(In reply to comment #21) > (From update of attachment 123876 [details]) > Shouldn't the setter function invalidate something (style, layout, or at least pixels)? We don't need to for the current tests/use cases but I suppose for generality, we can do that and allow other embedders to switch between the two modes.
*** Bug 73753 has been marked as a duplicate of this bug. ***
Created attachment 124117 [details] Patch
Created attachment 124217 [details] Patch
Comment on attachment 124217 [details] Patch Attachment 124217 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11297911
Created attachment 124254 [details] Patch
Comment on attachment 124254 [details] Patch Attachment 124254 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11163659
Created attachment 124323 [details] Patch
Comment on attachment 124323 [details] Patch This change looks nice to me. And also seems highly desirable per off-line discussion.
Comment on attachment 124323 [details] Patch Clearing flags on attachment: 124323 Committed r106146: <http://trac.webkit.org/changeset/106146>
All reviewed patches have been landed. Closing bug.