Bring the Mac Chromium render theme up to date and easier to keep merged. DO ...

Bring the Mac Chromium render theme up to date and easier to keep merged. DO NOT COMMIT; for upstreaming.

BUG=http://crbug.com/19604
TEST=no visible change on Leopard, draws correctly on Snow Leopard

[Avi (use Gerrit), 2009-08-21 19:15:35]

Chromium side of things.

The diff looks like hash because I threw away our current version, took the
current RenderThemeMac, and reapplied applicable parts. Some comments:

- There was a switch from Calibrated RGB to Device RGB (see
http://trac.webkit.org/changeset/38671). We'd branched before the change; I took
it but am not sure that this is correct.
- The drawing seems to be correct even without the sprinkling of
LocalCurrentGraphicsContexts everywhere.
- It looks like the future is platform/Theme.h rather than the old RenderTheme
stuff. There are a few functions that I needed to share. It's a bit ugly but if
RenderThemes are going away anyway...

To review this well, you might want to look at the diff of the new files to
RenderThemeMac (and platform/ThemeMac). The changes will be much clearer.

[Amanda Walker, 2009-08-21 19:57:54]

Looks very promising overall.  Two main questions:
- Does this affect layout test results?
- Do we need to do something different with the media player support, since we
aren't using QuickTime for it?

http://codereview.chromium.org/174242/diff/1005/22
File rendering/RenderThemeChromiumMac.mm (right):

http://codereview.chromium.org/174242/diff/1005/22#newcode1502
Line 1502: static int mediaControllerTheme()
Since Chromium's media support doesn't use QuickTime, do we need this?

[Avi (use Gerrit), 2009-08-21 20:01:14]

On 2009/08/21 19:57:54, Amanda Walker wrote:
> - Does this affect layout test results?

On a different client than usual; still checking them out. I hope not but dread
that they might. Stay tuned.

> - Do we need to do something different with the media player support, since we
> aren't using QuickTime for it?

No clue.

[Avi (use Gerrit), 2009-08-21 20:01:44]

http://codereview.chromium.org/174242/diff/1005/22
File rendering/RenderThemeChromiumMac.mm (right):

http://codereview.chromium.org/174242/diff/1005/22#newcode1502
Line 1502: static int mediaControllerTheme()
On 2009/08/21 19:57:54, Amanda Walker wrote:
> Since Chromium's media support doesn't use QuickTime, do we need this?

Probably not.

[Avi (use Gerrit), 2009-08-21 20:04:43]

Well, it's an open question at this point. I don't know enough to know how to
draw what we want. If it compiles, then we can deal with drawing themed media
controls later.

(That function is used by a whole bunch of media control themed drawing later.)

[Amanda Walker, 2009-08-21 20:07:02]

On 2009/08/21 20:04:43, Avi wrote:
> (That function is used by a whole bunch of media control themed drawing
later.)

Understood.  It just seems wrong to be testing for specific versions of
QuickTime, which we don't use.

[Avi (use Gerrit), 2009-08-21 21:23:44]

On 2009/08/21 20:07:02, Amanda Walker wrote:
> On 2009/08/21 20:04:43, Avi wrote:
> > (That function is used by a whole bunch of media control themed drawing
> later.)
> 
> Understood.  It just seems wrong to be testing for specific versions of
> QuickTime, which we don't use.

Layout tests are done. Tons are failing due to scrollbar mismatches. Still need
to play with things, then.

[John Grabowski, 2009-08-24 16:39:09]

On 2009/08/21 21:23:44, Avi wrote:
> On 2009/08/21 20:07:02, Amanda Walker wrote:
> > On 2009/08/21 20:04:43, Avi wrote:
> > > (That function is used by a whole bunch of media control themed drawing
> > later.)
> > 
> > Understood.  It just seems wrong to be testing for specific versions of
> > QuickTime, which we don't use.
> 
> Layout tests are done. Tons are failing due to scrollbar mismatches. Still
need
> to play with things, then.

As discussed in IRC, my main concern is that layout tests still pass.  As
discussed in our meeting, this could be a big time sink.  

As a secondary point, note that TestShell runs this in the main thread.  Make
sure that Chromium still works with this change when rendering:
- a bunch of native controls (checkbox, button, etc)
- the "misspell red underline"

[Avi (use Gerrit), 2009-09-10 14:48:21]

On 2009/08/24 16:39:09, John Grabowski wrote:
> my main concern is that layout tests still pass.

They do now!

> - a bunch of native controls (checkbox, button, etc)

All good to go!

> - the "misspell red underline"

Not a prbolem!

For your consideration.

[Amanda Walker, 2009-09-10 14:57:42]

LGTM, though the fact that we still have to keep these forked makes me sad.

[Mark Mentovai, 2009-09-10 15:08:16]

http://codereview.chromium.org/174242/diff/2001/2003
File platform/chromium/ThemeChromiumMac.mm (right):

http://codereview.chromium.org/174242/diff/2001/2003#newcode44
Line 44: extern NSRect IntRectToNSRect(const IntRect & rect);
I would prefer seeing this declared in some header.  Having to do this is
marginally hokey.

http://codereview.chromium.org/174242/diff/2001/2003#newcode45
Line 45: extern NSView* FlippedView();
I don't see this being used in this file.  Remove it from here, make the
"utility functions" comment singular, and mark the method static over in the
other file?

http://codereview.chromium.org/174242/diff/2001/2005
File rendering/RenderThemeChromiumMac.mm (right):

http://codereview.chromium.org/174242/diff/2001/2005#newcode129
Line 129: // |operator NSRect()| on WebCore::IntRect and FloatRect. This
substitutes for
Maybe we want to make those operators effective for CHROMIUM && DARWIN now too.

http://codereview.chromium.org/174242/diff/2001/2005#newcode152
Line 152: static NSView* view;
You can just use

static NSView* view = [[RTCMFlippedView alloc] init];
return view;

the "if uninitialized" check is implied.

[Avi (use Gerrit), 2009-09-10 15:28:28]

Amanda: "the fact that we still have to keep these forked makes me sad." ... me
too ...

http://codereview.chromium.org/174242/diff/2001/2003
File platform/chromium/ThemeChromiumMac.mm (right):

http://codereview.chromium.org/174242/diff/2001/2003#newcode44
Line 44: extern NSRect IntRectToNSRect(const IntRect & rect);
On 2009/09/10 15:08:16, Mark Mentovai wrote:
> I would prefer seeing this declared in some header.  Having to do this is
> marginally hokey.

I know. Can we include ThemeChromiumMac.h from RenderThemeChromiumMac.h or vice
versa? I'm not sure of the dependency rules of WebCore.

http://codereview.chromium.org/174242/diff/2001/2003#newcode45
Line 45: extern NSView* FlippedView();
On 2009/09/10 15:08:16, Mark Mentovai wrote:
> I don't see this being used in this file.

Look harder. Lines 229, 307, and 397.

http://codereview.chromium.org/174242/diff/2001/2005
File rendering/RenderThemeChromiumMac.mm (right):

http://codereview.chromium.org/174242/diff/2001/2005#newcode129
Line 129: // |operator NSRect()| on WebCore::IntRect and FloatRect. This
substitutes for
On 2009/09/10 15:08:16, Mark Mentovai wrote:
> Maybe we want to make those operators effective for CHROMIUM && DARWIN now
too.

I don't understand. They're only for use in this particular file, so whoever
decides that they want to use this file picks them up.

http://codereview.chromium.org/174242/diff/2001/2005#newcode152
Line 152: static NSView* view;
On 2009/09/10 15:08:16, Mark Mentovai wrote:
> You can just use
> 
> static NSView* view = [[RTCMFlippedView alloc] init];
> return view;
> 
> the "if uninitialized" check is implied.

Done.

[Mark Mentovai, 2009-09-10 16:30:05]

avi@chromium.org wrote:
> http://codereview.chromium.org/174242/diff/2001/2005#newcode129
> Line 129: // |operator NSRect()| on WebCore::IntRect and FloatRect. This
> substitutes for
> On 2009/09/10 15:08:16, Mark Mentovai wrote:
>>
>> Maybe we want to make those operators effective for CHROMIUM && DARWIN
>
> now too.
>
> I don't understand. They're only for use in this particular file, so
> whoever decides that they want to use this file picks them up.

I meant that maybe we want our versions of IntRect and FloatRect to
support operator NSRect() so that we can eschew the custom conversion
function and be one step closer to the Mac-non-Chromium theme
implementations.

Mark

[John Grabowski, 2009-09-10 17:57:32]

Please add more comments:
- explain in the code where these came from (e.g. "copied from BLAH.mm")
- explain in the code why we need our own version
- briefly explain in the code the differences from the original version

Other than that LGTM.

jrg

On Thu, Sep 10, 2009 at 7:48 AM, <avi@chromium.org> wrote:

> On 2009/08/24 16:39:09, John Grabowski wrote:
>
>> my main concern is that layout tests still pass.
>>
>
> They do now!
>
>  - a bunch of native controls (checkbox, button, etc)
>>
>
> All good to go!
>
>  - the "misspell red underline"
>>
>
> Not a prbolem!
>
> For your consideration.
>
> http://codereview.chromium.org/174242
>

[Avi (use Gerrit), 2009-09-10 21:46:20]

Changes to put comments in, put rect conversion back on the rects themselves.

[Avi (use Gerrit), 2009-09-17 19:17:42]

Landed upstream. See https://bugs.webkit.org/show_bug.cgi?id=29243 .