[webkit-reviews] A patch I have!

Devin Lane devin at devserver.org
Tue Jun 14 11:24:03 PDT 2005


John:

Thanks!

In response:

1.  My mistake -- I misunderstood your corrections.
2.  I have always gotten a compiler warning when I call a method that  
isn't declared anywhere, even if it's in the same class.  Since I  
noticed that that warnings were counted as errors, and thus caused a  
build failure, I decided to declare the method in a category.  Does  
the warning not happen with GCC 4.0? Does the warning not happen at  
all, and I just got some other weird thing?
3.  Ah, I have my tab width set on 4, because it makes the source  
look better to me.  Shouldn't it appear as 8 to you?  Perhaps somehow  
it got rest to use spaces.

No problem!  Happy I was help, especially with such a prestigious  
project!

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 14, 2005, at 11:16 AM, John Sullivan wrote:

> I was suspicious that you might have some mail trouble going on.  
> I'm glad you figured it out.
>
> I'm going to land your new patch with a few tweaks:
>
> 1) You declared @class WebView and @class WebFrame in the .h file  
> unnecessarily (probably you got confused by my earlier  
> explanation), so I removed those.
> 2) You declared the new updateScalingToReflectTextSize method in a  
> category unnecessarily. Since both callers were in the same  
> implementation block, just placing the new method somewhere in the  
> implementation block above the first caller is sufficient.
> 3) I renamed updateScalingToReflectTextSize to  
> _updateScalingToReflectTextSize. The convention in objective-C  
> frameworks at Apple is to in general start all non-public method  
> names with an underscore.
> 4) A couple of the new lines were indented too far in comparison  
> with the existing code, possibly due to your tab settings in Xcode.  
> We have "Insert 'tabs' instead of spaces" off, a tab width of 8,  
> and an indent width of 4, which helps make these discrepancies  
> stand out.
>
> Thanks again for getting this done.
>
> John
>
> On Jun 13, 2005, at 8:19 PM, Devin Lane wrote:
>
>> 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.
>> <patch.txt>
>>
>> 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 --------------
An HTML attachment was scrubbed...
URL: http://lists.macosforge.org/pipermail/webkit-reviews/attachments/20050614/31c1f104/attachment.html


More information about the webkit-reviews mailing list