Bug 35347 - Convert the zoom mode into a proper enum
Summary: Convert the zoom mode into a proper enum
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-24 08:50 PST by Jakob Petsovits
Modified: 2010-03-01 16:56 PST (History)
10 users (show)

See Also:


Attachments
Convert the zoom mode into a proper enum (18.43 KB, patch)
2010-02-24 08:56 PST, Jakob Petsovits
manyoso: review-
Details | Formatted Diff | Diff
Convert the zoom mode into a proper enum (try 2) (18.71 KB, patch)
2010-02-24 09:16 PST, Jakob Petsovits
no flags Details | Formatted Diff | Diff
Convert the zoom mode into a proper enum (try 3) (21.21 KB, patch)
2010-02-24 10:42 PST, Jakob Petsovits
no flags Details | Formatted Diff | Diff
Convert the zoom mode into a proper enum (try 4) (21.26 KB, patch)
2010-02-24 10:59 PST, Jakob Petsovits
no flags Details | Formatted Diff | Diff
Convert the zoom mode into a proper enum (try 5) (22.34 KB, patch)
2010-02-24 15:02 PST, Jakob Petsovits
no flags Details | Formatted Diff | Diff
Patch (26.28 KB, patch)
2010-02-25 09:36 PST, Daniel Bates
abarth: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (27.93 KB, patch)
2010-03-01 13:07 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakob Petsovits 2010-02-24 08:50:25 PST
"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!
Comment 1 Jakob Petsovits 2010-02-24 08:56:33 PST
Created attachment 49401 [details]
Convert the zoom mode into a proper enum
Comment 2 Adam Treat 2010-02-24 09:12:40 PST
Comment on attachment 49401 [details]
Convert the zoom mode into a proper enum

> +2010-02-24  Jakob Petsovits  <jakob.petsovits@torchmobile.com>

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.
Comment 3 Jakob Petsovits 2010-02-24 09:16:24 PST
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.
Comment 4 mitz 2010-02-24 09:22:18 PST
> +++ 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?
Comment 5 Jakob Petsovits 2010-02-24 10:11:51 PST
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.
Comment 6 Jakob Petsovits 2010-02-24 10:42:47 PST
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.
Comment 7 David Levin 2010-02-24 10:52:32 PST
(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
Comment 8 Jakob Petsovits 2010-02-24 10:56:18 PST
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?
Comment 9 Jakob Petsovits 2010-02-24 10:59:30 PST
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.
Comment 10 WebKit Review Bot 2010-02-24 12:55:43 PST
Attachment 49413 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/308039
Comment 11 WebKit Review Bot 2010-02-24 13:02:54 PST
Attachment 49413 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/309045
Comment 12 Jakob Petsovits 2010-02-24 15:02:12 PST
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.
Comment 13 Jakob Petsovits 2010-02-25 08:23:21 PST
Bots are all green, except for mac which hasn't run yet. Hope that doesn't take too long.
Comment 14 Darin Adler 2010-02-25 08:28:11 PST
(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.
Comment 15 Darin Adler 2010-02-25 08:30:00 PST
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.
Comment 16 Adam Barth 2010-02-25 09:15:15 PST
The mac-ews is stuck unable to build.  Eric, can you kick it?
Comment 17 Daniel Bates 2010-02-25 09:36:56 PST
Created attachment 49498 [details]
Patch

Added ZoomMode.h to Xcode project and made change as per Darin's comment.
Comment 18 Adam Barth 2010-02-25 09:38:40 PST
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.
Comment 19 Eric Seidel (no email) 2010-02-25 14:10:01 PST
The mac-ews bot has been kicked, but it has some back-log.
Comment 20 WebKit Commit Bot 2010-02-26 06:52:18 PST
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
Comment 21 Eric Seidel (no email) 2010-02-26 07:11:20 PST
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. :(
Comment 22 Jakob Petsovits 2010-02-26 07:20:53 PST
So... where do we go from here?
Comment 23 Eric Seidel (no email) 2010-02-26 07:27:08 PST
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.
Comment 24 Adam Barth 2010-02-27 02:09:13 PST
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 **
Comment 25 Jakob Petsovits 2010-02-27 11:07:17 PST
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.
Comment 26 Jakob Petsovits 2010-03-01 09:04:36 PST
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.)
Comment 27 Darin Adler 2010-03-01 09:38:26 PST
(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.
Comment 28 Daniel Bates 2010-03-01 13:07:28 PST
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.
Comment 29 Jakob Petsovits 2010-03-01 14:00:21 PST
Thanks Dan, I really appreciate it :)
Comment 30 WebKit Commit Bot 2010-03-01 16:56:11 PST
Comment on attachment 49746 [details]
Patch

Clearing flags on attachment: 49746

Committed r55387: <http://trac.webkit.org/changeset/55387>
Comment 31 WebKit Commit Bot 2010-03-01 16:56:18 PST
All reviewed patches have been landed.  Closing bug.