Bug 86678 – Inconsistency in FrameView::forceLayoutForPagination and FrameView::forceLayout.

• Status: UNCONFIRMED
• Component: Printing

[Vitaly Buka]

FrameView::forceLayoutForPagination calls adjustViewSize when forceLayout does not.

However after every call to forceLayout near forceLayoutForPagination followed by adjustViewSize.

[Vitaly Buka]

Created attachment 142348 [details]
Removed adjustViewSize from forceLayoutForPagination.

[Vitaly Buka]

Comment on attachment 142348 [details]
Removed adjustViewSize from forceLayoutForPagination.

View in context: https://bugs.webkit.org/attachment.cgi?id=142348&action=review

> Source/WebCore/WebCore.order:28540
> +__ZN7WebCore9FrameView24forceLayoutForPaginationERKNS_9FloatSizeEf

I have no idea if manual changing of this file and above is appropriate.

[Alexey Proskuryakov]

> I have no idea if manual changing of this file and above is appropriate.

It's unnecessary, yet harmless.

[Vitaly Buka]

(In reply to comment #3)
> > I have no idea if manual changing of this file and above is appropriate.
> 
> It's unnecessary, yet harmless.

Without change build-webkit script fails linking some targets.

[Vitaly Buka]

Created attachment 143392 [details]
Removed adjustViewSize from forceLayoutForPagination.

Updated with repository changes.

[WebKit Review Bot]

Attachment 143392 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/page/FrameView.h:267:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/page/FrameView.cpp:3222:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vitaly Buka]

(In reply to comment #6)
> Attachment 143392 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/page/FrameView.h:267:  Should have a space between // and comment  [whitespace/comments] [4]
> Source/WebCore/page/FrameView.cpp:3222:  Should have a space between // and comment  [whitespace/comments] [4]
> Total errors found: 2 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I didn't expect so fast response. I have uploaded wrong patch.

[Vitaly Buka]

Created attachment 143396 [details]
Removed adjustViewSize from forceLayoutForPagination.

[Brent Fulgham]

Comment on attachment 143396 [details]
Removed adjustViewSize from forceLayoutForPagination.

View in context: https://bugs.webkit.org/attachment.cgi?id=143396&action=review

I think this change looks good. I'm curious if the two methods are ever used in isolation such that there actually does need this additional parameter?

It sounds like the answer is no, and therefore this simplification is a great change.

Can you confirm this, and I will r+ the patch so it can be landed?

> Source/WebCore/ChangeLog:7
> +

You should add the helpful text from your bug report: "
FrameView::forceLayoutForPagination calls adjustViewSize internally, while forceLayout does not. However, every call to forceLayout (near forceLayoutForPagination) is paired with a call to adjustViewSize. Make the two calls consistent."

> Source/WebKit/mac/WebView/WebHTMLView.mm:-3059
> -            coreView->forceLayoutForPagination(pageSize, originalPageSize, maximumShrinkRatio, adjustViewSize ? AdjustViewSize : DoNotAdjustViewSize);

Is the forceLayoutForPagination method ever called without a nearby call to forceLayout?