[chromium] Avoid #ifdefs in code that constructs a GraphicsContext by adding a helper class.
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.
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.
> > 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 :-)
(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.
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.
(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 :-(
(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.
Landed as http://trac.webkit.org/changeset/77383
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
(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.