[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]

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?