[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