Bug 76459 – Rename shouldLayoutFixedElementsRelativeToFrame and make it a setting

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Fady Samuel]

Make shouldLayoutFixedElementsRelativeToFrame a setting

[Fady Samuel]

Created attachment 122773 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Simon Fraser (smfr)]

Will this setting ever be changed at runtime?

[Fady Samuel]

(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.

[Gustavo Noronha (kov)]

Comment on attachment 122773 [details]
Patch

Attachment 122773 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11219013

[Darin Fisher (:fishd, Google)]

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.

[Fady Samuel]

(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.

[Fady Samuel]

(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.

[Fady Samuel]

Created attachment 123369 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 123369 [details]
Patch

Attachment 123369 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11246203

[Hajime Morrita]

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.

[Fady Samuel]

Created attachment 123690 [details]
Patch

[Fady Samuel]

Created attachment 123820 [details]
Patch

[Darin Fisher (:fishd, Google)]

(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.

[Fady Samuel]

(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.

[Fady Samuel]

Created attachment 123876 [details]
Patch

[Fady Samuel]

(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.

[Gustavo Noronha (kov)]

Comment on attachment 123876 [details]
Patch

Attachment 123876 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11242770

[Darin Fisher (:fishd, Google)]

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"!

[Fady Samuel]

(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)?

[Simon Fraser (smfr)]

fixedElementsLayoutRelativeToFrame()?

[Fady Samuel]

(In reply to comment #22)
> fixedElementsLayoutRelativeToFrame()?

That sounds good to me:

fixedElementsLayoutRelativeToFrame()

and 

setFixedElementsLayoutRelativeToFrame(...)

Thanks, Simon.

[Fady Samuel]

(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.

[Fady Samuel]

*** Bug 73753 has been marked as a duplicate of this bug. ***

[Fady Samuel]

Created attachment 124117 [details]
Patch

[Fady Samuel]

Created attachment 124217 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 124217 [details]
Patch

Attachment 124217 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11297911

[Fady Samuel]

Created attachment 124254 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 124254 [details]
Patch

Attachment 124254 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11163659

[Fady Samuel]

Created attachment 124323 [details]
Patch

[Robert Kroeger]

Comment on attachment 124323 [details]
Patch

This change looks nice to me. And also seems highly desirable per off-line discussion.

[WebKit Review Bot]

Comment on attachment 124323 [details]
Patch

Clearing flags on attachment: 124323

Committed r106146: <http://trac.webkit.org/changeset/106146>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.