Bug 29243 – Update Chromium Mac render theme files

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Avi Drissman]

Created attachment 39542 [details]
The patch

The existing Chromium Mac render theme code is forked from an ancient version of RenderThemeMac. This patch brings that file up to date with the current version for ease of maintenance. (To be investigated: determining if we can merge the two files so they don't have to be kept in sync.)

Please see http://codereview.chromium.org/174242 for the initial review.

[Avi Drissman]

A quick comment: the diff is complete hash. It's best to compare a patched RenderThemeChromiumMac to RenderThemeMac.

[Avi Drissman]

Created attachment 39543 [details]
Updated patch; the .gypi file caught unwanted changes

[Eric Seidel (no email)]

FYI, general behavior seems to be that you can mark cq=? if you want to make sure the reviewer sets cq+ when reviewing.

[Dimitri Glazkov (Google)]

Comment on attachment 39543 [details]
Updated patch; the .gypi file caught unwanted changes

Can you please:

* include bug URL
* use svn cp to keep the history and minimize the amount of perceived changes made?

[Eric Seidel (no email)]

prepare-ChangeLog --bug 29243
will automatically fill in the bug stuff for you.  see prepare-ChangeLog --help

[Avi Drissman]

Created attachment 39601 [details]
New patch

[Avi Drissman]

(In reply to comment #4)
> (From update of attachment 39543 [details])
> Can you please:
> 
> * include bug URL
> * use svn cp to keep the history and minimize the amount of perceived changes
> made?

Can you keep the svn cp status through a patch? I didn't think it was possible and would need someone to help me land this anyway.

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 39543 [details] [details])
> > Can you please:
> > 
> > * include bug URL
> > * use svn cp to keep the history and minimize the amount of perceived changes
> > made?
> 
> Can you keep the svn cp status through a patch? I didn't think it was possible
> and would need someone to help me land this anyway.

No, but it makes reviewing and landing it a heckuva lot easier :). If you're looking for a rubberstamp, you've got it. But whoever lands this would have to cry tears of pain trying to svn cp, then apply this patch.

[Avi Drissman]

Created attachment 39603 [details]
Patch with svn cp used to mark origins of files

OK, used svn cp to mark files' origins.

[Eric Seidel (no email)]

Doesn't svn-create-patch store copy information?  I thought it did.  Also, if there are any tears-of-pain needed, then the commit-queue won't be of use to you, because it doesn't know how to cry.  It just knows how to make you cry when it rejects your patch. :)

[Darin Fisher (:fishd, Google)]

Comment on attachment 39603 [details]
Patch with svn cp used to mark origins of files

> Index: WebCore/ChangeLog
...
> +        http://crbug.com/19604

No need for the crbug reference since you mention the bugs.webkit.org link.


> Index: WebCore/platform/graphics/FloatPoint.h
> +#if (PLATFORM(MAC) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES)) \
> +      || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

nit: indent by 8 spaces instead of 6.  normal indent is 4 spaces, so 2x that
seems appropriate for a line continuation.


> Index: WebCore/platform/graphics/FloatRect.h
...
> +#if (PLATFORM(MAC) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES)) \
> +      || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

ditto


> Index: WebCore/platform/graphics/FloatSize.h
...
> +#if (PLATFORM(MAC) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES)) \
> +      || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

ditto


> Index: WebCore/platform/graphics/IntRect.h
...
> +#if (PLATFORM(MAC) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES)) \
> +      || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))
...
> +#if (PLATFORM(MAC) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES)) \
> +      || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

ditto

[Avi Drissman]

Created attachment 39605 [details]
Update for fishd's comments

[Dimitri Glazkov (Google)]

Comment on attachment 39605 [details]
Update for fishd's comments

Needs downstream change.

[Avi Drissman]

(In reply to comment #13)
> (From update of attachment 39605 [details])
> Needs downstream change.

Dmitri, I can coordinate with the WebKit gardener. Can we land this?

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/48469.

[Avi Drissman]

Created attachment 39708 [details]
Quick fix to make it compile

Oops, there was a line that referred to a non-existent member in the media theme drawing routines. The media drawing is TBD, so for now we're just pulling it out.

[Dimitri Glazkov (Google)]

Comment on attachment 39708 [details]
Quick fix to make it compile

r=me.

[Dimitri Glazkov (Google)]

Fix landed as http://trac.webkit.org/changeset/48481.