Bug 64754 – Rename FloatSize to GraphicsSize

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[Levi Weintraub]

The first class to be renamed (to cut down on patch size).

[Levi Weintraub]

Created attachment 101207 [details]
Patch

[WebKit Review Bot]

Attachment 101207 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebKit2/Shared/WebEvent.h:182:  The parameter name "phase" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/WebEvent.h:182:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 155 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Levi Weintraub]

Created attachment 101214 [details]
Patch

[Levi Weintraub]

Any comments/review would be much appreciated.

[Levi Weintraub]

(In reply to comment #4)
> Any comments/review would be much appreciated.

Ping?

[Levi Weintraub]

Simon, it'd be great if you could take a look at this.

[Darin Adler]

Renaming the class seems wrong. Is a graphics-system-specific floating point size the only case where we want a floating point size? At the moment we use FloatSize a lot in WebKit, and on Mac OS X under 64-bit the graphics system size would be a DoubleSize, not a FloatSize. I am not sure that renaming is the right way to go here; we might need both or to use typedefs.

[Simon Fraser (smfr)]

(In reply to comment #7)
> Renaming the class seems wrong. Is a graphics-system-specific floating point size the only case where we want a floating point size? At the moment we use FloatSize a lot in WebKit, and on Mac OS X under 64-bit the graphics system size would be a DoubleSize, not a FloatSize. I am not sure that renaming is the right way to go here; we might need both or to use typedefs.

I agree. I also think that we might want to use floats for layout, but CGFloat for GraphicsContext etc (i.e. double on 64-bit).

[Levi Weintraub]

(In reply to comment #7)
> Renaming the class seems wrong. Is a graphics-system-specific floating point size the only case where we want a floating point size? At the moment we use FloatSize a lot in WebKit, and on Mac OS X under 64-bit the graphics system size would be a DoubleSize, not a FloatSize. I am not sure that renaming is the right way to go here; we might need both or to use typedefs.

I wouldn't be particularly excited to create a whole new class for DoubleSize and such. At that point it seems like a template would be the answer, and we could always do something like:
typedef Size<float> FloatSize;
and maybe
typedef Size<CGFloat> GraphicsSize;
on Mac?

Any thoughts? I'm also not sure that moving the Int* classes into a set of template specializations would be a win for simplicity, but I'm open to the thought and willing to give it a try.

[Darin Adler]

Template classes tend to make function names much longer for things like the .exp files and debugging much less pleasant. If we can mitigate that issue, then I think a template is fine.

[Levi Weintraub]

Comment on attachment 101214 [details]
Patch

Sounds like this one isn't the answer. Taking off the r?. I'll give templating a try unless there's pushback.

[Levi Weintraub]

(In reply to comment #10)
> Template classes tend to make function names much longer for things like the .exp files and debugging much less pleasant. If we can mitigate that issue, then I think a template is fine.

I agree. It just *seems* cleaner than creating a clone of the Float* classes that just substitute Double... I'm honestly not sure how to make dealing with templates less of a pain though :(

[Levi Weintraub]

I wanted to have something like this:


#if (USE(CG) || (PLATFORM(WX) && OS(DARWIN)) || USE(SKIA_ON_MAC_CHROMIUM)) && defined(CGFLOAT_IS_DOUBLE)
    typedef DoubleRect GraphicsRect;
#else
    typedef FloatRect GraphicsRect;
#endif

But many graphics functions are exported, and we can't export a typedef that changed like that (that I know of?). How important is it really to convert the GraphicsContext to CGFloat on Mac platforms? It doesn't seem like this will grant us any more precision, but only scale up to doubles on 64 bit platforms one step earlier than it does currently.

[Darin Adler]

(In reply to comment #13)
> #if (USE(CG) || (PLATFORM(WX) && OS(DARWIN)) || USE(SKIA_ON_MAC_CHROMIUM)) && defined(CGFLOAT_IS_DOUBLE)
>     typedef DoubleRect GraphicsRect;
> #else
>     typedef FloatRect GraphicsRect;
> #endif
> 
> But many graphics functions are exported, and we can't export a typedef that changed like that (that I know of?).

On Mac we have #if statements in our exp files. And we have various techniques to deal with the fact that CGFloat is a typedef.

> How important is it really to convert the GraphicsContext to CGFloat on Mac platforms?

If that’s not what GraphicsRect means, then why not use FloatRect everywhere?

[Levi Weintraub]

We no longer are taking this tact.