Bug 102790 - PageViewportControllerClient::didChangeViewportAttributes called twice
Summary: PageViewportControllerClient::didChangeViewportAttributes called twice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-20 03:44 PST by Allan Sandfeld Jensen
Modified: 2012-11-20 08:23 PST (History)
4 users (show)

See Also:

Attachments
Patch (3.78 KB, patch)
2012-11-20 03:46 PST, Allan Sandfeld Jensen 		no flags Details | Formatted Diff | Diff
Patch (3.16 KB, patch)
2012-11-20 03:47 PST, Allan Sandfeld Jensen 		no flags Details | Formatted Diff | Diff
Patch (3.44 KB, patch)
2012-11-20 06:59 PST, Allan Sandfeld Jensen 		kenneth: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-11-20 03:44:00 PST 
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.
Comment 1 Allan Sandfeld Jensen 2012-11-20 03:46:32 PST 
Created attachment 175181 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2012-11-20 03:47:42 PST 
Created attachment 175182 [details]
Patch

Accidently uploaded unrelated change
Comment 3 Kenneth Rohde Christiansen 2012-11-20 04:48:38 PST 
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?
Comment 4 Allan Sandfeld Jensen 2012-11-20 04:57:39 PST 
(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.
Comment 5 Andras Becsi 2012-11-20 05:36:19 PST 
(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.
Comment 6 Allan Sandfeld Jensen 2012-11-20 06:59:10 PST 
Created attachment 175209 [details]
Patch
Comment 7 Kenneth Rohde Christiansen 2012-11-20 07:58:29 PST 
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?
Comment 8 Allan Sandfeld Jensen 2012-11-20 08:15:34 PST 
(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.
Comment 9 Allan Sandfeld Jensen 2012-11-20 08:23:47 PST 
Committed r135283: <http://trac.webkit.org/changeset/135283>