[WebKit-devel] Zoom actions change proposal (patch)

[Paweł Prażak]

Some comments:
> 0001-Intendation-cleanup:
> Note that such a change makres merging with the original file from KHTML
> harder, but it's okay for me since it improves readabilty.


Hmm, I understand. Is it possible to patch KHTML code to change this (I know
it's just a detail but I wouldn't want to debug this code ;)

0001-Zoom-actions-changes:
> * Are you sue the bool var m_zoomTextOnly is always initiated?


No, I'm not sure, but all of the settings are initialized the same way as I
see, so I assumed this is the working way. I don't know if this is a good,
idea but we can move all bit fields to union with an int and init the whole
union at once. But I'm not sure if it's necessary.

* "Zoom Normal" is not a very nice text IMHO. What about "Reset zoom"? Or
> what
> do you suggest?


"Reset Zoom" or "Zoom Reset" are good. I didn't gave much thought about the
name.
Maybe "Default Zoom", but reset is probably better. Is there a convention
established in KDE?

* Just thought about it: is it the usual way that the user sets an option
> for
> one tab, and it gets saved as a global setting? Probably you could add a
> new
> option in the HTML options, and respect that option? Though the issue would
> be
> that KHTML would not follow this setting... So not really sure how to
> handle
> that. Probably your way is the only really working way. Any thoughts,
> anyone?


FF have global setting as well, so it's not that bad ;) If we make this
setting per tab how do we save this? per domain? I don't know how other
people but I only use option to set text only zoom when I'm testing/writing
the code, so it's not a hot feature anyway. ;)

0002-Code-cleanup:
> This patch makes merging really harder. I also like nice code styling a
> lot,
> but I'm not sure if it is worth for loosing automatic merging...


Yes, now I understand why it looks how it looks. Now I see there is no point
in style cleanup without cleanup upstream.

 0003-Added-astyle-script-for-kdelibs-style.-Applyed-sctip.patch:
> Have you manually reviewed all changes? In some rare cases astyle styling
> is
> not the optimal thing.


No I didn't, but good to know that astyle isn't perfect. Anyway, I can try
to rewrite the script to force styling only on files that have no
imporeted/merged code (but I'm not sure if it makes sense).

Best Regards,
Paweł
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/webkit-devel/attachments/20090503/d3213942/attachment.htm