Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1633363002:
Remove NPAPI Flash migration code. (Closed)
|
Created:
4 years, 10 months ago by Will Harris Modified:
4 years, 10 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove NPAPI Flash migration code.
BUG=514250
TEST=Make sure Flash still works and you can enable/disable it.
Committed: https://crrev.com/73eb7a1940c659477308eaf6f195aebf0fe3c738
Cr-Commit-Position: refs/heads/master@{#371822}
Patch Set 1 #
Total comments: 4
Patch Set 2 : static_cast #Messages
Total messages: 13 (6 generated)
Description was changed from ========== Remove NPAPI Flash migration code. BUG=514250 TEST=Make sure Flash still works and you can enable/disable it. ========== to ========== Remove NPAPI Flash migration code. BUG=514250 TEST=Make sure Flash still works and you can enable/disable it. ==========
wfh@chromium.org changed reviewers: + bauerb@chromium.org
This code is seriously gnarly... see issue 581569 I raised while doing this. I think this is safe though and completes my TODO. https://codereview.chromium.org/1633363002/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_prefs.cc (right): https://codereview.chromium.org/1633363002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_prefs.cc:342: reinterpret_cast<base::DictionaryValue*>(plugin_value); some code uses reinterpret_cast and some uses static_cast here. I chose reinterpret...
LGTM with one issue addressed: https://codereview.chromium.org/1633363002/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_prefs.cc (right): https://codereview.chromium.org/1633363002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_prefs.cc:342: reinterpret_cast<base::DictionaryValue*>(plugin_value); On 2016/01/27 02:17:01, Will Harris wrote: > some code uses reinterpret_cast and some uses static_cast here. I chose > reinterpret... Urr... no, I think static_cast is the correct one here. reinterpret_cast literally means "interpret this pointer as that pointer", which would get e.g. vtable offsets wrong, whereas static_cast will do the necessary adjustments and also check that the types are in a subtype relation.
Thanks for review https://codereview.chromium.org/1633363002/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_prefs.cc (right): https://codereview.chromium.org/1633363002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_prefs.cc:342: reinterpret_cast<base::DictionaryValue*>(plugin_value); On 2016/01/27 15:51:43, Bernhard Bauer wrote: > On 2016/01/27 02:17:01, Will Harris wrote: > > some code uses reinterpret_cast and some uses static_cast here. I chose > > reinterpret... > > Urr... no, I think static_cast is the correct one here. reinterpret_cast > literally means "interpret this pointer as that pointer", which would get e.g. > vtable offsets wrong, whereas static_cast will do the necessary adjustments and > also check that the types are in a subtype relation. okay thanks! Should go and fix these or are these safe? https://code.google.com/p/chromium/codesearch#search/&q=%22reinterpret_cast%3...
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1633363002/#ps20001 (title: "static_cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1633363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1633363002/20001
https://codereview.chromium.org/1633363002/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_prefs.cc (right): https://codereview.chromium.org/1633363002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_prefs.cc:342: reinterpret_cast<base::DictionaryValue*>(plugin_value); On 2016/01/27 17:03:51, Will Harris wrote: > On 2016/01/27 15:51:43, Bernhard Bauer wrote: > > On 2016/01/27 02:17:01, Will Harris wrote: > > > some code uses reinterpret_cast and some uses static_cast here. I chose > > > reinterpret... > > > > Urr... no, I think static_cast is the correct one here. reinterpret_cast > > literally means "interpret this pointer as that pointer", which would get e.g. > > vtable offsets wrong, whereas static_cast will do the necessary adjustments > and > > also check that the types are in a subtype relation. > > okay thanks! Should go and fix these or are these safe? > > https://code.google.com/p/chromium/codesearch#search/&q=%22reinterpret_cast%3... Uh, both? 😄 I think they are effectively safe for DictionaryValue because there is no vtable adjustment necessary, but static_cast is more idiomatic, and safer if someone tries to do this for other types, so it would be nice to fix the others. Thanks!
Message was sent while issue was closed.
Description was changed from ========== Remove NPAPI Flash migration code. BUG=514250 TEST=Make sure Flash still works and you can enable/disable it. ========== to ========== Remove NPAPI Flash migration code. BUG=514250 TEST=Make sure Flash still works and you can enable/disable it. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove NPAPI Flash migration code. BUG=514250 TEST=Make sure Flash still works and you can enable/disable it. ========== to ========== Remove NPAPI Flash migration code. BUG=514250 TEST=Make sure Flash still works and you can enable/disable it. Committed: https://crrev.com/73eb7a1940c659477308eaf6f195aebf0fe3c738 Cr-Commit-Position: refs/heads/master@{#371822} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/73eb7a1940c659477308eaf6f195aebf0fe3c738 Cr-Commit-Position: refs/heads/master@{#371822} |
