From GraphicsContext.h: GraphicsContextPrivate* m_common; GraphicsContextPlatformPrivate* m_data; // Deprecated; m_commmon can just be downcasted. To be removed.
Created attachment 75155 [details] Patch
Comment on attachment 75155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75155&action=review > WebCore/platform/graphics/GraphicsContext.h:445 > static GraphicsContextPrivate* createGraphicsContextPrivate(); > static void destroyGraphicsContextPrivate(GraphicsContextPrivate*); These two methods should be removed since they are no longer used/needed. > WebCore/platform/graphics/wx/GraphicsContextWx.cpp:139 > destroyGraphicsContextPrivate(m_common); > - delete m_data; > + delete platformPrivate(); This will crash! (Double delete)
Attachment 75155 [details] did not build on mac: Build output: http://queues.webkit.org/results/6461021
Attachment 75155 [details] did not build on gtk: Build output: http://queues.webkit.org/results/6328116
Comment on attachment 75155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75155&action=review Generally the patch would do it's job correctly, but I am doubtful about what do we win with that. As I see a renaming from GraphicsContextPrivate to GraphicsContextState and m_common to m_state would be enough to make the roles of those member clear. Could you give some arguments of the change in the ChangeLog? > WebCore/platform/graphics/GraphicsContext.h:-448 > GraphicsContextPrivate* m_common; > - GraphicsContextPlatformPrivate* m_data; // Deprecated; m_commmon can just be downcasted. To be removed. I think m_common should be renamed to m_data. > WebCore/platform/graphics/cg/GraphicsContextCG.cpp:114 > } > > GraphicsContext::GraphicsContext(CGContextRef cgContext) > - : m_common(createGraphicsContextPrivate()) > - , m_data(new GraphicsContextPlatformPrivate(cgContext)) > + : m_common(new GraphicsContextPlatformPrivate(cgContext)) > { > setPaintingDisabled(!cgContext); > if (cgContext) { After this change nobody should ever create a GraphicsContextPrivate object so it's constructor should be private (with a comment maybe). > WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:173 > + rgb_color oldColor = platformPrivate()->m_view->HighColor(); > + platformPrivate()->m_view->SetHighColor(color); > + platformPrivate()->m_view->FillRect(rect); > + platformPrivate()->m_view->SetHighColor(oldColor); platformPrivate() should be assigned to a local variable. I am not sure about what is the preferred style, but maybe you should do it in every case when you call a getter more than once in a function. Personally I am OK with calling it twice (but not more) because it is just an inline getter. > WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:296 > + float oldSize = platformPrivate()->m_view->PenSize(); > + platformPrivate()->m_view->SetPenSize(width); > + platformPrivate()->m_view->StrokeRect(rect, getHaikuStrokeStyle()); > + platformPrivate()->m_view->SetPenSize(oldSize); Ditto. > WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:317 > - m_data->m_view->SetLineMode(mode, m_data->m_view->LineJoinMode(), m_data->m_view->LineMiterLimit()); > + platformPrivate()->m_view->SetLineMode(mode, platformPrivate()->m_view->LineJoinMode(), platformPrivate()->m_view->LineMiterLimit()); Ditto. I will not repeat that observation in the rest of the patch.
(In reply to comment #5) > As I see a renaming from GraphicsContextPrivate to GraphicsContextState and m_common to m_state would be > enough to make the roles of those member clear. GraphicsContextPrivate has a member "GraphicsContextPrivate::GraphicsContextState state" so this would be rather confusing, e.g "m_state->state" > I think m_common should be renamed to m_data. Agreed. > platformPrivate() should be assigned to a local variable. I am not sure about what is the preferred style, but maybe you should do it in every case when > you call a getter more than once in a function. Personally I am OK with calling it twice (but not more) because it is just an inline getter. I don't know if there's a preferred style, but I agree that repeating platformPrivate() all over the place is rather ugly. :)
Comment on attachment 75155 [details] Patch Resetting r- as the patch has numerous issues. (Guess the re-r? was accidental.)
(In reply to comment #6) > (In reply to comment #5) > > As I see a renaming from GraphicsContextPrivate to GraphicsContextState and m_common to m_state would be > > enough to make the roles of those member clear. > > GraphicsContextPrivate has a member "GraphicsContextPrivate::GraphicsContextState state" so this would be rather confusing, e.g "m_state->state" That's true but the point is that we are trying to merge two different things into one class. Actually GraphicsContextPrivate is just data (no methods) so we could simply merge it into GraphicsContext and let GraphicsContextPlatformPrivate as it is.
(In reply to comment #8) > That's true but the point is that we are trying to merge two different > things into one class. Actually GraphicsContextPrivate is just data (no methods) > so we could simply merge it into GraphicsContext and let GraphicsContextPlatformPrivate > as it is. You are absolutely right, this would be a lot cleaner. :)
Created attachment 75370 [details] Patch This patch follows the previous suggestions. I merged all data from GraphicsContextPrivate into GraphicsContext and removed the m_common member.
Comment on attachment 75370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75370&action=review Good patch with not so good Changelog. > WebCore/ChangeLog:9 > + GraphicsContext: Merge m_common and m_data > + https://bugs.webkit.org/show_bug.cgi?id=49914 > + > + Move data members from GraphicsContextPrivate into GraphicsContext. So the m_common member became unnecessary. > + It is removed. Why? I know but the Changelog should contain arguments for the change. Don't sparing with the words in Changelog's! :)
Comment on attachment 75370 [details] Patch Removing r? since Reni is working on a new version of this patch.
Created attachment 75729 [details] Patch
Created attachment 75737 [details] Patch
Attachment 75737 [details] did not build on win: Build output: http://queues.webkit.org/results/6851058
(In reply to comment #15) > Attachment 75737 [details] did not build on win: > Build output: http://queues.webkit.org/results/6851058 3>..\platform\graphics\win\GraphicsContextCGWin.cpp(65) : error C2511: 'void WebCore::GraphicsContext::platformInit(HDC,bool)' : overloaded member function not found in 'WebCore::GraphicsContext' 3> c:\cygwin\home\buildbot\WebKit\WebCore\platform\graphics\GraphicsContext.h(200) : see declaration of 'WebCore::GraphicsContext'
Comment on attachment 75737 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75737&action=review Getting better, but not quite there yet. :) First and foremost your copy of GraphicsContextState appears to be out-of-date, smfr reordered the members in <http://trac.webkit.org/changeset/73292> > WebCore/platform/graphics/GraphicsContext.cpp:102 > + LOG_ERROR("ERROR void GraphicsContext::restore() m_stack is empty"); Nit: I suspect that "stack" here refers to the state stack, not the actual variable name. > WebCore/platform/graphics/GraphicsContext.h:206 > + virtual ~GraphicsContext(); > + > + virtual void platformInit(PlatformGraphicsContext*); > + virtual void platformDestroy(); These should not be virtual! Nothing inherits from GraphicsContext. > WebCore/platform/graphics/GraphicsContext.h:243 > + GraphicsContextState state() const; This should return a 'const GraphicsContextState&' instead of a copy. > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:202 > -GraphicsContext::GraphicsContext(PlatformGraphicsContext* cr) > - : m_common(createGraphicsContextPrivate()) > - , m_data(new GraphicsContextPlatformPrivate) > +void GraphicsContext::platformInit(PlatformGraphicsContext* cr) > { > + m_data = new GraphicsContextPlatformPrivate; Why are you switching away from using initializer list syntax for m_data? (This comment repeated for all GraphicsContext implementations.)
(In reply to comment #17) > Why are you switching away from using initializer list syntax for m_data? > (This comment repeated for all GraphicsContext implementations.) Ehm. Disregard this, I am an idiot. :)
Created attachment 75799 [details] Patch
Comment on attachment 75799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75799&action=review > WebCore/ChangeLog:9 > + and m_common member became unnecessary. They are removed. Double space after 'and'. s/m_common member/m_common/ > WebCore/ChangeLog:10 > + Add two virtual methods to GraphicsContext: platformInit() and platformDestroy(), which Not virtual anymore. > WebCore/platform/graphics/GraphicsContext.h:208 > + void platformInit(PlatformGraphicsContext*); > + void platformDestroy(); These methods should be private. > WebCore/platform/graphics/GraphicsContext.h:245 > + const GraphicsContextState& state(); Should be "const GraphicsContext& state() const" since the method doesn't modify any GraphicsContext members. > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:75 > + GraphicsContextState state = context->state(); Avoid the unnecessary copy: const GraphicsContextState& state = context->state(); > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:96 > + GraphicsContextState state = context->state(); Ditto.
Created attachment 75806 [details] Patch
Comment on attachment 75806 [details] Patch Excellent, r=me
The commit-queue encountered the following flaky tests while processing attachment 75806 [details]: http/tests/misc/script-async.html java/lc3/JSObject/ToObject-001.html Please file bugs against the tests. These tests were authored by ap@webkit.org and tonyg@chromium.org. The commit-queue is continuing to process your patch.
Comment on attachment 75806 [details] Patch Clearing flags on attachment: 75806 Committed r73492: <http://trac.webkit.org/changeset/73492>
All reviewed patches have been landed. Closing bug.
Hi, Your change caused a LOT of pixel test failures on Chromium's canary bot. Sounds like Linux bot is producing ~350 test failures. Windows and Mac bots are still compiling, and I guess they are likely to fail as well. As far as I looked at the test results, <input> controls are not rendered correctly (border of input text box is missing, radio button is not rendered at all, and so forth). Do you have any ideas about this breakage? I'd like to hear from you as soon as possible because these failures seems pretty severe.
I confirmed that Mac WebKit (non-Chromium) also causes a lot of pixel test failures as well. I'm going to roll out this change for this reason.
Reverted r73492 for reason: Caused a lot of pixel test failures and broke Windows build Committed r73496: <http://trac.webkit.org/changeset/73496>
@Reni: The problem here was most likely the missing initialization of GraphicsContext::m_updatingControlTints
Created attachment 76191 [details] Patch
Attachment 76191 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6991032
Attachment 76191 [details] did not build on qt: Build output: http://queues.webkit.org/results/6975024
Created attachment 76193 [details] Patch
Attachment 76191 [details] did not build on win: Build output: http://queues.webkit.org/results/6906041
This was landed in <http://trac.webkit.org/changeset/73728>, closing..