Summary: | [chromium] need DragImage implementation | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Stade <estade> | ||||||||||||||||||||||||||||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, cjerdonek, commit-queue, dglazkov, eric, fishd, hamaji, pfeldman, thakis, webkit.review.bot | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 37069 | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Evan Stade
2010-03-05 14:22:21 PST
Created attachment 50127 [details]
try1
Attachment 50127 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/chromium/DragImageRef.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 50129 [details]
style
Attachment 50127 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/335639 Created attachment 50131 [details]
forgot to svn add
Attachment 50131 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/chromium/DragImageRef.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 50132 [details]
style, 2
Attachment 50132 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/chromium/DragImageRef.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 50129 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/336632 Created attachment 50136 [details]
mac build fix
Created attachment 50241 [details]
review comments
Comment on attachment 50241 [details] review comments > Index: WebCore/platform/chromium/DragImageChromium.cpp ... > +#if OS(DARWIN) > +#include "skia/ext/skia_utils_mac.h" > +#endif Can you use CG in the Mac port instead? We should really avoid using skia/ext in WebCore. It introduces a circular dependency between the repositories! > +DragImageRef createDragImageFromImage(Image* image) > +{ > +#if OS(DARWIN) > + SkBitmap bitmap = gfx::CGImageToSkBitmap(image->getCGImageRef()); please avoid adding gfx:: namespace stuff to WebCore! that stuff is chromium only. > Index: WebKit/chromium/ChangeLog ... > + * public/WebDragImageRef.h: Added. > + * src/DragClientImpl.cpp: > + (WebKit::DragClientImpl::startDrag): > + * src/WebViewImpl.cpp: > + (WebKit::WebViewImpl::startDragging): > + * src/WebViewImpl.h: is there some other public interface change missing? I don't see any public API that uses WebDragImageRef. also, please see WebImage.h to note how things differ between WEBKIT_USING_SKIA and WEBKIT_USING_CG. It seems like WebDragImageRef should be made consistent with WebImage. (In reply to comment #12) > (From update of attachment 50241 [details]) > > Index: WebCore/platform/chromium/DragImageChromium.cpp > ... > > +#if OS(DARWIN) > > +#include "skia/ext/skia_utils_mac.h" > > +#endif > > Can you use CG in the Mac port instead? We should really avoid using > skia/ext in WebCore. It introduces a circular dependency between the > repositories! we can use CG for Mac, but then we have to implement DragImageChromium twice, and fork a lot of code on the chrome side of this patch. > > > > +DragImageRef createDragImageFromImage(Image* image) > > +{ > > +#if OS(DARWIN) > > + SkBitmap bitmap = gfx::CGImageToSkBitmap(image->getCGImageRef()); > > please avoid adding gfx:: namespace stuff to WebCore! that stuff is > chromium only. After talking to several mac people, this is the only way I could find to convert CG to SkBitmap. Is there a better way? If not I can leave the mac side unimplemented for the time being. > > > > Index: WebKit/chromium/ChangeLog > ... > > + * public/WebDragImageRef.h: Added. > > + * src/DragClientImpl.cpp: > > + (WebKit::DragClientImpl::startDrag): > > + * src/WebViewImpl.cpp: > > + (WebKit::WebViewImpl::startDragging): > > + * src/WebViewImpl.h: > > is there some other public interface change missing? I don't see > any public API that uses WebDragImageRef. > > also, please see WebImage.h to note how things differ between > WEBKIT_USING_SKIA and WEBKIT_USING_CG. > > It seems like WebDragImageRef should be made consistent with > WebImage. Yes, the change to WebViewClient::StartDragging is missing (see FIXME in WebViewImpl). That is not part of this patch because I am breaking it up into multiple pieces. Created attachment 50269 [details]
use WebImage, no mac implementation
Attachment 50269 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Could not read file. Skipping: "WebCore/platform/chromium/DragImageChromium.cpp"
WebCore/platform/chromium/DragImageChromiumSkia.cpp:32: You should add a blank line after implementation file's own header. [build/include_order] [4]
WebCore/platform/chromium/DragImageChromiumSkia.cpp:38: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 50269 [details] use WebImage, no mac implementation > Index: WebKit/chromium/public/WebViewClient.h ... > virtual void startDragging( > - const WebPoint& from, const WebDragData&, WebDragOperationsMask) { } > + const WebDragData&, WebDragOperationsMask, const WebImage&, const WebPoint&) { } Can you avoid making this a two-sided patch landing by continuing to support the old API temporarily? > Index: WebKit/chromium/src/DragClientImpl.cpp ... > +#if WEBKIT_USING_SKIA > + m_webView->startDragging( > + dragData, static_cast<WebDragOperationsMask>(dragOperationMask), > + WebImage(*dragImage), offsetPoint); > +#else > + // FIXME ^^^ Can you add a comment about this FIXME? Something about needing to pass a non-empty image perhaps? Are the style bot issues something to worry about? (In reply to comment #16) > (From update of attachment 50269 [details]) > > Index: WebKit/chromium/public/WebViewClient.h > ... > > virtual void startDragging( > > - const WebPoint& from, const WebDragData&, WebDragOperationsMask) { } > > + const WebDragData&, WebDragOperationsMask, const WebImage&, const WebPoint&) { } > > Can you avoid making this a two-sided patch landing by continuing > to support the old API temporarily? we can land the chrome side first now without anything breaking. Sorry, I should have mentioned that here. I only mentioned that at http://codereview.chromium.org/668125/show. So perhaps it would be best to review that side of it first? > > > > Index: WebKit/chromium/src/DragClientImpl.cpp > ... > > +#if WEBKIT_USING_SKIA > > + m_webView->startDragging( > > + dragData, static_cast<WebDragOperationsMask>(dragOperationMask), > > + WebImage(*dragImage), offsetPoint); > > +#else > > + // FIXME > > ^^^ Can you add a comment about this FIXME? Something about needing > to pass a non-empty image perhaps? ok > > > Are the style bot issues something to worry about? yes Created attachment 50868 [details]
comment + style
the chrome side was landed. I have updated the patch. Possible to take another look? thanks.
Attachment 50868 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/892042 Created attachment 50908 [details]
try to fix mysterious build failure
don't really understand the build failure. The chromium trybots like the patch ok.
Attachment 50908 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Could not read file. Skipping: "WebCore/platform/chromium/DragImageChromium.cpp"
WebCore/platform/chromium/DragImageChromiumSkia.cpp:32: You should add a blank line after implementation file's own header. [build/include_order] [4]
WebCore/platform/chromium/DragImageChromiumSkia.cpp:38: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 50910 [details]
fixed
Created attachment 50912 [details]
correct relative paths
Comment on attachment 50912 [details]
correct relative paths
r=me
Comment on attachment 50912 [details] correct relative paths Rejecting patch 50912 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Fisher', '--force']" exit_code: 1 Last 500 characters of output: ching file WebKit/chromium/src/DragClientImpl.cpp patching file WebKit/chromium/src/WebViewImpl.cpp Hunk #2 succeeded at 1933 (offset 49 lines). patching file WebKit/chromium/public/WebViewClient.h Hunk #1 FAILED at 47. Hunk #2 succeeded at 223 (offset 3 lines). 1 out of 2 hunks FAILED -- saving rejects to file WebKit/chromium/public/WebViewClient.h.rej patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKit/chromium/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Full output: http://webkit-commit-queue.appspot.com/results/1628184 Created attachment 52339 [details]
re-synced
synced to ToT
Comment on attachment 52339 [details]
re-synced
Rejecting patch 52339 from commit-queue.
Unexpected failure when landing patch! Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--build-style=both', '--quiet', '52339', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
webkitpy/tool/commands/stepsequence.py", line 60, in _run
step(tool, options).run(state)
File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 59, in run
if self._has_valid_reviewer(changelog_entry):
File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 41, in _has_valid_reviewer
if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'
That was not a very elegant error on our part. webkitpy.common.system.executive.ScriptError: Failed to parse ChangeLog: /Projects/WebKit/WebKit/chromium/ChangeLog Seems to be the problem. Comment on attachment 52339 [details]
re-synced
Looks like your chromium changelog has a leading + sign. That would throw our ChangeLog parser off. :)
Created attachment 52426 [details]
plus--
Comment on attachment 52426 [details]
plus--
Rejecting patch 52426 from commit-queue.
Unexpected failure when landing patch! Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '52426', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
webkitpy/tool/commands/stepsequence.py", line 60, in _run
step(tool, options).run(state)
File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 59, in run
if self._has_valid_reviewer(changelog_entry):
File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 41, in _has_valid_reviewer
if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'
This looks like another ChangeLog parsing problem. Again, my apologies for our poor error reporting. You can test this locally by running: webkit-patch commit-message Created attachment 52445 [details] space-- thanks for the tip Parsing ChangeLog: /usr/local/google/WebKit/WebKit/chromium/ChangeLog Parsing ChangeLog: /usr/local/google/WebKit/WebCore/ChangeLog 2010-04-01 Evan Stade <estade@chromium.org> Reviewed by NOBODY (OOPS!). [chromium] need DragImage implementation https://bugs.webkit.org/show_bug.cgi?id=35811 Use the DragImageRef that the DragController passes to us. * public/WebViewClient.h: (WebKit::WebViewClient::startDragging): * src/DragClientImpl.cpp: (WebKit::DragClientImpl::startDrag): * src/WebViewImpl.cpp: (WebKit::WebViewImpl::startDragging): * src/WebViewImpl.h: 2010-04-01 Evan Stade <estade@chromium.org> Reviewed by NOBODY (OOPS!). [chromium] need DragImage implementation https://bugs.webkit.org/show_bug.cgi?id=35811 Basic implementation using SkBitmap. Transformations are not supported yet. No implementation for mac. * WebCore.gyp/WebCore.gyp: * WebCore.gypi: * platform/chromium/DragImageChromium.cpp: * platform/chromium/DragImageRef.h: Comment on attachment 52445 [details]
space--
Sorry our tools have been such trouble. /me crosses fingers.
Comment on attachment 52445 [details] space-- Clearing flags on attachment: 52445 Committed r57028: <http://trac.webkit.org/changeset/57028> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/57028 might have broken Chromium Linux Release Created attachment 52465 [details]
svn add
these two files got lost in the shuffle
Comment on attachment 52465 [details] svn add Clearing flags on attachment: 52465 Committed r57032: <http://trac.webkit.org/changeset/57032> All reviewed patches have been landed. Closing bug. Just about drag-n-drop related layout test is now failing for the chromium build. I think this needs to be reverted. Mac side at https://bugs.webkit.org/show_bug.cgi?id=37069 . (In reply to comment #41) > Just about drag-n-drop related layout test is now failing for the chromium > build. I think this needs to be reverted. I am on duty starting on Monday. Will revert this unless tests get fixed by Monday morning GMT. I can look after the tests if someone tells me where the list of now failing tests is. http://codereview.chromium.org/1591013 should do the trick. I'm having troubles running layout tests locally (see my mail to chromium-dev), so I'm not 100% sure that it does, but it seems very likely that that's the cause. Rolled out both r57028 and r57032. Committing to http://svn.webkit.org/repository/webkit/trunk ... R WebCore/platform/chromium/DragImageChromiumMac.cpp => WebCore/platform/chromium/DragImageChromium.cpp D WebCore/platform/chromium/DragImageChromiumSkia.cpp M WebCore/ChangeLog M WebCore/WebCore.gyp/WebCore.gyp M WebCore/WebCore.gypi M WebCore/platform/chromium/DragImageRef.h M WebKit/chromium/ChangeLog M WebKit/chromium/public/WebViewClient.h M WebKit/chromium/src/DragClientImpl.cpp M WebKit/chromium/src/WebViewImpl.cpp M WebKit/chromium/src/WebViewImpl.h Committed r57064 Created attachment 52573 [details]
null check
Fix was null check in DragClientImpl::startDragging
As said on the corresponding chrome bug, not implementing scaleDragImage() destroys the IPC connection when dragging big images, which looks like a renderer crash to the browser. If it's just a couple lines, maybe you can just implement it. Attachment 52573 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
ERROR: File does not exist: WebCore/platform/chromium/DragImageChromium.cpp
If any of these errors are false positives, please file a bug against check-webkit-style.
Chris, this is an bug in check-webkit-style, no? I retract my comment. I didn't realize that Evan does `return 0;` instead of `return image` in scaleDragImage(). Created attachment 52578 [details]
scale image implemented
Well, I couldn't get it to crash even for large images, but here is a scale implementation anyway (which appears to work, as scale is called on small images).
Attachment 52578 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
ERROR: File does not exist: WebCore/platform/chromium/DragImageChromium.cpp
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #51) > Chris, this is an bug in check-webkit-style, no? I think so. I had originally made it just log a message, but Shinichiro requested that I change it to raise an exception: https://bugs.webkit.org/show_bug.cgi?id=36957#c3 (In reply to comment #55) > (In reply to comment #51) > > Chris, this is an bug in check-webkit-style, no? > > I think so. I had originally made it just log a message, but Shinichiro > requested that I change it to raise an exception: > > https://bugs.webkit.org/show_bug.cgi?id=36957#c3 Filed bug here: https://bugs.webkit.org/show_bug.cgi?id=37122 Comment on attachment 52578 [details]
scale image implemented
ok.
Comment on attachment 52578 [details] scale image implemented Clearing flags on attachment: 52578 Committed r57162: <http://trac.webkit.org/changeset/57162> All reviewed patches have been landed. Closing bug. |