[webkit-reviews] A patch I have!

Devin Lane devin at devserver.org
Mon Jun 13 20:19:29 PDT 2005


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!


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