Bug 35347 - Convert the zoom mode into a proper enum
: Convert the zoom mode into a proper enum
Status: RESOLVED FIXED
: WebKit
Frames
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-02-24 08:50 PST by
Modified: 2010-03-01 16:56 PST (History)


Attachments
Convert the zoom mode into a proper enum (18.43 KB, patch)
2010-02-24 08:56 PST, Jakob Petsovits
manyoso: review-
Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Patch (26.28 KB, patch)
2010-02-25 09:36 PST, Daniel Bates
abarth: review-
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (27.93 KB, patch)
2010-03-01 13:07 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-02-24 08:56:33 PST -------
Created an attachment (id=49401) [details]
Convert the zoom mode into a proper enum
------- Comment #2 From 2010-02-24 09:12:40 PST -------
(From update of attachment 49401 [details])
> +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 From 2010-02-24 09:16:24 PST -------
Created an attachment (id=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 From 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 From 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 From 2010-02-24 10:42:47 PST -------
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.
------- Comment #7 From 2010-02-24 10:52:32 PST -------
(In reply to comment #6)
> Created an attachment (id=49410) [details] [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 From 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 From 2010-02-24 10:59:30 PST -------
Created an attachment (id=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 From 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 From 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 From 2010-02-24 15:02:12 PST -------
Created an attachment (id=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 From 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 From 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 From 2010-02-25 08:30:00 PST -------
(From update of attachment 49446 [details])
> -    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 From 2010-02-25 09:15:15 PST -------
The mac-ews is stuck unable to build.  Eric, can you kick it?
------- Comment #17 From 2010-02-25 09:36:56 PST -------
Created an attachment (id=49498) [details]
Patch

Added ZoomMode.h to Xcode project and made change as per Darin's comment.
------- Comment #18 From 2010-02-25 09:38:40 PST -------
(From update of attachment 49498 [details])
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 From 2010-02-25 14:10:01 PST -------
The mac-ews bot has been kicked, but it has some back-log.
------- Comment #20 From 2010-02-26 06:52:18 PST -------
(From update of attachment 49498 [details])
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 From 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 From 2010-02-26 07:20:53 PST -------
So... where do we go from here?
------- Comment #23 From 2010-02-26 07:27:08 PST -------
(From update of attachment 49498 [details])
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 From 2010-02-27 02:09:13 PST -------
(From update of attachment 49498 [details])
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 From 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 From 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 From 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 From 2010-03-01 13:07:28 PST -------
Created an attachment (id=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 From 2010-03-01 14:00:21 PST -------
Thanks Dan, I really appreciate it :)
------- Comment #30 From 2010-03-01 16:56:11 PST -------
(From update of attachment 49746 [details])
Clearing flags on attachment: 49746

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