[webkit-reviews] A patch I have!
John Sullivan
sullivan at apple.com
Thu Jun 9 10:52:25 PDT 2005
Thanks Devin. This is definitely along the right path, but needs a
little bit more adjustment before it's ready to land. Here are my
comments:
1) You really do need all three of those #imports to compile.
However, in general it's better to avoid putting the #imports in
header files, since that can cause unnecessary recompilation, thus
making builds slower. It's better form to just declare the classes
and even protocols that are mentioned in the header file itself using
@class and @protocol, and then to put the #imports into the
implementation file.
In this case, WebFrame and WebView are needed by WebPDFView.m to
compile with your changes, but these classes aren't mentioned in
WebPDFView.h, so you can just move those two #imports to the list of
#imports at the top of WebPDFView.m. WebDocumentInternal.h defines
the protocol that the patch adds to WebPDFView, so it is also needed.
You could leave the #import of WebDocumentInternal.h in WebPDFView.h,
since the _web_WebDocumentTextSizing protocol is now mentioned in
WebPDFView.h. This is the style WebTextView.h uses. However, even in
this case I'd recommend moving the #import into WebPDFView.m, and
adding "@protocol _web_WebDocumentTextSizing" to WebPDFView.h, just
below the two @class lines that are already in that file.
2) You don't need any of the casts in your new code. E.g., this line:
WebView *view = (WebView *)[(WebFrame *)frame webView];
should just be:
WebView *view = [frame webView];
In this example, "frame" was declared as a WebFrame*, so there's no
need to cast it when using it. And the return value of -[WebFrame
webView] is declared as a WebView*, so there's no need to cast that
either.
3) Your change as is works great for changing the zoom level of a PDF
that you are currently viewing. But it doesn't handle the case where
the webview's textSizeMultiplier is set to something other than 1.0
before the PDF is displayed. For example, if you search in Google for
"really interesting pdf", then use command-plus to enlarge the text
in the Google search results, then click on one of the PDF links in
the Google search results, the PDF will come up at normal size, but
it should come up zoomed in, since the "Make Text Bigger/Smaller"
value is per-webview, not per-webpage. To fix this, you need to call
the same code that you've got here in _web_textSizeMultiplierChanged
from inside the method -[WebPDFView setDataSource:]. (You'll see that
WebTextView.m does this also.) So you should put your current code
into a new method internal to WebPDFView.m, and then call that method
from both setDataSource: and _web_textSizeMultiplierChnaged.
4) It's something of a coincidence (as you mentioned) that the
scaleFactor of a PDF uses the same range as the textSizeMultiplier of
a WebView. So I'd recommend mentioning this in a comment, something
along the lines of:
// The scale factor and text size multiplier conveniently use
the same units, so we can just
// treat the values as interchangeable.
[PDFSubview setScaleFactor:[view textSizeMultiplier]];
5) There is some possibility that your new code could be called when
view is nil. I'm not sure exactly when/if this would happen, but a
comment in the similar code in WebTextView.m says it can happen. if
view were nil, then [view textSizeMultiplier] would have an undefined
result (see http://wincent.com/a/knowledge-base/archives/2004/12/
messaging_nil.php). So to be safe you should check that view is non-nil:
if (view != nil) {
[PDFSubview setScaleFactor:[view textSizeMultiplier]];
}
6) There's no need for the frame local variable at all, since it's
only used once and the expression that returns it is nice and short.
All things being equal, it's a good practice to avoid unnecessary
local variables because they can lead to maintenance trouble later.
So I'd change
> WebFrame *frame = [(WebDataSource *)dataSource webFrame];
> WebView *view = (WebView *)[(WebFrame *)frame webView];
to
WebView *view = [[dataSource webFrame] webView];
7) There is no number seven.
I'm really glad you dived into this change, because this is a pet
peeve of many people. Please send another patch that takes my
comments into account and I'll land it.
John
On Jun 9, 2005, at 10:16 AM, Devin Lane wrote:
> John:
>
> Thanks for the reply! I went ahead and did some looking and some
> coding, and implemented it the way you wanted. It's pretty
> convenient how the textSizeMultiplier matches up perfectly with the
> PDFView's scaling. (1.0 for 100% in both cases) Although I
> couldn't find a way to adjust the range that the text can be
> resized within, the range seems to provide ample zoom levels for
> all but the most demanding users.
>
> Below is the patch file:
> <patch.txt>
>
> Also, I'm not two happy about having to include 3 more files just
> to get it to compile. I couldn't get gcc to figure out about the
> webView and textSizeMultiplier methods without importing the
> headers for WebView and WebFrame, and the WebDocumentInternal is
> for the protocol.
>
> Hopefully, this is a closer match to what you were looking for.
>
> Thanks,
>
> Devin Lane
> Cocoa Programmer
> Phone: (503) 775-4906
> E-Mail: devin at devserver.org
>
> On Jun 8, 2005, at 9:19 PM, John Sullivan wrote:
>
>> In general we are less likely to accept patches for adding UI
>> features than we are to accept patches for website compatibility
>> issues, because there are more design tradeoffs involved with UI
>> features and they need to be vetted across a wider set of people.
>>
>> In this particular case, it's arguable whether the lack of support
>> for keyboard zooming in PDFs is a missing feature or a bug. We've
>> had a number of requests for the PDF view in Safari to support the
>> existing Make Text Bigger and Make Text Smaller commands for
>> zooming. This is not a perfect match, because zooming in PDFs
>> changes the size of images as well as text, whereas these commands
>> in HTML only affect text size, not image size. Even so, I think
>> it's a close enough match that it's a clear improvement to make
>> these commands affect PDF pages. These commands are built into
>> WebKit so that any client could choose to implement UI for them;
>> they have the keyboard equivalents of command-plus and command-
>> minus in Safari.
>>
>> So I do think the general idea of your patch is something we want
>> to do in WebKit. However, the implementation in your patch isn't
>> the best way to do it, because it's disconnected from the existing
>> WebKit feature. The approach I'd like to see here is to make
>> PDFView implement the _web_WebDocumentTextSizing protocol that's
>> defined in WebDocumentInternal.h. That way, the existing menu and
>> toolbar items for text sizing in Safari (and other WebKit clients
>> that have implemented UI for this feature) will work for PDFs also.
>>
>> There are two implementations of this protocol in WebKit currently
>> that you could look at as models for how to do it in PDFView. One
>> is in WebHTMLView, the other is in WebTextView. I haven't looked
>> at the PDF code myself enough yet to know how straightforward it
>> will be to map WebKit's "text size multiplier" concept to PDF's
>> "zoom level" concept, but that mapping would be the heart of the
>> work to do this feature.
>>
>> I'm pretty likely to do this work at some point if nobody beats me
>> to it, but feel free to beat me to it!
>>
>> John
>>
>> On Jun 7, 2005, at 11:49 PM, Devin Lane wrote:
>>
>>> Reviewers:
>>>
>>> I'm not sure if you are accepting patches for non-bugs, but I
>>> made a patch that adds the ability for the plus and minus keys to
>>> control the zoom level of a PDF that is being viewed in Safari.
>>>
>>> Here is the patch file:
>>>
>>> <zoompatch.txt>
>>>
>>> Since this is my first patch, I'm not at all expecting it to be
>>> included in the actual source, but I thought I would pass it
>>> along just to see if you were interested.
>>>
>>> I did not preform any regression tests, since this patch affects
>>> neither the renderer or the javascript engine.
>>>
>>> Let me know if you have any questions,
>>>
>>> Devin Lane
>>> Cocoa Programmer
>>> Editor-in-Chief of Tech Pep (Benson H.S. Newspaper)
>>> Phone: (503) 775-4906
>>> E-Mail: devin at devserver.org
>>>
>>> _______________________________________________
>>> webkit-reviews mailing list
>>> webkit-reviews at opendarwin.org
>>> http://www.opendarwin.org/mailman/listinfo/webkit-reviews
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.macosforge.org/pipermail/webkit-reviews/attachments/20050609/39fbccdb/attachment.html
More information about the webkit-reviews
mailing list