Bug 102790 – PageViewportControllerClient::didChangeViewportAttributes called twice

• Component: WebKit2
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

Both PageViewportController::updateMinimumScaleToFit and PageViewportController::didChangeViewportAttributes calls didChangeViewportAttributes on the PageViewportControllerClient, and since the former calls the later this means we end up firing didChangeViewportAttributes twice on the client.

[Allan Sandfeld Jensen]

Created attachment 175181 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 175182 [details]
Patch

Accidently uploaded unrelated change

[Kenneth Rohde Christiansen]

Comment on attachment 175182 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:218
> +    updateMinimumScaleToFit(false);

wouldn't it be nicer if it returned a bool instead?

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 175182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175182&action=review
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:218
> > +    updateMinimumScaleToFit(false);
> 
> wouldn't it be nicer if it returned a bool instead?

Perhaps. I am fine with either way.

[Andras Becsi]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 175182 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=175182&action=review
> > 
> > > Source/WebKit2/UIProcess/PageViewportController.cpp:218
> > > +    updateMinimumScaleToFit(false);
> > 
> > wouldn't it be nicer if it returned a bool instead?
> 
> Perhaps. I am fine with either way.

I also think returning a bool indicating whether it updated the minimum size or not is the preferred pattern instead of a bool argument.

[Allan Sandfeld Jensen]

Created attachment 175209 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 175209 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:108
> +    if (updateMinimumScaleToFit())

Maybe that should ahve IfNeeded() added?

[Allan Sandfeld Jensen]

(In reply to comment #7)
> (From update of attachment 175209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175209&action=review
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:108
> > +    if (updateMinimumScaleToFit())
> 
> Maybe that should ahve IfNeeded() added?

I don't feel that is necessary for a method name starting with update, since update usually means to change something if needed.

[Allan Sandfeld Jensen]

Committed r135283: <http://trac.webkit.org/changeset/135283>