Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2041263002:
Remove the use of OwnedPtrDeleter in WebHelperPlugin. (Closed)
|
Created:
4 years, 6 months ago by Yuta Kitamura Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove the use of OwnedPtrDeleter in WebHelperPlugin.
It's hard to mechanically replace specializations of OwnedPtrDeleter<T>
to std::unique_ptr equivalents. Therefore, they are converted to
std::unique_ptr manually.
A new typedef WebHelperPluginUniquePtr is created so people can always
use std::unique_ptr with a correct deleter.
BUG=617504
Committed: https://crrev.com/fb9b91321850375af904a3f9fc017ee18a9d32b3
Cr-Commit-Position: refs/heads/master@{#398508}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Experimentally remove destruction timer. #Patch Set 3 : Revert to PS1. #
Messages
Total messages: 33 (14 generated)
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041263002/1
yutak@chromium.org changed reviewers: + dcheng@chromium.org, tzik@chromium.org
yutak@chromium.org changed required reviewers: + dcheng@chromium.org, tzik@chromium.org
tzik & dcheng: PTAL? dcheng: Also review as an OWNER of web/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2041263002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebHelperPluginImpl.cpp (right): https://codereview.chromium.org/2041263002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebHelperPluginImpl.cpp:44: WebHelperPlugin* WebHelperPlugin::create(const WebString& pluginType, WebLocalFrame* frame) Does our policy forbid to return std::unique_ptr here? If not, could you replace the raw ptr here with unique_ptr.
https://codereview.chromium.org/2041263002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebHelperPluginImpl.cpp (right): https://codereview.chromium.org/2041263002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebHelperPluginImpl.cpp:44: WebHelperPlugin* WebHelperPlugin::create(const WebString& pluginType, WebLocalFrame* frame) On 2016/06/07 06:57:18, tzik wrote: > Does our policy forbid to return std::unique_ptr here? > If not, could you replace the raw ptr here with unique_ptr. I think that's okay; would like to hear from dcheng on the context on the design of this.
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041263002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2041263002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebHelperPluginImpl.cpp (right): https://codereview.chromium.org/2041263002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebHelperPluginImpl.cpp:44: WebHelperPlugin* WebHelperPlugin::create(const WebString& pluginType, WebLocalFrame* frame) On 2016/06/07 at 07:21:03, Yuta Kitamura wrote: > On 2016/06/07 06:57:18, tzik wrote: > > Does our policy forbid to return std::unique_ptr here? > > If not, could you replace the raw ptr here with unique_ptr. > > I think that's okay; would like to hear from dcheng on > the context on the design of this. For other Blink public APIs, we tried to avoid exposing the refcounting nature of Blink in the public API, so we used create() / destroy() / close() functions. I don't think that applies here though: this thing has unique ownership, so we should probably just return std::unique_ptr and make things simpler. Unfortunately, it looks like destroy() deletes the object in a timer... ideally we should try to get rid of the need for the custom deleter if we expose it. Can you see if it's still necessary?
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041263002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Apparently, removing the destruction timer causes two browser_tests failures: ECKEncryptedMediaTest.CDMCrashDuringDecode ECKEncryptedMediaTest.CDMExpectedCrash So it could be non-trivial to actually clear it up. Reviewers: do you agree on landing the Patch Set 1 (possibly with a TODO comment added) meanwhile? The existence of OwnedPtrDeleter is currently a blocker of my work.
PS1 LGTM. We can figure out how to clean this up (and what we want to do with classes in the public API) in a separate CL.
tzik: How about you? (re: landing PS1)
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041263002/40001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yutak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2041263002/#ps40001 (title: "Revert to PS1.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041263002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove the use of OwnedPtrDeleter in WebHelperPlugin. It's hard to mechanically replace specializations of OwnedPtrDeleter<T> to std::unique_ptr equivalents. Therefore, they are converted to std::unique_ptr manually. A new typedef WebHelperPluginUniquePtr is created so people can always use std::unique_ptr with a correct deleter. BUG=617504 ========== to ========== Remove the use of OwnedPtrDeleter in WebHelperPlugin. It's hard to mechanically replace specializations of OwnedPtrDeleter<T> to std::unique_ptr equivalents. Therefore, they are converted to std::unique_ptr manually. A new typedef WebHelperPluginUniquePtr is created so people can always use std::unique_ptr with a correct deleter. BUG=617504 Committed: https://crrev.com/fb9b91321850375af904a3f9fc017ee18a9d32b3 Cr-Commit-Position: refs/heads/master@{#398508} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fb9b91321850375af904a3f9fc017ee18a9d32b3 Cr-Commit-Position: refs/heads/master@{#398508}
Message was sent while issue was closed.
Patchset #4 (id:60001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
