Bug 53575 – [chromium] Avoid #ifdefs in code that constructs a GraphicsContext by adding a helper class.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Fisher (:fishd, Google)]

[chromium] Avoid #ifdefs in code that constructs a GraphicsContext by
adding a helper class.

[Darin Fisher (:fishd, Google)]

Created attachment 80890 [details]
v1 patch

A couple notes about this patch:

1- This introduces the src/painting directory.  I plan to add some more files to that directory shortly.  I feel like src/ has reached the point where it badly needs subdirectories!  (This is just a start.)

2- GraphicsContextBuilder allocates a LocalCurrentGraphicsContext object on the Mac, even though this is not always needed.  I'm not too concerned about this.  I think the simplification to the code is worth it.

[Kenneth Russell]

Comment on attachment 80890 [details]
v1 patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80890&action=review

Looks like a good cleanup. One question that comes up in a few places.

> Source/WebKit/chromium/src/WebFontImpl.cpp:98
> +    GraphicsContextBuilder builder(canvas);

The new code implicitly instantiates a LocalCurrentGraphicsContext here; is there any issue with doing this?

> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:168
> +        m_widget->paint(&GraphicsContextBuilder(canvas).context(), rect);

Same question here.

> Source/WebKit/chromium/src/WebScrollbarImpl.cpp:122
> +    m_scrollbar->paint(&GraphicsContextBuilder(canvas).context(), rect);

And here.

[Darin Fisher (:fishd, Google)]

> > Source/WebKit/chromium/src/WebFontImpl.cpp:98
> > +    GraphicsContextBuilder builder(canvas);
> 
> The new code implicitly instantiates a LocalCurrentGraphicsContext here; is there any issue with doing this?

See point 2 in comment #1 :-)

[Kenneth Russell]

(In reply to comment #3)
> > > Source/WebKit/chromium/src/WebFontImpl.cpp:98
> > > +    GraphicsContextBuilder builder(canvas);
> > 
> > The new code implicitly instantiates a LocalCurrentGraphicsContext here; is there any issue with doing this?
> 
> See point 2 in comment #1 :-)

Oops, sorry, I didn't see that comment at all before looking at the patch.

[Kenneth Russell]

Comment on attachment 80890 [details]
v1 patch

I think painting/GraphicsContextBuilder.h should be added to webkit/sources in Source/WebKit/chromium/WebKit.gyp; looks good otherwise. Leaving cq? so you can decide whether to make this change upon landing.

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> (From update of attachment 80890 [details])
> I think painting/GraphicsContextBuilder.h should be added to webkit/sources in Source/WebKit/chromium/WebKit.gyp; looks good otherwise. Leaving cq? so you can decide whether to make this change upon landing.

Oops, yes.. I have that in my tree.  I just created the patch from the wrong level :-(

[Kenneth Russell]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 80890 [details] [details])
> > I think painting/GraphicsContextBuilder.h should be added to webkit/sources in Source/WebKit/chromium/WebKit.gyp; looks good otherwise. Leaving cq? so you can decide whether to make this change upon landing.
> 
> Oops, yes.. I have that in my tree.  I just created the patch from the wrong level :-(

No, as you pointed out to me I completely missed the fact that the gyp change was already in your patch. Sorry about that. Looks good to me.

[Darin Fisher (:fishd, Google)]

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

[James Robinson]

Comment on attachment 80890 [details]
v1 patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80890&action=review

> Source/WebKit/chromium/src/WebFontImpl.cpp:37
> +#include "painting/GraphicsContextBuilder.h"

Is this pattern intentional (including by paths rather than adding new crap to the include path)? I prefer this way myself but it's unusual for WebKit

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
> (From update of attachment 80890 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80890&action=review
> 
> > Source/WebKit/chromium/src/WebFontImpl.cpp:37
> > +#include "painting/GraphicsContextBuilder.h"
> 
> Is this pattern intentional (including by paths rather than adding new crap to the include path)? I prefer this way myself but it's unusual for WebKit

Yeah, it is intentional.  I was concerned that if a header file does not appear directly in src/ that someone might waste time looking for the header in WebCore/.  The painting/ prefix gives you a clue that it is actually local to WebKit/chromium/src/.  Also, there is also some convention for this.  We include {win,mac,linux}/WebThemeEngine.h in some places.