[webkit-reviews] review denied: [Bug 4091] PDF views should keep a
separate scaling factor from shared text scaling factor :
[Attachment 3040] proposed patch - also fixed 4015
bugzilla-request-daemon at opendarwin.org
bugzilla-request-daemon at opendarwin.org
Thu Jul 21 09:43:28 PDT 2005
John Sullivan <sullivan at apple.com> has denied Trey Matteson <trey at usa.net>'s
request for review:
Bug 4091: PDF views should keep a separate scaling factor from shared text
scaling factor
http://bugzilla.opendarwin.org/show_bug.cgi?id=4091
Attachment 3040: proposed patch - also fixed 4015
http://bugzilla.opendarwin.org/attachment.cgi?id=3040&action=edit
------- Additional Comments from John Sullivan <sullivan at apple.com>
You convinced me that trying to use zoom factor and text scaling
interchangeably has UI problems that are worse than the UI problems of reusing
the Make Text Smaller/Larger menu items in a related but non-interdependent
way. So I think this approach is great, and the code looks really good too. I
just have a few comments and questions, so I'm marking it review- for this
round.
Typo in ChangeLog: "Sace scrollState"
+ // Parent is the clipview of the DynamicScrollView the
WebFrame installs
+ point = [parent bounds].origin;
I'd add an ASSERT here to match the comment. I realize that this code didn't
change, but as the surrounding code gets more complex I think the ASSERT is a
useful safety measure.
It looks like the WebHistoryItem private data structure now has a scrollPoint
item and a viewState item that contains a scrollPoint item. Can the standalone
scrollPoint item be eliminated, and the viewState always used?
What's the story with removing these lines:
-
- at interface WebHTMLView (TextSizing) <_web_WebDocumentTextSizing>
- at end
-
We're not very consistent with our naming of internal-to-webkit protocols, but
I think that _WebDocumentViewState should be _web_WebDocumentViewState to match
_web_WebDocumentTextSizing.
With this patch, when are "Make Text Larger/Smaller" dimmed in the menu when
viewing a PDF flie?
More information about the webkit-reviews
mailing list