Hi Roman,
here are some comments on the patch.
1) src/share/classes/java/awt/peer/CheckboxMenuItemPeer.java
+import java.awt.CheckboxMenuItem;
if that's really needed? I thought that [EMAIL PROTECTED] ...} doesn't require such
stuff.
The same for the following classes:
src/share/classes/java/awt/peer/ChoicePeer.java file.
src/share/classes/java/awt/peer/FileDialogPeer.java
src/share/classes/java/awt/peer/FramePeer.java
src/share/classes/java/awt/peer/LabelPeer.java
src/share/classes/java/awt/peer/MenuComponentPeer.java
src/share/classes/java/awt/peer/MenuItemPeer.java
src/share/classes/java/awt/peer/MenuPeer.java
src/share/classes/java/awt/peer/PopupMenuPeer.java
src/share/classes/java/awt/peer/ScrollPanePeer.java
src/share/classes/java/awt/peer/SystemTrayPeer.java
src/share/classes/java/awt/peer/TextAreaPeer.java
src/share/classes/java/awt/peer/TextComponentPeer.java
src/share/classes/java/awt/peer/TextFieldPeer.java
src/share/classes/java/awt/peer/TrayIconPeer.java
2) src/share/classes/java/awt/peer/ContainerPeer.java
There is a typo in the second word:
- * Indicates availabiltity of restacking operation in this container.
+ * Indicates availability of restacking operation in this container.
3) Common thing.
<code>true</code> have a shorter synonym since JDK 5: [EMAIL PROTECTED] true}
4) src/share/classes/java/awt/peer/RobotPeer.java
I noticed that you prefer not to leave "public" modifier in an
interface. But here you are leaving all of them.
5) src/share/classes/java/awt/peer/RobotPeer.java
public int getNumberOfButtons(); has left w/o any comments.
6) src/share/classes/java/awt/peer/ComponentPeer.java
+ /**
+ * Called by [EMAIL PROTECTED] EventQueue#coalescePaintEvent} to let the
component
+ * peer coalesce paint events.
+ *
+ * @param e the paint event to consider to coalesce
+ *
+ * @see EventQueue#coalescePaintEvent
+ */
+ void coalescePaintEvent(PaintEvent e);
I'd say it's not "to let .... coalesce paint events", but "to coalesce
paint events".
7) at the same class.
You wrote:
+ // TODO: Maybe change this to force Graphics2D, since many things will
+ // break with plain Graphics nowadays.
+ Graphics getGraphics();
Do you know a scenario to show what's exactly might be broken. We
probably need to introduce another peer for that, right?
8) Similar to 7):
+ // TODO: Maybe make that return a BufferedImage, because some stuff
will
+ // break if a different kind of image is returned.
+ Image createImage(int width, int height);
The rest is just fine!
Finally I've applied the patch and reviewed the webrev. It was just more
convenient for me.
There was an exception for ComponentPeer.java - it caused some conflicts
and I didn't succeeded to resolve them fast enough so just looked on the
patch itself for that file.
At last the JDK7 workspace was successfully built.
Thanks,
Andrei
Roman Kennke wrote:
Hi,
I found that reasonable since the very first time evaluated the cacio
code. Though this patch probably requires more time to get reviewed...
So? We do it in a separate review&push or we put both (the peer cleanup
and the documentation) together?
/Roman
Thanks,
Andrei
Roman Kennke wrote:
Hi Andrei,
Yeah sure. I reviewed all the existing implementations and they should
not break, but a lot of stuff could be removed.
Just appeared to public access:
http://bugs.sun.com/view_bug.do?bug_id=6749920
Cool. BTW, I have another patch for the peer interfaces in the Cacio
repo, that one adds API docs to all the interfaces. Nothing serious,
only roughly what it does (this might be obvious by the method names
anyway) and pointers to where it is used in AWT (I found this important
for implementing the peers, so that people can look up the nitty gritty
details themselves). In the projects I was involved in so far we had
policies to separate code changes from doc and formatting changes, but I
don't know how this is handled in OpenJDK. Maybe we want to push these
together. Find this doc patch attached (this follows up on the
cleanpeers patch, will not apply on clean OpenJDK!)
Cheers, Roman
