Chromium uses custom skia canvas classes which makes it difficult to use other types of canvases provided by Skia such as gpu-canvas, picture-canvas, etc. It also leads to cyclic dependencies between chromium and webkit. Webkit skia platform should just use Skia not chromium's subclass skia::PlatformCanvas.
Created attachment 87919 [details] proposed patch A work in progress. Uploaded to help reviewers understand the changes in Chromium.
Created attachment 88570 [details] proposed patch
Comment on attachment 88570 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=88570&action=review > WebCore/platform/graphics/skia/PlatformContextSkia.cpp:617 > + return false; Can we always return false? > WebCore/platform/graphics/skia/PlatformContextSkia.cpp:626 > + return false; Can we always return false?
Comment on attachment 88570 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=88570&action=review >> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:617 >> + return false; > > Can we always return false? This patch is not ready yet. I uploaded it for the reviewers of my chromium patch to understand what I am trying to do. I could resolve this by adding a isPrinting member variable to PlatformContextSkia. But is it really required? I noticed that it is only being used to decide image filtering mode. Can we deduce it by any other means? IsVectorial is a roundabout way of deducing printing any way. And returning true in case of SKIA_GPU is also a roundabout way to forcing a particular filtering mode. >> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:626 >> + return false; > > Can we always return false? No I need to add skia::IsNativeDrawingAllowed(SkCanvas*) on the chromium side.
This looks great! Just a question though: 55 m_skiaCanvas = skia::CreateBitmapCanvas(size.width(), size.height(), false); That looks like the sole creation point for the SkCanvas now, using a factory defined in the new skia platform layer, right? Does this mean that if someone wants to use, say, a SkGpuCanvas instead of an SkCanvas, they'd have to modify webkit code? Would it be better to abstract the canvas-type selection out into the factory it calls as well, so then a port could choose more easily and on-the-fly what type of canvas they want?
(In reply to comment #5) > This looks great! Just a question though: > > 55 m_skiaCanvas = skia::CreateBitmapCanvas(size.width(), size.height(), false); > > That looks like the sole creation point for the SkCanvas now, using a factory defined in the new skia platform layer, right? Does this mean that if someone wants to use, say, a SkGpuCanvas instead of an SkCanvas, they'd have to modify webkit code? Would it be better to abstract the canvas-type selection out into the factory it calls as well, so then a port could choose more easily and on-the-fly what type of canvas they want? Exactly. I have not looked at all the places in webkit where a canvas is being created but I have a feeling that we might need a few more parameters for the port to decide which type of canvas to create. We will eventually get there. This CL does not change any functionality or the type of canvas created - only the interface.
(In reply to comment #6) > Exactly. I have not looked at all the places in webkit where a canvas is being created but I have a feeling that we might need a few more parameters for the port to decide which type of canvas to create. > > We will eventually get there. This CL does not change any functionality or the type of canvas created - only the interface. Ah, that makes sense. Would CreateBitmapDevice be a misnomer then for the future if down the road we would be looking to change its functionality to be able to create more than one type of canvas (which may or may not be a bitmap one)?
(In reply to comment #7) > (In reply to comment #6) > > Exactly. I have not looked at all the places in webkit where a canvas is being created but I have a feeling that we might need a few more parameters for the port to decide which type of canvas to create. > > > > We will eventually get there. This CL does not change any functionality or the type of canvas created - only the interface. > > Ah, that makes sense. Would CreateBitmapDevice be a misnomer then for the future if down the road we would be looking to change its functionality to be able to create more than one type of canvas (which may or may not be a bitmap one)? I think you meant CreateBitmapCanvas instead of CreateBitmapDevice. Yes As I mentioned earlier we might need a few more parameters in which case we could rename this as CreateCanvas. But at this point I am not sure what those parameters will be.
(In reply to comment #8) > I think you meant CreateBitmapCanvas instead of CreateBitmapDevice. Yes As I mentioned earlier we might need a few more parameters in which case we could rename this as CreateCanvas. But at this point I am not sure what those parameters will be. Sorry, yes, I meant CreateBitmapCanvas not Device :) That sounds good. Just wondered if it would be worth naming it CreateCanvas from the start, is all.
Created attachment 89020 [details] proposed patch
looks fine to me (but I'm not a reviewer)
Attachment 89020 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8386531
Why doesn't this compile on the cr-linux bot? Compile error: CXX(target) out/Release/obj.target/glue/Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.o Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.cc: In member function 'virtual void webkit::npapi::WebPluginDelegateImpl::Paint(WebKit::WebCanvas*, const gfx::Rect&)': Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.cc:114: error: 'class SkCanvas' has no member named 'beginPlatformPaint' Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.cc:116: error: 'class SkCanvas' has no member named 'endPlatformPaint' make: *** [out/Release/obj.target/glue/Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.o] Error 1 make: *** Waiting for unfinished jobs....
(In reply to comment #13) > Why doesn't this compile on the cr-linux bot? Compile error: > > CXX(target) out/Release/obj.target/glue/Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.o > Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.cc: In member function 'virtual void webkit::npapi::WebPluginDelegateImpl::Paint(WebKit::WebCanvas*, const gfx::Rect&)': > Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.cc:114: error: 'class SkCanvas' has no member named 'beginPlatformPaint' > Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.cc:116: error: 'class SkCanvas' has no member named 'endPlatformPaint' > make: *** [out/Release/obj.target/glue/Source/WebKit/chromium/webkit/plugins/npapi/webplugin_delegate_impl_gtk.o] Error 1 > make: *** Waiting for unfinished jobs.... Because it is not compiling against TOT? It needs two CLs in chromium (r81107 and r81009) that I submitted this morning.
In that case you need to update the chromium rev specified in Source/WebKit/chromium/DEPS to whatever chromium # you need.
Created attachment 89066 [details] proposed patch Rolled DEPS
Comment on attachment 89066 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=89066&action=review > Source/WebCore/platform/graphics/skia/ImageSkia.cpp:271 > +#if ENABLE(SKIA_GPU) > + resampling = RESAMPLE_NONE; > +#else > + resampling = platformContext->printing() ? RESAMPLE_NONE : This doesn't seem right - in SKIA_GPU we always do nearest neighbor?
Comment on attachment 89066 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=89066&action=review >> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:271 >> + resampling = platformContext->printing() ? RESAMPLE_NONE : > > This doesn't seem right - in SKIA_GPU we always do nearest neighbor? I am only replicating whatever logic was here before. PlatformContextSkia::isPrinting was always returning true in SKIA_GPU. I do not know who added SKIA_GPU in isPrinting in the first place - Mike/Brian?
(In reply to comment #17) > (From update of attachment 89066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89066&action=review > > > Source/WebCore/platform/graphics/skia/ImageSkia.cpp:271 > > +#if ENABLE(SKIA_GPU) > > + resampling = RESAMPLE_NONE; > > +#else > > + resampling = platformContext->printing() ? RESAMPLE_NONE : > > This doesn't seem right - in SKIA_GPU we always do nearest neighbor? No, we either do nearest neighbor or bilinear.
(In reply to comment #18) > (From update of attachment 89066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89066&action=review > > >> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:271 > >> + resampling = platformContext->printing() ? RESAMPLE_NONE : > > > > This doesn't seem right - in SKIA_GPU we always do nearest neighbor? > > I am only replicating whatever logic was here before. PlatformContextSkia::isPrinting was always returning true in SKIA_GPU. I do not know who added SKIA_GPU in isPrinting in the first place - Mike/Brian? I think Mike did that as a hack just to avoid the cast to PlatformCanvas when the canvas was really a SkGpuCanvas. It wasn't correct.
Comment on attachment 89066 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=89066&action=review >>>>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:271 >>>>> + resampling = platformContext->printing() ? RESAMPLE_NONE : >>>> >>>> This doesn't seem right - in SKIA_GPU we always do nearest neighbor? >>> >>> I am only replicating whatever logic was here before. PlatformContextSkia::isPrinting was always returning true in SKIA_GPU. I do not know who added SKIA_GPU in isPrinting in the first place - Mike/Brian? >> >> No, we either do nearest neighbor or bilinear. > > I think Mike did that as a hack just to avoid the cast to PlatformCanvas when the canvas was really a SkGpuCanvas. It wasn't correct. Is it possible to do the right thing now?
Comment on attachment 89066 [details] proposed patch You have some merge conflicts on DEPS - please update the patch and repost (I'd really like to see at least one clean cr-linux run on this patch just to be sure there aren't any other issues with the DEPS roll by itself).
Comment on attachment 89066 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=89066&action=review >>>>>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:271 >>>>> >>>>> This doesn't seem right - in SKIA_GPU we always do nearest neighbor? >>>> >>>> I am only replicating whatever logic was here before. PlatformContextSkia::isPrinting was always returning true in SKIA_GPU. I do not know who added SKIA_GPU in isPrinting in the first place - Mike/Brian? >>> >>> No, we either do nearest neighbor or bilinear. >> >> I think Mike did that as a hack just to avoid the cast to PlatformCanvas when the canvas was really a SkGpuCanvas. It wasn't correct. > > Is it possible to do the right thing now? Done.
Created attachment 89155 [details] proposed patch Resolved merge issues in DEPS.
Comment on attachment 89155 [details] proposed patch Looks good. Please make sure this doesn't break all native controls on windows :)
Comment on attachment 89155 [details] proposed patch Rejecting attachment 89155 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2 Last 500 characters of output: /ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/chromium/DEPS Hunk #1 FAILED at 32. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/chromium/DEPS.rej patching file Source/WebKit/chromium/public/WebCanvas.h patching file Source/WebKit/chromium/src/WebFrameImpl.cpp patching file Source/WebKit/chromium/tests/TransparencyWinTest.cpp Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'James Robinson', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8389580
Created attachment 89256 [details] proposed patch No longer need DEPS. It has already been rolled to a revision higher than this patch requires.
Comment on attachment 89256 [details] proposed patch Clearing flags on attachment: 89256 Committed r83649: <http://trac.webkit.org/changeset/83649>
All reviewed patches have been landed. Closing bug.
Reverted r83649 for reason: This patch causes compiling errors for chromium Committed r83666: <http://trac.webkit.org/changeset/83666>
(In reply to comment #30) > Reverted r83649 for reason: > > This patch causes compiling errors for chromium > > Committed r83666: <http://trac.webkit.org/changeset/83666> Could you point me to the build log that failed?
(In reply to comment #31) > (In reply to comment #30) > > Reverted r83649 for reason: > > > > This patch causes compiling errors for chromium > > > > Committed r83666: <http://trac.webkit.org/changeset/83666> > > Could you point me to the build log that failed? http://build.webkit.org/builders/Chromium%20Win%20Release/builds/22580
Created attachment 89411 [details] proposed patch Fixed DRT compile errors.
Attachment 89411 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Tools/DumpRenderTree/chromium/WebThemeControlDRTWin.h:125: The parameter name "canvas" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 89455 [details] proposed patch Fixed style.
Comment on attachment 89455 [details] proposed patch Cool. Thanks for cleaning up the comments+signature.
Comment on attachment 89455 [details] proposed patch Rejecting attachment 89455 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2 Last 500 characters of output: atching file Tools/DumpRenderTree/chromium/TestShell.cpp patching file Tools/DumpRenderTree/chromium/TestShell.h patching file Tools/DumpRenderTree/chromium/WebThemeControlDRTWin.cpp patching file Tools/DumpRenderTree/chromium/WebThemeControlDRTWin.h patching file Tools/DumpRenderTree/chromium/WebViewHost.cpp patching file Tools/DumpRenderTree/chromium/WebViewHost.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'James Robinson', u'--f..." exit_code: 1 Full output: http://queues.webkit.org/results/8401604
Created attachment 89591 [details] proposed patch Resolved merge issues. The commit bot took ~24 hours to process the last patch only fail due to merge issues. One of the files I had changed got renamed while my patch was sitting in the commit queue. James: Could you please land this patch for me?
(In reply to comment #38) > Created an attachment (id=89591) [details] > proposed patch > > Resolved merge issues. The commit bot took ~24 hours to process the last patch only fail due to merge issues. One of the files I had changed got renamed while my patch was sitting in the commit queue. > > James: Could you please land this patch for me? ping!
I can land it by hand but probably not until next week. If you merge the patch up I can try cq+'ing it again. If you need it landed sooner you'll have to find someone else to help, I'm afraid.
(In reply to comment #40) > I can land it by hand but probably not until next week. If you merge the patch up I can try cq+'ing it again. If you need it landed sooner you'll have to find someone else to help, I'm afraid. No Problem. Lets try the commit queue again. It is ready to go.
Comment on attachment 89591 [details] proposed patch Fingers crossed!
Comment on attachment 89591 [details] proposed patch Clearing flags on attachment: 89591 Committed r83941: <http://trac.webkit.org/changeset/83941>
Created attachment 89735 [details] proposed patch Fixed DRT compile error on MAC.
Comment on attachment 89735 [details] proposed patch Clearing flags on attachment: 89735 Committed r83946: <http://trac.webkit.org/changeset/83946>