Bug 57563 – Remove dependency on chromium skia::PlatformCanvas

• Component: Platform
• Status: RESOLVED FIXED

[Alok Priyadarshi]

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.

[Alok Priyadarshi]

Created attachment 87919 [details]
proposed patch

A work in progress. Uploaded to help reviewers understand the changes in Chromium.

[Alok Priyadarshi]

Created attachment 88570 [details]
proposed patch

[Mike Reed]

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?

[Alok Priyadarshi]

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.

[George Wright]

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?

[Alok Priyadarshi]

(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.

[George Wright]

(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)?

[Alok Priyadarshi]

(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.

[George Wright]

(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.

[Alok Priyadarshi]

Created attachment 89020 [details]
proposed patch

[Mike Reed]

looks fine to me (but I'm not a reviewer)

[WebKit Review Bot]

Attachment 89020 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8386531

[James Robinson]

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....

[Alok Priyadarshi]

(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.

[James Robinson]

In that case you need to update the chromium rev specified in Source/WebKit/chromium/DEPS to whatever chromium # you need.

[Alok Priyadarshi]

Created attachment 89066 [details]
proposed patch

Rolled DEPS

[James Robinson]

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?

[Alok Priyadarshi]

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?

[Brian Salomon]

(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.

[Brian Salomon]

(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.

[James Robinson]

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?

[James Robinson]

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).

[Alok Priyadarshi]

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.

[Alok Priyadarshi]

Created attachment 89155 [details]
proposed patch

Resolved merge issues in DEPS.

[James Robinson]

Comment on attachment 89155 [details]
proposed patch

Looks good.  Please make sure this doesn't break all native controls on windows :)

[WebKit Commit Bot]

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

[Alok Priyadarshi]

Created attachment 89256 [details]
proposed patch

No longer need DEPS. It has already been rolled to a revision higher than this patch requires.

[WebKit Commit Bot]

Comment on attachment 89256 [details]
proposed patch

Clearing flags on attachment: 89256

Committed r83649: <http://trac.webkit.org/changeset/83649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jian Li]

Reverted r83649 for reason:

This patch causes compiling errors for chromium

Committed r83666: <http://trac.webkit.org/changeset/83666>

[Alok Priyadarshi]

(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?

[Jian Li]

(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

[Alok Priyadarshi]

Created attachment 89411 [details]
proposed patch

Fixed DRT compile errors.

[WebKit Review Bot]

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.

[Alok Priyadarshi]

Created attachment 89455 [details]
proposed patch

Fixed style.

[James Robinson]

Comment on attachment 89455 [details]
proposed patch

Cool.  Thanks for cleaning up the comments+signature.

[WebKit Commit Bot]

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

[Alok Priyadarshi]

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?

[Alok Priyadarshi]

(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!

[James Robinson]

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.

[Alok Priyadarshi]

(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.

[James Robinson]

Comment on attachment 89591 [details]
proposed patch

Fingers crossed!

[WebKit Commit Bot]

Comment on attachment 89591 [details]
proposed patch

Clearing flags on attachment: 89591

Committed r83941: <http://trac.webkit.org/changeset/83941>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alok Priyadarshi]

Created attachment 89735 [details]
proposed patch

Fixed DRT compile error on MAC.

[Adam Barth]

Comment on attachment 89735 [details]
proposed patch

Clearing flags on attachment: 89735

Committed r83946: <http://trac.webkit.org/changeset/83946>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.