Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2490183002:
Filter webrtc::RTCStats by whitelist in surfacing them to Blink. (Closed)
|
Created:
4 years, 1 month ago by hbos_chromium Modified:
4 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFilter webrtc::RTCStats by whitelist in surfacing them to Blink.
This ensures that we don't accidentally leak RTCStats dictionaries
that are not ready to be exposed to the web, and makes it so that
exposing a new dictionary requires a Chromium CL (not just WebRTC
repo CLs).
Note that the white list is not applied to members of whitelisted
dictionaries and that all such members are exposed.
BUG=chromium:627816
Committed: https://crrev.com/20033c03bd6c08920f93544110a263a13e256c2f
Cr-Commit-Position: refs/heads/master@{#434501}
Patch Set 1 : Problem with static const char kType[] comparison #Patch Set 2 : Whitelist backed up by std::set instead to avoid const char* equality problem #Patch Set 3 : Fixed compile error #Patch Set 4 : Fix test: WhitelistStatsForTesting to allow RTCTestStats in rtc_peer_connection_handler_unittest.cc #Patch Set 5 : Make win compile: BLINK_PLATFORM_EXPORT and cpp file (also rebase /w master) #
Total comments: 2
Patch Set 6 : git cl format: {\n} -> {} #
Messages
Total messages: 65 (45 generated)
Patchset #1 (id:1) has been deleted
hbos@chromium.org changed reviewers: + hta@chromium.org
This shows the problem I'm facing with the rtc_stats.cc and rtc_stats_unittest.cc... If you don't CONTENT_EXPORT you get link errors instead by the unittest trying to reference rtc_stats.h things. I'm going to see if this is a problem only in the unittest or if webrtc-created stats also fail the kType check.
The CQ bit was checked by hbos@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...
Please take a look, hta. I'm thinking that with chromium being built the way it is we shouldn't rely on kType == kType and instead do a string comparison, so I got around this problem by using an std::set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hbos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/22 09:45:33, hbos_chromium wrote: > Please take a look, hta. > > I'm thinking that with chromium being built the way it is we shouldn't rely on > kType == kType and instead do a string comparison, so I got around this problem > by using an std::set. Hang on, I'll fix the errors before that.
The CQ bit was checked by hbos@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...
Please take a look, hta
Description was changed from ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. BUG=chromium:627816 ========== to ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Blink CL (not just WebRTC repo CLs). BUG=chromium:627816 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by hbos@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...
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:100001) has been deleted
hbos@chromium.org changed reviewers: + foolip@chromium.org
And please take a look foolip
https://codereview.chromium.org/2490183002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebRTCStats.cpp (right): https://codereview.chromium.org/2490183002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebRTCStats.cpp:9: WebRTCStatsReport::~WebRTCStatsReport() { Do you have other examples of where the destructor had to be outlined to fix the windows build? Can you document here why it's done like this?
https://msdn.microsoft.com/en-us/library/3tdb471s.aspx is the documentation for the compiler error, claims "An exported class was derived from a class that was not exported."
https://codereview.chromium.org/2490183002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebRTCStats.cpp (right): https://codereview.chromium.org/2490183002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebRTCStats.cpp:9: WebRTCStatsReport::~WebRTCStatsReport() { On 2016/11/24 12:26:30, foolip wrote: > Do you have other examples of where the destructor had to be outlined to fix the > windows build? Can you document here why it's done like this? No, I don't understand this. Most examples have a .cpp file and a .h file, but there are cases of inlined virual destructors that are BLINK_PLATFORM_EXPORT'ed.
On 2016/11/24 12:33:50, foolip wrote: > https://msdn.microsoft.com/en-us/library/3tdb471s.aspx is the documentation for > the compiler error, claims "An exported class was derived from a class that was > not exported." It might have something to do with BLINK_PLATFORM_EXPORT being defined as "dllimport" or "dllexport" depending on the context. So an imported interface "was not exported" I guess. Moving it to the .cpp gets around this problem, but I don't understand why.
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by hbos@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...
Description was changed from ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Blink CL (not just WebRTC repo CLs). BUG=chromium:627816 ========== to ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Blink CL (not just WebRTC repo CLs). Note that the white list is not applied to members of whitelisted dictionaries and that all such members are exposed. BUG=chromium:627816 ==========
On 2016/11/24 12:56:59, hbos_chromium wrote: > On 2016/11/24 12:33:50, foolip wrote: > > https://msdn.microsoft.com/en-us/library/3tdb471s.aspx is the documentation > for > > the compiler error, claims "An exported class was derived from a class that > was > > not exported." > > It might have something to do with BLINK_PLATFORM_EXPORT being defined as > "dllimport" or "dllexport" depending on the context. So an imported interface > "was not exported" I guess. Moving it to the .cpp gets around this problem, but > I don't understand why. Looks like "= default;" on the destructor in the header file also gets around this problem (for some reason).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm This is my lgtm day. Note, however: content/ is still not Blink. Please s/Blink/Chrome/ in the CL description.
The CQ bit was checked by hbos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Can't say I understand the compile failure, but LGTM when you're satisfied. Might be worth explaining anything that's odd in a comment and/or CL description.
Patchset #8 (id:240001) has been deleted
Patchset #7 (id:220001) has been deleted
Patchset #6 (id:200001) has been deleted
On 2016/11/25 10:19:30, foolip wrote: > Can't say I understand the compile failure, but LGTM when you're satisfied. > Might be worth explaining anything that's odd in a comment and/or CL > description. I'm not 100% sure, but based on previous (now deleted) patch sets where it was working with an empty WebRTCStats.cpp file that did nothing except for including the header (even though everything was inlined and entirely defined in the .h) but it wasn't working if that .cpp file was removed, I have a strong feeling that this has to do with whether or not a lonesome .h file included in the compiled component or not. I've had similar issues before on some obscure osx/ios bot in a different repo for different types of changes. I'm landing this with .h declarations and .cpp definitions for the sake of ensuring a .cpp in the "platform" component is including that header. Everything is successfully BLINK_PLATFORM_EXPORT'ed.
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2490183002/#ps160001 (title: "Make win compile: BLINK_PLATFORM_EXPORT and cpp file (also rebase /w master)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2490183002/#ps260001 (title: "git cl format: {\n} -> {}")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Blink CL (not just WebRTC repo CLs). Note that the white list is not applied to members of whitelisted dictionaries and that all such members are exposed. BUG=chromium:627816 ========== to ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Chromium CL (not just WebRTC repo CLs). Note that the white list is not applied to members of whitelisted dictionaries and that all such members are exposed. BUG=chromium:627816 ==========
The CQ bit was checked by hbos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1480081752330360,
"parent_rev": "8df775a26855247354d749c1a3a56f31d9412065", "commit_rev":
"71896c718514da05f45f71cfe4b6e72137afcf06"}
Message was sent while issue was closed.
Description was changed from ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Chromium CL (not just WebRTC repo CLs). Note that the white list is not applied to members of whitelisted dictionaries and that all such members are exposed. BUG=chromium:627816 ========== to ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Chromium CL (not just WebRTC repo CLs). Note that the white list is not applied to members of whitelisted dictionaries and that all such members are exposed. BUG=chromium:627816 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Chromium CL (not just WebRTC repo CLs). Note that the white list is not applied to members of whitelisted dictionaries and that all such members are exposed. BUG=chromium:627816 ========== to ========== Filter webrtc::RTCStats by whitelist in surfacing them to Blink. This ensures that we don't accidentally leak RTCStats dictionaries that are not ready to be exposed to the web, and makes it so that exposing a new dictionary requires a Chromium CL (not just WebRTC repo CLs). Note that the white list is not applied to members of whitelisted dictionaries and that all such members are exposed. BUG=chromium:627816 Committed: https://crrev.com/20033c03bd6c08920f93544110a263a13e256c2f Cr-Commit-Position: refs/heads/master@{#434501} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/20033c03bd6c08920f93544110a263a13e256c2f Cr-Commit-Position: refs/heads/master@{#434501} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
