Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2125853002:
Replace RefPtrs with raw pointers (imageFromNode in Editor.cpp) (Closed)
|
Created:
4 years, 5 months ago by hajimehoshi Modified:
4 years, 5 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, dshwang, ajuma+watch-canvas_chromium.org, blink-reviews-html_chromium.org, Justin Novosad, loading-reviews_chromium.org, dglazkov+blink, Rik, gavinp+loader_chromium.org, blink-reviews, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace RefPtrs with raw pointers (imageFromNode in Editor.cpp)
imageFromNode in Editor.cpp now returns a RefPtr to an Image, but this
function creates a return value from ImageResource::getImage(), which
returns a raw pointer to ImageResource::m_image. To reduce unnecessary
refs of that field, this CL replaces (Pass)RefPtrs with raw pointers.
The goal is to change ImageResource::m_image to be a unique_ptr, and
this CL is one of all steps.
BUG=n/a
TEST=webkit_unit_tests --gtest_filter=CanvasRenderingContext2DTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Patch Set 1 : impl #
Total comments: 5
Patch Set 2 : (rebase) #
Depends on Patchset: Messages
Total messages: 19 (8 generated)
Description was changed from ========== Replace RefPtrs with raw pointers (imageFromNode in Editor.cpp) imageFromNode in Editor.cpp now returns a RefPtr to an Image, but this function creates a return value from ImageResource::getImage(), which returns a raw pointer to ImageResource::m_image. To reduce unnecessary owners of that field, this CL replaces (Pass)RefPtrs with raw pointers. BUG=n/a TEST=webkit_unit_tests --gtest_filter=CanvasRenderingContext2DTest.* ========== to ========== Replace RefPtrs with raw pointers (imageFromNode in Editor.cpp) imageFromNode in Editor.cpp now returns a RefPtr to an Image, but this function creates a return value from ImageResource::getImage(), which returns a raw pointer to ImageResource::m_image. To reduce unnecessary owners of that field, this CL replaces (Pass)RefPtrs with raw pointers. BUG=n/a TEST=webkit_unit_tests --gtest_filter=CanvasRenderingContext2DTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Replace RefPtrs with raw pointers (imageFromNode in Editor.cpp) imageFromNode in Editor.cpp now returns a RefPtr to an Image, but this function creates a return value from ImageResource::getImage(), which returns a raw pointer to ImageResource::m_image. To reduce unnecessary owners of that field, this CL replaces (Pass)RefPtrs with raw pointers. BUG=n/a TEST=webkit_unit_tests --gtest_filter=CanvasRenderingContext2DTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Replace RefPtrs with raw pointers (imageFromNode in Editor.cpp) imageFromNode in Editor.cpp now returns a RefPtr to an Image, but this function creates a return value from ImageResource::getImage(), which returns a raw pointer to ImageResource::m_image. To reduce unnecessary refs of that field, this CL replaces (Pass)RefPtrs with raw pointers. BUG=n/a TEST=webkit_unit_tests --gtest_filter=CanvasRenderingContext2DTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
hajimehoshi@chromium.org changed reviewers: + haraken@chromium.org, kbr@chromium.org
PTAL haraken: core/ kbr: modules/webgl
https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.h:267: mutable RefPtr<Image> m_transparentImage; Why do we need to keep alive the image after copiedImage() returns? This CL is going to unnecessarily prolong the lifetime of m_transparentImage, which doesn't seem like a right thing to do to me.
kbr@chromium.org changed reviewers: + junov@chromium.org
+junov, who should review this and provide guidance too. https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1197: return m_transparentImage.get(); This seems to introduce a bunch of caching problems. What happens if the caller indirectly calls this method, then changes the canvas's content, and then calls it again?
https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1197: return m_transparentImage.get(); On 2016/07/06 21:37:04, Ken Russell wrote: > This seems to introduce a bunch of caching problems. What happens if the caller > indirectly calls this method, then changes the canvas's content, and then calls > it again? Even when the content is changed, as long as the size is not changed, the same transparent image will be used. https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.h:267: mutable RefPtr<Image> m_transparentImage; On 2016/07/06 12:46:44, haraken wrote: > > Why do we need to keep alive the image after copiedImage() returns? > > This CL is going to unnecessarily prolong the lifetime of m_transparentImage, > which doesn't seem like a right thing to do to me. Hmm, I agree. Do you think it'd make sense if m_transparentImage lives only when m_context is null?
https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.h:267: mutable RefPtr<Image> m_transparentImage; On 2016/07/07 10:53:07, hajimehoshi wrote: > On 2016/07/06 12:46:44, haraken wrote: > > > > Why do we need to keep alive the image after copiedImage() returns? > > > > This CL is going to unnecessarily prolong the lifetime of m_transparentImage, > > which doesn't seem like a right thing to do to me. > > Hmm, I agree. > > Do you think it'd make sense if m_transparentImage lives only when m_context is > null? I don't fully understand what problem you're trying to solve. I don't think HTMLCanvasElement should hold m_transparentImage. HTMLCanvasElement is just responsible for creating the transparent image and return the image to someone else. If the someone wants to hold the reference, the someone should do that. So the current design looks reasonable to me. HTMLCanvasElement returns a PassRefPtr to pass the ownership. The receiver is responsible for handling the ownership. In this particular case, the receiver doesn't need to hold the reference, so the receiver just drops it immediately. This design doesn't look strange to me.
On 2016/07/07 11:00:56, haraken wrote: > https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): > > https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLCanvasElement.h:267: mutable > RefPtr<Image> m_transparentImage; > On 2016/07/07 10:53:07, hajimehoshi wrote: > > On 2016/07/06 12:46:44, haraken wrote: > > > > > > Why do we need to keep alive the image after copiedImage() returns? > > > > > > This CL is going to unnecessarily prolong the lifetime of > m_transparentImage, > > > which doesn't seem like a right thing to do to me. > > > > Hmm, I agree. > > > > Do you think it'd make sense if m_transparentImage lives only when m_context > is > > null? > > I don't fully understand what problem you're trying to solve. > > I don't think HTMLCanvasElement should hold m_transparentImage. > HTMLCanvasElement is just responsible for creating the transparent image and > return the image to someone else. If the someone wants to hold the reference, > the someone should do that. > > So the current design looks reasonable to me. HTMLCanvasElement returns a > PassRefPtr to pass the ownership. The receiver is responsible for handling the > ownership. In this particular case, the receiver doesn't need to hold the > reference, so the receiver just drops it immediately. This design doesn't look > strange to me. My intention was that I didn't want a return value of ImageResource::getImage() to be passed the ownership. If the current design is right, maybe what I'm going to do is wrong or difficult and for now ImageResource::getImage() should return a PassRefPtr rather than a raw pointer?
On 2016/07/07 11:08:54, hajimehoshi wrote: > On 2016/07/07 11:00:56, haraken wrote: > > > https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): > > > > > https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/HTMLCanvasElement.h:267: mutable > > RefPtr<Image> m_transparentImage; > > On 2016/07/07 10:53:07, hajimehoshi wrote: > > > On 2016/07/06 12:46:44, haraken wrote: > > > > > > > > Why do we need to keep alive the image after copiedImage() returns? > > > > > > > > This CL is going to unnecessarily prolong the lifetime of > > m_transparentImage, > > > > which doesn't seem like a right thing to do to me. > > > > > > Hmm, I agree. > > > > > > Do you think it'd make sense if m_transparentImage lives only when m_context > > is > > > null? > > > > I don't fully understand what problem you're trying to solve. > > > > I don't think HTMLCanvasElement should hold m_transparentImage. > > HTMLCanvasElement is just responsible for creating the transparent image and > > return the image to someone else. If the someone wants to hold the reference, > > the someone should do that. > > > > So the current design looks reasonable to me. HTMLCanvasElement returns a > > PassRefPtr to pass the ownership. The receiver is responsible for handling the > > ownership. In this particular case, the receiver doesn't need to hold the > > reference, so the receiver just drops it immediately. This design doesn't look > > strange to me. > > My intention was that I didn't want a return value of ImageResource::getImage() > to be passed the ownership. If the current design is right, maybe what I'm going > to do is wrong or difficult and for now ImageResource::getImage() should return > a PassRefPtr rather than a raw pointer? s/to be passed the ownership/to be reffed by another object/
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/07 11:10:03, hajimehoshi wrote: > On 2016/07/07 11:08:54, hajimehoshi wrote: > > On 2016/07/07 11:00:56, haraken wrote: > > > > > > https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): > > > > > > > > > https://codereview.chromium.org/2125853002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/html/HTMLCanvasElement.h:267: mutable > > > RefPtr<Image> m_transparentImage; > > > On 2016/07/07 10:53:07, hajimehoshi wrote: > > > > On 2016/07/06 12:46:44, haraken wrote: > > > > > > > > > > Why do we need to keep alive the image after copiedImage() returns? > > > > > > > > > > This CL is going to unnecessarily prolong the lifetime of > > > m_transparentImage, > > > > > which doesn't seem like a right thing to do to me. > > > > > > > > Hmm, I agree. > > > > > > > > Do you think it'd make sense if m_transparentImage lives only when > m_context > > > is > > > > null? > > > > > > I don't fully understand what problem you're trying to solve. > > > > > > I don't think HTMLCanvasElement should hold m_transparentImage. > > > HTMLCanvasElement is just responsible for creating the transparent image and > > > return the image to someone else. If the someone wants to hold the > reference, > > > the someone should do that. > > > > > > So the current design looks reasonable to me. HTMLCanvasElement returns a > > > PassRefPtr to pass the ownership. The receiver is responsible for handling > the > > > ownership. In this particular case, the receiver doesn't need to hold the > > > reference, so the receiver just drops it immediately. This design doesn't > look > > > strange to me. > > > > My intention was that I didn't want a return value of > ImageResource::getImage() > > to be passed the ownership. If the current design is right, maybe what I'm > going > > to do is wrong or difficult and for now ImageResource::getImage() should > return > > a PassRefPtr rather than a raw pointer? > > s/to be passed the ownership/to be reffed by another object/ Sorry, would you elaborate on what you want to achieve? You're saying that you want to drop PassRefPtr from ImageResource::getImage() but in this CL you're dropping PassRefPtr from Editor::imageFromNode and HTMLCanvasElement::copiedImage.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Description was changed from ========== Replace RefPtrs with raw pointers (imageFromNode in Editor.cpp) imageFromNode in Editor.cpp now returns a RefPtr to an Image, but this function creates a return value from ImageResource::getImage(), which returns a raw pointer to ImageResource::m_image. To reduce unnecessary refs of that field, this CL replaces (Pass)RefPtrs with raw pointers. BUG=n/a TEST=webkit_unit_tests --gtest_filter=CanvasRenderingContext2DTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Replace RefPtrs with raw pointers (imageFromNode in Editor.cpp) imageFromNode in Editor.cpp now returns a RefPtr to an Image, but this function creates a return value from ImageResource::getImage(), which returns a raw pointer to ImageResource::m_image. To reduce unnecessary refs of that field, this CL replaces (Pass)RefPtrs with raw pointers. The goal is to change ImageResource::m_image to be a unique_ptr, and this CL is one of all steps. BUG=n/a TEST=webkit_unit_tests --gtest_filter=CanvasRenderingContext2DTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
We chatted in hangout and concluded that it is difficult to change ImageResource::m_image to be a unique_ptr. This would require Image not to be RefCounted and functions like copiedImage which return either a new image or an image owned by an object to be rewritten. Let me close this CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
