[Webkit-unassigned] [Bug 27511] Add WinCE specific platform/graphics files to WebCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27511





--- Comment #77 from Eric Seidel <eric at webkit.org>  2009-08-10 07:55:24 PDT ---
(In reply to comment #76)
> (In reply to comment #69)
> > (From update of attachment 34394 [details] [details])
> All ports use BitmapImage class, including WINCE port. But platform image type
> (defined as "NativeImagePtr") is still required. On Windows platform, it can be
> HBITMAP. SharedBitmap is a replacement of HBITMAP, and is more flexible and
> useful than HBITMAP.
> 
> I don't think this basic acknowledge of WebCore should go to ChangeLog. I've
> mentioned NativeImagePtr in there, and I think people who are familiar with
> WebCore image stuff can understand it with no problem.

I'm about as familiar with WebKit Images as you can expect from a reviewer. :) 
I've worked on the Image stuff several times, including the most recent
re-factoring splitting out BitmapImage from Image.   Why does WinCE need a
NativeImagePtr Image wrapper and other ports don't?  I guess other ports use
BitmapImage to wrap their NativeImagePtr, no?

> If we need more description to help new webkit developers understand webkit
> source code, ChangeLog is not a good place to hold it.

Agreed.  The ChangeLog is about making your change understandable to those
reviewing it and later people reading it.  If the code itself needs explanation
that needs to go in the code.

ChangeLogs are in part about making your reviews obviously correct to those
reviewing them.  Anything that seems non-obvious should be explained in plain
english.

Non-obvious reviews tend to get r-'d or more likely just ignored until they
rot.  As others (more famous than I) have noted, ones code is read many more
times than it is written, and it's important to make it obviously correct to
each of those people reading it. :)  This is part of why we have reviews and
style guidelines.

I think we need to move these into separate bugs given how much parallel
discussion is going on.  (Small bugs, with small patches is part of trying to
make your patches obviously correct for quick review.  Once bugs or patches get
to a certain size, reviewers tend to ignore them or r- them.)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.