[webkit-reviews] A patch I have!
John Sullivan
sullivan at apple.com
Tue Jun 14 11:16:40 PDT 2005
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/aa20340d/attachment.html
More information about the webkit-reviews
mailing list