Bug 76459 - Rename shouldLayoutFixedElementsRelativeToFrame and make it a setting
Summary: Rename shouldLayoutFixedElementsRelativeToFrame and make it a setting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
: 73753 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-01-17 08:25 PST by Fady Samuel
Modified: 2012-01-27 13:52 PST (History)
8 users (show)

See Also:


Attachments
Patch (11.89 KB, patch)
2012-01-17 08:27 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (12.67 KB, patch)
2012-01-20 13:39 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (15.27 KB, patch)
2012-01-23 19:52 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (15.71 KB, patch)
2012-01-24 15:05 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (24.36 KB, patch)
2012-01-24 19:46 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (24.42 KB, patch)
2012-01-26 07:27 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (24.57 KB, patch)
2012-01-26 16:55 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (25.99 KB, patch)
2012-01-26 21:42 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (27.42 KB, patch)
2012-01-27 08:21 PST, Fady Samuel
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 2012-01-17 08:25:43 PST
Make shouldLayoutFixedElementsRelativeToFrame a setting
Comment 1 Fady Samuel 2012-01-17 08:27:14 PST
Created attachment 122773 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-17 08:29:28 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Simon Fraser (smfr) 2012-01-17 09:09:41 PST
Will this setting ever be changed at runtime?
Comment 4 Fady Samuel 2012-01-17 09:53:30 PST
(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 5 Gustavo Noronha (kov) 2012-01-17 11:41:09 PST
Comment on attachment 122773 [details]
Patch

Attachment 122773 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11219013
Comment 6 Darin Fisher (:fishd, Google) 2012-01-18 13:52:05 PST
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.
Comment 7 Fady Samuel 2012-01-20 10:13:54 PST
(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.
Comment 8 Fady Samuel 2012-01-20 13:12:22 PST
(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.
Comment 9 Fady Samuel 2012-01-20 13:39:49 PST
Created attachment 123369 [details]
Patch
Comment 10 Collabora GTK+ EWS bot 2012-01-20 14:14:10 PST
Comment on attachment 123369 [details]
Patch

Attachment 123369 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11246203
Comment 11 Hajime Morrita 2012-01-23 13:40:04 PST
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.
Comment 12 Fady Samuel 2012-01-23 19:52:37 PST
Created attachment 123690 [details]
Patch
Comment 13 Fady Samuel 2012-01-24 15:05:44 PST
Created attachment 123820 [details]
Patch
Comment 14 Darin Fisher (:fishd, Google) 2012-01-24 17:26:19 PST
(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.
Comment 15 Fady Samuel 2012-01-24 17:34:30 PST
(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.
Comment 16 Fady Samuel 2012-01-24 19:46:18 PST
Created attachment 123876 [details]
Patch
Comment 17 Fady Samuel 2012-01-24 19:48:55 PST
(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 18 Gustavo Noronha (kov) 2012-01-25 05:22:57 PST
Comment on attachment 123876 [details]
Patch

Attachment 123876 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11242770
Comment 19 Darin Fisher (:fishd, Google) 2012-01-25 09:30:31 PST
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"!
Comment 20 Fady Samuel 2012-01-25 09:41:53 PST
(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 21 mitz 2012-01-25 09:42:48 PST
Comment on attachment 123876 [details]
Patch

Shouldn't the setter function invalidate something (style, layout, or at least pixels)?
Comment 22 Simon Fraser (smfr) 2012-01-25 09:47:58 PST
fixedElementsLayoutRelativeToFrame()?
Comment 23 Fady Samuel 2012-01-25 12:19:54 PST
(In reply to comment #22)
> fixedElementsLayoutRelativeToFrame()?

That sounds good to me:

fixedElementsLayoutRelativeToFrame()

and 

setFixedElementsLayoutRelativeToFrame(...)

Thanks, Simon.
Comment 24 Fady Samuel 2012-01-25 12:23:05 PST
(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.
Comment 25 Fady Samuel 2012-01-25 19:21:31 PST
*** Bug 73753 has been marked as a duplicate of this bug. ***
Comment 26 Fady Samuel 2012-01-26 07:27:52 PST
Created attachment 124117 [details]
Patch
Comment 27 Fady Samuel 2012-01-26 16:55:13 PST
Created attachment 124217 [details]
Patch
Comment 28 Gustavo Noronha (kov) 2012-01-26 21:07:22 PST
Comment on attachment 124217 [details]
Patch

Attachment 124217 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11297911
Comment 29 Fady Samuel 2012-01-26 21:42:31 PST
Created attachment 124254 [details]
Patch
Comment 30 Gustavo Noronha (kov) 2012-01-26 23:54:04 PST
Comment on attachment 124254 [details]
Patch

Attachment 124254 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11163659
Comment 31 Fady Samuel 2012-01-27 08:21:55 PST
Created attachment 124323 [details]
Patch
Comment 32 Robert Kroeger 2012-01-27 09:49:49 PST
Comment on attachment 124323 [details]
Patch

This change looks nice to me. And also seems highly desirable per off-line discussion.
Comment 33 WebKit Review Bot 2012-01-27 13:52:17 PST
Comment on attachment 124323 [details]
Patch

Clearing flags on attachment: 124323

Committed r106146: <http://trac.webkit.org/changeset/106146>
Comment 34 WebKit Review Bot 2012-01-27 13:52:24 PST
All reviewed patches have been landed.  Closing bug.