[webkit-reviews] A patch I have!
Devin Lane
devin at devserver.org
Mon Jun 13 20:19:29 PDT 2005
John:
Jeeze! Sorry for the huge delay. I thought I didn't get a reply,
that's why I sent it again. It just occurred to me today (4 days
later) that I had setup a rule that was putting all webkit-reviews
messages in a folder that wasn't at all obvious. In short, I got both
of your replies, but I couldn't see them....
Anyway, I've made the changes you wanted. Sorry for my n00bish
programming skills, but thanks for teaching me the right way to do
it! I aim to become a great software engineer some day (Hopefully
for Apple!), and any tips I get along the way are surely appreciated!
Un oh. However, I can't get webkit to compile anymore, it fails with
a linker error somewhere in WebCore. Since I didn't change anything
in there, I'm guessing that some other change has broken CVS.
In short, I have no way of testing my code, but I'm pretty sure it'll
work. I'll send along a patch anyway in case somehow my webkit is
just busted.

Let me know if there are more things I'll need to correct!
Thanks,
Devin Lane
Cocoa Programmer
Editor-in-Chief of Tech Pep (Benson H.S. Newspaper)
Phone: (503) 775-4906
E-Mail: devin at devserver.org
On Jun 9, 2005, at 10:52 AM, John Sullivan wrote:
> 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 --------------
Skipped content of type multipart/mixed
More information about the webkit-reviews
mailing list