Bug 57563 - Remove dependency on chromium skia::PlatformCanvas
Summary: Remove dependency on chromium skia::PlatformCanvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-31 11:08 PDT by Alok Priyadarshi
Modified: 2011-04-14 22:19 PDT (History)
10 users (show)

See Also:


Attachments
proposed patch (24.01 KB, patch)
2011-04-01 14:55 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (15.74 KB, patch)
2011-04-06 21:59 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (21.28 KB, patch)
2011-04-11 09:56 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (21.57 KB, patch)
2011-04-11 13:20 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (19.32 KB, patch)
2011-04-11 21:23 PDT, Alok Priyadarshi
jamesr: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
proposed patch (21.21 KB, patch)
2011-04-12 13:13 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (30.78 KB, patch)
2011-04-13 11:34 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (31.00 KB, patch)
2011-04-13 14:18 PDT, Alok Priyadarshi
jamesr: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
proposed patch (30.89 KB, patch)
2011-04-14 09:21 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff
proposed patch (1.24 KB, patch)
2011-04-14 22:03 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2011-03-31 11:08:44 PDT
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.
Comment 1 Alok Priyadarshi 2011-04-01 14:55:45 PDT
Created attachment 87919 [details]
proposed patch

A work in progress. Uploaded to help reviewers understand the changes in Chromium.
Comment 2 Alok Priyadarshi 2011-04-06 21:59:16 PDT
Created attachment 88570 [details]
proposed patch
Comment 3 Mike Reed 2011-04-07 06:22:26 PDT
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 4 Alok Priyadarshi 2011-04-07 08:14:42 PDT
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.
Comment 5 George Wright 2011-04-07 08:23:26 PDT
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?
Comment 6 Alok Priyadarshi 2011-04-07 08:51:51 PDT
(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.
Comment 7 George Wright 2011-04-07 08:58:55 PDT
(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)?
Comment 8 Alok Priyadarshi 2011-04-07 09:22:17 PDT
(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.
Comment 9 George Wright 2011-04-07 09:39:26 PDT
(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.
Comment 10 Alok Priyadarshi 2011-04-11 09:56:50 PDT
Created attachment 89020 [details]
proposed patch
Comment 11 Mike Reed 2011-04-11 10:05:36 PDT
looks fine to me (but I'm not a reviewer)
Comment 12 WebKit Review Bot 2011-04-11 10:23:20 PDT
Attachment 89020 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8386531
Comment 13 James Robinson 2011-04-11 12:51:25 PDT
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....
Comment 14 Alok Priyadarshi 2011-04-11 12:55:18 PDT
(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.
Comment 15 James Robinson 2011-04-11 12:59:35 PDT
In that case you need to update the chromium rev specified in Source/WebKit/chromium/DEPS to whatever chromium # you need.
Comment 16 Alok Priyadarshi 2011-04-11 13:20:57 PDT
Created attachment 89066 [details]
proposed patch

Rolled DEPS
Comment 17 James Robinson 2011-04-11 13:41:32 PDT
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 18 Alok Priyadarshi 2011-04-11 13:46:54 PDT
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?
Comment 19 Brian Salomon 2011-04-11 13:50:25 PDT
(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.
Comment 20 Brian Salomon 2011-04-11 13:51:28 PDT
(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 21 James Robinson 2011-04-11 16:22:41 PDT
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 22 James Robinson 2011-04-11 16:23:39 PDT
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 23 Alok Priyadarshi 2011-04-11 21:22:51 PDT
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.
Comment 24 Alok Priyadarshi 2011-04-11 21:23:58 PDT
Created attachment 89155 [details]
proposed patch

Resolved merge issues in DEPS.
Comment 25 James Robinson 2011-04-12 11:31:05 PDT
Comment on attachment 89155 [details]
proposed patch

Looks good.  Please make sure this doesn't break all native controls on windows :)
Comment 26 WebKit Commit Bot 2011-04-12 12:34:26 PDT
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
Comment 27 Alok Priyadarshi 2011-04-12 13:13:50 PDT
Created attachment 89256 [details]
proposed patch

No longer need DEPS. It has already been rolled to a revision higher than this patch requires.
Comment 28 WebKit Commit Bot 2011-04-12 15:27:20 PDT
Comment on attachment 89256 [details]
proposed patch

Clearing flags on attachment: 89256

Committed r83649: <http://trac.webkit.org/changeset/83649>
Comment 29 WebKit Commit Bot 2011-04-12 15:27:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Jian Li 2011-04-12 16:57:04 PDT
Reverted r83649 for reason:

This patch causes compiling errors for chromium

Committed r83666: <http://trac.webkit.org/changeset/83666>
Comment 31 Alok Priyadarshi 2011-04-12 18:01:48 PDT
(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?
Comment 32 Jian Li 2011-04-12 18:12:21 PDT
(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
Comment 33 Alok Priyadarshi 2011-04-13 11:34:54 PDT
Created attachment 89411 [details]
proposed patch

Fixed DRT compile errors.
Comment 34 WebKit Review Bot 2011-04-13 11:38:09 PDT
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.
Comment 35 Alok Priyadarshi 2011-04-13 14:18:01 PDT
Created attachment 89455 [details]
proposed patch

Fixed style.
Comment 36 James Robinson 2011-04-13 14:26:50 PDT
Comment on attachment 89455 [details]
proposed patch

Cool.  Thanks for cleaning up the comments+signature.
Comment 37 WebKit Commit Bot 2011-04-14 00:49:46 PDT
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
Comment 38 Alok Priyadarshi 2011-04-14 09:21:41 PDT
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?
Comment 39 Alok Priyadarshi 2011-04-14 14:48:46 PDT
(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!
Comment 40 James Robinson 2011-04-14 14:55:16 PDT
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.
Comment 41 Alok Priyadarshi 2011-04-14 15:00:08 PDT
(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 42 James Robinson 2011-04-14 15:02:56 PDT
Comment on attachment 89591 [details]
proposed patch

Fingers crossed!
Comment 43 WebKit Commit Bot 2011-04-14 20:52:46 PDT
Comment on attachment 89591 [details]
proposed patch

Clearing flags on attachment: 89591

Committed r83941: <http://trac.webkit.org/changeset/83941>
Comment 44 WebKit Commit Bot 2011-04-14 20:52:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 Alok Priyadarshi 2011-04-14 22:03:19 PDT
Created attachment 89735 [details]
proposed patch

Fixed DRT compile error on MAC.
Comment 46 Adam Barth 2011-04-14 22:19:30 PDT
Comment on attachment 89735 [details]
proposed patch

Clearing flags on attachment: 89735

Committed r83946: <http://trac.webkit.org/changeset/83946>
Comment 47 Adam Barth 2011-04-14 22:19:37 PDT
All reviewed patches have been landed.  Closing bug.