Bug 35347 – Convert the zoom mode into a proper enum

• Component: Frames
• Status: RESOLVED FIXED

[Jakob Petsovits]

"setZoomFactor(0.8, false)" has annoyed me since I first saw it. Unless you're really familiar with the code or able to deduct it from the bug title, you have little chance of knowing what the "false" argument means. The Qt "how to design good APIs" guide specifically mentions vague boolean arguments like this one as example of not to design APIs. Unfortunately, most ports have already adopted this argument style. That doesn't mean, though, that we shouldn't fix it in the internal WebCore API, as it still makes for vastly better readability.

So, the patch below replaces the boolean argument by an enum called ZoomMode, and adapts all ports without changing their public APIs (I'll leave that decision to the respective port maintainers and their ABI policies). Hopefully I didn't miss any occurrence, let's see what the bots say. Please review, thanks!

[Jakob Petsovits]

Created attachment 49401 [details]
Convert the zoom mode into a proper enum

[Adam Treat]

Comment on attachment 49401 [details]
Convert the zoom mode into a proper enum

> [phone]  Jakob Petsovits  <[e-mail]>

Please change to correct email address.

> diff --git a/WebCore/page/Settings.cpp b/WebCore/page/Settings.cpp
> index cc8f7af..a9f6c5c 100644
> --- a/WebCore/page/Settings.cpp
> +++ b/WebCore/page/Settings.cpp
> @@ -102,7 +102,7 @@ Settings::Settings(Page* page)
>      , m_inApplicationChromeMode(false)
>      , m_offlineWebApplicationCacheEnabled(false)
>      , m_shouldPaintCustomScrollbars(false)
> -    , m_zoomsTextOnly(false)
> +    , m_zoomMode(ZoomPage)

...

> @@ -309,6 +314,7 @@ namespace WebCore {
>          size_t m_maximumDecodedImageSize;
>          unsigned m_localStorageQuota;
>          unsigned m_pluginAllowedRunTime;
> +        ZoomMode m_zoomMode;
>          bool m_isJavaEnabled : 1;
>          bool m_loadsImagesAutomatically : 1;
>          bool m_privateBrowsingEnabled : 1;
> @@ -347,7 +353,6 @@ namespace WebCore {
>          bool m_inApplicationChromeMode : 1;
>          bool m_offlineWebApplicationCacheEnabled : 1;
>          bool m_shouldPaintCustomScrollbars : 1;
> -        bool m_zoomsTextOnly : 1;
>          bool m_enforceCSSMIMETypeInStrictMode : 1;
>          bool m_usesEncodingDetector : 1;
>          bool m_allowScriptsToCloseWindows : 1;

Incorrect initialization order...

Please update and fix above to produce a new patch.  Since this touches most ports it'd be nice if the bots were working to check the build with this patch.

[Jakob Petsovits]

Created attachment 49402 [details]
Convert the zoom mode into a proper enum (try 2)

Fixes email address and initialization order. Hope the bots will work.

> +++ b/WebCore/page/Frame.h
> @@ -37,6 +37,7 @@
> #include "ScriptController.h"
> #include "ScrollBehavior.h"
> #include "SelectionController.h"
> +#include "Settings.h"

This is unfortunate. Have you considered putting the enum in a separate header file?

[Jakob Petsovits]

I have, and it seemed kind of overkill to me so I thought I'd wait whether it's an issue for the reviewers. Seems it is, so let's make it a separate header... or maybe there's a better place to add it to? Maybe not. Anyways, I'll have a look and change the patch accordingly.

[Jakob Petsovits]

Created attachment 49410 [details]
Convert the zoom mode into a proper enum (try 3)

Adding WebCore/page/ZoomMode.h and including that instead of the whole Settings.h.

[David Levin]

(In reply to comment #6)
> Created an attachment (id=49410) [details]
> Convert the zoom mode into a proper enum (try 3)
> 
> Adding WebCore/page/ZoomMode.h and including that instead of the whole
> Settings.h.

A few notes/nits that you may want to fix:
1. Your copyright says 2009. It should be 2010.
2. The newly added file doesn't have header guards. (ifndef/define/endif).
3. The header file wasn't added to xcode project file WebCore.xcodeproj/project.pbxproj

[Jakob Petsovits]

1. and 2. blargh, let me fix that right away.
3. There's no way I can add that without having a Mac with XCode. You can't seriously expect me to generate hashs manually?

[Jakob Petsovits]

Created attachment 49413 [details]
Convert the zoom mode into a proper enum (try 4)

Updated patch with David Levin's points 1. and 2., XCode project file still untouched.

[WebKit Review Bot]

Attachment 49413 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/308039

[WebKit Review Bot]

Attachment 49413 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/309045

[Jakob Petsovits]

Created attachment 49446 [details]
Convert the zoom mode into a proper enum (try 5)

Yay for bots! This one should fix GTK, Qt, and even Wx.

[Jakob Petsovits]

Bots are all green, except for mac which hasn't run yet. Hope that doesn't take too long.

[Darin Adler]

(In reply to comment #13)
> Bots are all green, except for mac which hasn't run yet. Hope that doesn't take
> too long.

Mac EWS is different and won't run unless the patch is put up for review by a certain set of people. Adam Barth set it up and knows the details.

[Darin Adler]

Comment on attachment 49446 [details]
Convert the zoom mode into a proper enum (try 5)

> -    return _private->page->settings()->zoomsTextOnly();
> +    return _private->page->settings()->zoomMode() == ZoomTextOnly ? YES : NO;

The "? YES : NO" here is not needed or helpful.

Someone will have to make the Xcode project changes.

The patch seems fine. r=me if it builds on Mac.

[Adam Barth]

The mac-ews is stuck unable to build.  Eric, can you kick it?

[Daniel Bates]

Created attachment 49498 [details]
Patch

Added ZoomMode.h to Xcode project and made change as per Darin's comment.

[Adam Barth]

Comment on attachment 49498 [details]
Patch

Forwarding Darin's review.  The commit-queue will ensure that this builds on Mac before landing, completing the last of Darin's requests.

[Eric Seidel (no email)]

The mac-ews bot has been kicked, but it has some back-log.

[WebKit Commit Bot]

Comment on attachment 49498 [details]
Patch

Rejecting patch 49498 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--no-update', '--parent-command=commit-queue', '--build-style=both', '--quiet', '49498']" exit_code: 1
Last 500 characters of output:
ebkitpy/statusserver.py", line 87, in update_status
    return NetworkTransaction().run(lambda: self._post_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 52, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 58, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.networktransaction.NetworkTimeout

[Eric Seidel (no email)]

Sorry for the commit-queue spew.  Normally that means that this patch fails to build on Mac in such a way that the build log is so huge the commit-queue is having trouble uploading it. :(

[Jakob Petsovits]

So... where do we go from here?

[Eric Seidel (no email)]

Comment on attachment 49498 [details]
Patch

We mark it for review again and see if the mac EWS barfs similarly.  Or someone with a mac box tries to manually download the patch and builds it and reports back the error.  Eventually we'll fix the commit-queue. :)

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/statusserver.py#L67
is the code that's failing.  It probably just needs to be smart enough to only upload the last 500k of the file or similar.  Or more likely that method should warn for large files and the callsites should be restricting the upload size.

[Adam Barth]

Comment on attachment 49498 [details]
Patch

This patch doesn't build on Mac:

Undefined symbols:
  "__ZN7WebCore5Frame13setZoomFactorEfb", referenced from:
     -exported_symbols_list command line option
  "__ZN7WebCore8Settings16setZoomsTextOnlyEb", referenced from:
     -exported_symbols_list command line option
ld: symbol(s) not found
collect2: ld returned 1 exit status
** BUILD FAILED **

[Jakob Petsovits]

Thanks everyone. I'll investigate what I might have forgotten to change, but chances are I'll need help from someone with a Mac build to get this working. Let's see.

[Jakob Petsovits]

It seems to me like the only occurrences of these functions are in WebCore/WebCore.base.exp and WebCore/WebCore.order. Those files probably only need to be regenerated, any idea on how to do that? (Given that I'm not a mangling expert, I think I shouldn't try to do it without the proper script.)

[Darin Adler]

(In reply to comment #26)
> It seems to me like the only occurrences of these functions are in
> WebCore/WebCore.base.exp and WebCore/WebCore.order. Those files probably only
> need to be regenerated, any idea on how to do that? (Given that I'm not a
> mangling expert, I think I shouldn't try to do it without the proper script.)

WebCore.order should be left alone.

WebCore.base.exp can be updated based on build failure messages by someone with a Mac. First remove the old symbols so WebCore builds, then build again and add the new symbols based on the failure in the WebKit build.

[Daniel Bates]

Created attachment 49746 [details]
Patch

Substituted symbols __ZN7WebCore5Frame13setZoomFactorEfNS_8ZoomModeE and __ZN7WebCore8Settings11setZoomModeENS_8ZoomModeE for __ZN7WebCore5Frame13setZoomFactorEfb and __ZN7WebCore8Settings16setZoomsTextOnlyEb, respectively.

Set role of ZoomMode.h to private in Xcode project.

Resolved compiler error in [-WebView _setZoomMultiplier:isTextOnly] caused by using the old prototype for Frame::setZoomFactor (which took a boolean type for the second argument). Instead, the second argument is now an enum value.

[Jakob Petsovits]

Thanks Dan, I really appreciate it :)

[WebKit Commit Bot]

Comment on attachment 49746 [details]
Patch

Clearing flags on attachment: 49746

Committed r55387: <http://trac.webkit.org/changeset/55387>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.