Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1784433003:
Track CTR of uncommon download warning. (Closed)
|
Created:
4 years, 9 months ago by Jialiu Lin Modified:
4 years, 9 months ago CC:
asanka, chromium-reviews, darin-cc_chromium.org, grt+watch_chromium.org, jam, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack CTR of uncommon download warning.
1) When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR.
2) add browsertest in DownloadTest to capture this new feature.
BUG=574590
Committed: https://crrev.com/bd9ba658cc76a6112bf1327720c457251bd6ec44
Cr-Commit-Position: refs/heads/master@{#382067}
Patch Set 1 #Patch Set 2 : cl format #
Total comments: 6
Patch Set 3 : address asanka's comment #Patch Set 4 : nit #
Total comments: 4
Patch Set 5 : change to ON_DANGEROUS_DOWNLOAD_QUIT #
Total comments: 8
Patch Set 6 : Add FakeDownloadProtectionService #Patch Set 7 : remove empty line #Patch Set 8 : Fix all the breaking tests #Patch Set 9 : move function inside FULL_SAFE_BROWSING macros #
Total comments: 6
Patch Set 10 : nit #
Total comments: 7
Patch Set 11 : Add one more test #
Total comments: 1
Messages
Total messages: 34 (11 generated)
Description was changed from ========== Track CTR of uncommon download warning. BUG= ========== to ========== Track CTR of uncommon download warning. When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR: BUG=574590 ==========
Description was changed from ========== Track CTR of uncommon download warning. When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR: BUG=574590 ========== to ========== Track CTR of uncommon download warning. 1) When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR. 2) add browsertest in DownloadTest to capture this new feature. 3) Minor change in test fixture content/public/test/download_test_observer.* to facilitate #2) BUG=574590 ==========
jialiul@chromium.org changed reviewers: + rdsmith@chromium.org
Hi rdsmith@, Could you take a look? Thanks!
rdsmith@chromium.org changed reviewers: + asanka@chromium.org - rdsmith@chromium.org
Asanka's a better person than I for this type of thing, if he's available.
On 2016/03/10 15:54:45, Randy Smith - Not in Fridays wrote: > Asanka's a better person than I for this type of thing, if he's available. Sure. Thanks, Randy. +asanka@, could you take a look when you have time? Thanks!
This only works if the user keeps the download via the download shelf context menu, correct? What about the downloads page? https://codereview.chromium.org/1784433003/diff/20001/chrome/browser/download... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/20001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:272: dangerous_downloads_seen().insert(download->GetId()); This is inserting an item into a temporary set that's immediately destroyed. https://codereview.chromium.org/1784433003/diff/20001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:513: safe_browsing::SafeBrowsingService::RegisterFactory( Why is this necessary to be in SetUp() instead of SetUpOnMainThread()? https://codereview.chromium.org/1784433003/diff/20001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:526: feature_list->InitializeFromCommandLine(features::kDownloadResumption.name, Nit: There's a number of formatting only changes that are unnecessary for this CL. Would you mind reverting them?
Hi Asanka, Thanks for your review! Yes, this CL only deals with the Keep button on the download shelf. The part that tracks chrome://downloads page was already checked in last year (https://codereview.chromium.org/1436273002). https://codereview.chromium.org/1784433003/diff/20001/chrome/browser/download... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/20001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:272: dangerous_downloads_seen().insert(download->GetId()); On 2016/03/11 03:43:26, asanka wrote: > This is inserting an item into a temporary set that's immediately destroyed. Oops, should be return by reference instead. fixed in download_test.observer.* files. https://codereview.chromium.org/1784433003/diff/20001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:513: safe_browsing::SafeBrowsingService::RegisterFactory( On 2016/03/11 03:43:26, asanka wrote: > Why is this necessary to be in SetUp() instead of SetUpOnMainThread()? Because the registerFactory function needs to be called before InProcessBrowserTest::SetUp(), while SetUpOnMainThread() is in fact called after the SetUp(). Also, there are other steps between SetUp() and SetUpOnMainThread(), so merge SetUpOnMainThread() to SetUp() doesn't work either. https://codereview.chromium.org/1784433003/diff/20001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:526: feature_list->InitializeFromCommandLine(features::kDownloadResumption.name, On 2016/03/11 03:43:26, asanka wrote: > Nit: There's a number of formatting only changes that are unnecessary for this > CL. Would you mind reverting them? No problem.... seems I need to be more careful with 'git cl format'. :-)
https://codereview.chromium.org/1784433003/diff/60001/chrome/browser/download... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/60001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:254: class DownloadTestObserverAcceptUncommon Rather than going this route, can you create a DownloadTestObserverTerminal with a danger action of ON_DANGEROUS_DOWNLOAD_QUIT? Then your observer will quit when it sees the dangerous download and then the test can invoke DownloadCommands::ExecuteCommand(). This way you don't need to expose the test observer guts. https://codereview.chromium.org/1784433003/diff/60001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:526: feature_list->InitializeFromCommandLine(features::kDownloadResumption.name, Nit: unnecessary change.
Thanks Asanka! This is brilliant suggestion. I guess I was too obsessed with mimicking the "Keep" behavior. https://codereview.chromium.org/1784433003/diff/60001/chrome/browser/download... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/60001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:254: class DownloadTestObserverAcceptUncommon On 2016/03/11 22:05:32, asanka wrote: > Rather than going this route, can you create a DownloadTestObserverTerminal with > a danger action of ON_DANGEROUS_DOWNLOAD_QUIT? Then your observer will quit when > it sees the dangerous download and then the test can invoke > DownloadCommands::ExecuteCommand(). > > This way you don't need to expose the test observer guts. This is brilliant! https://codereview.chromium.org/1784433003/diff/60001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:526: feature_list->InitializeFromCommandLine(features::kDownloadResumption.name, On 2016/03/11 22:05:32, asanka wrote: > Nit: unnecessary change. Done.
https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:393: class FakeSafeBrowsingService : public safe_browsing::SafeBrowsingService { This the 7th FakeSafeBrowsingService :-(. Not necessarily for this CL, but we really should make this a test fixture under chrome/browser/safe_browsing/. https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:403: // Disable download protection to prevent flackiness. *flakiness. Also, you'll need to explain how disabling the download protection service at this point prevents flakiness. https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:419: if (!fake_safe_browsing_service_) { This function should be getting called at most once per browser session. It's acceptable to verify it and rely on it. I.e. DCHECK(!fake_safe_browsing_service_) and always create one. Otherwise it appears from this test that there's uncertainty about whether multiple safe browsing service instances could be created. https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:3353: download->OnContentCheckCompleted( OnContentCheckCompleted cannot be issued until all the data has been saved. The test will crash if OnAllDataSaved() hasn't been called prior to this point. Since the in_progress_observer doesn't enforce it, we may end up being flaky. :-( In decreasing order of preference, here's what I think should happen instead: * Make DownloadProtectionService flag the download as UNCOMMON. One approach would be to introduce a new FakeDownloadProtectionService that could be returned by FakeSafeBrowsingService::download_protection_service(). In response to FakeDownloadProtectionService::CheckClientDownload(), it can respond with a verdict of UNCOMMON. Then you can wait for the dangerous_observer to complete without calling OnContentCheckCompleted(). You can call DownloadCommands::ExecuteCommand(KEEP) after this. * Failing that, add a DownloadTestObserver derivative that waits for DownloadItem::AllDataSaved() to be true. Since the download is expected to be marked as dangerous, receiving all the data will not result in the download being marked as complete. Once AllDataSaved() becomes true, you can safely call OnContentCheckCompleted(). You only need the DownloadAllDataSavedObserver (which you wait on before calling OnCOntentCheckCompleted) and a DownloadTestObserverTerminal that you wait on at the end to make sure that the download runs to completion.
Thank you so much for walking me around this download browser test, Asanka! https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:393: class FakeSafeBrowsingService : public safe_browsing::SafeBrowsingService { On 2016/03/14 18:13:46, asanka wrote: > This the 7th FakeSafeBrowsingService :-(. Not necessarily for this CL, but we > really should make this a test fixture under chrome/browser/safe_browsing/. Totally agree. Bug created at https://crbug.com/594683 https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:403: // Disable download protection to prevent flackiness. On 2016/03/14 18:13:46, asanka wrote: > *flakiness. > > Also, you'll need to explain how disabling the download protection service at > this point prevents flakiness. No need of this line since FakeDownloadProtectionService is used. https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:419: if (!fake_safe_browsing_service_) { On 2016/03/14 18:13:46, asanka wrote: > This function should be getting called at most once per browser session. It's > acceptable to verify it and rely on it. I.e. > DCHECK(!fake_safe_browsing_service_) and always create one. Otherwise it appears > from this test that there's uncertainty about whether multiple safe browsing > service instances could be created. Done. https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... chrome/browser/download/download_browsertest.cc:3353: download->OnContentCheckCompleted( On 2016/03/14 18:13:46, asanka wrote: > OnContentCheckCompleted cannot be issued until all the data has been saved. The > test will crash if OnAllDataSaved() hasn't been called prior to this point. > Since the in_progress_observer doesn't enforce it, we may end up being flaky. > :-( > > In decreasing order of preference, here's what I think should happen instead: > > * Make DownloadProtectionService flag the download as UNCOMMON. One approach > would be to introduce a new FakeDownloadProtectionService that could be returned > by FakeSafeBrowsingService::download_protection_service(). In response to > FakeDownloadProtectionService::CheckClientDownload(), it can respond with a > verdict of UNCOMMON. Then you can wait for the dangerous_observer to complete > without calling OnContentCheckCompleted(). You can call > DownloadCommands::ExecuteCommand(KEEP) after this. > > * Failing that, add a DownloadTestObserver derivative that waits for > DownloadItem::AllDataSaved() to be true. Since the download is expected to be > marked as dangerous, receiving all the data will not result in the download > being marked as complete. Once AllDataSaved() becomes true, you can safely call > OnContentCheckCompleted(). You only need the DownloadAllDataSavedObserver (which > you wait on before calling OnCOntentCheckCompleted) and a > DownloadTestObserverTerminal that you wait on at the end to make sure that the > download runs to completion. implemented based on option 1 here. Now , no need to create an in_progress_observer.
On 2016/03/14 23:54:04, JialiuLin wrote: > Thank you so much for walking me around this download browser test, Asanka! > > https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... > File chrome/browser/download/download_browsertest.cc (right): > > https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... > chrome/browser/download/download_browsertest.cc:393: class > FakeSafeBrowsingService : public safe_browsing::SafeBrowsingService { > On 2016/03/14 18:13:46, asanka wrote: > > This the 7th FakeSafeBrowsingService :-(. Not necessarily for this CL, but we > > really should make this a test fixture under chrome/browser/safe_browsing/. > Totally agree. Bug created at https://crbug.com/594683 > > https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... > chrome/browser/download/download_browsertest.cc:403: // Disable download > protection to prevent flackiness. > On 2016/03/14 18:13:46, asanka wrote: > > *flakiness. > > > > Also, you'll need to explain how disabling the download protection service at > > this point prevents flakiness. > > No need of this line since FakeDownloadProtectionService is used. > > https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... > chrome/browser/download/download_browsertest.cc:419: if > (!fake_safe_browsing_service_) { > On 2016/03/14 18:13:46, asanka wrote: > > This function should be getting called at most once per browser session. It's > > acceptable to verify it and rely on it. I.e. > > DCHECK(!fake_safe_browsing_service_) and always create one. Otherwise it > appears > > from this test that there's uncertainty about whether multiple safe browsing > > service instances could be created. > > Done. > > https://codereview.chromium.org/1784433003/diff/80001/chrome/browser/download... > chrome/browser/download/download_browsertest.cc:3353: > download->OnContentCheckCompleted( > On 2016/03/14 18:13:46, asanka wrote: > > OnContentCheckCompleted cannot be issued until all the data has been saved. > The > > test will crash if OnAllDataSaved() hasn't been called prior to this point. > > Since the in_progress_observer doesn't enforce it, we may end up being flaky. > > :-( > > > > In decreasing order of preference, here's what I think should happen instead: > > > > * Make DownloadProtectionService flag the download as UNCOMMON. One approach > > would be to introduce a new FakeDownloadProtectionService that could be > returned > > by FakeSafeBrowsingService::download_protection_service(). In response to > > FakeDownloadProtectionService::CheckClientDownload(), it can respond with a > > verdict of UNCOMMON. Then you can wait for the dangerous_observer to complete > > without calling OnContentCheckCompleted(). You can call > > DownloadCommands::ExecuteCommand(KEEP) after this. > > > > * Failing that, add a DownloadTestObserver derivative that waits for > > DownloadItem::AllDataSaved() to be true. Since the download is expected to be > > marked as dangerous, receiving all the data will not result in the download > > being marked as complete. Once AllDataSaved() becomes true, you can safely > call > > OnContentCheckCompleted(). You only need the DownloadAllDataSavedObserver > (which > > you wait on before calling OnCOntentCheckCompleted) and a > > DownloadTestObserverTerminal that you wait on at the end to make sure that the > > download runs to completion. > implemented based on option 1 here. Now , no need to create an > in_progress_observer. Oops, breaking other tests. Truly apologize. Will ping you again when I get them fixed.
jialiul@chromium.org changed reviewers: + nparker@chromium.org
Hi Asanka, All try bots are green now. Also add Nathan to the reviewer list. Thanks, Jialiu
LGTM % nits. Thanks! Also you need to update the CL description. Eventually I'd like to move the reporting code into chrome/browser/safe_browsing. But since the parallel code is already there in chrome/browser/downloads, I don't mind it for this CL. https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:1144: InProcessBrowserTest::SetUp(); Nit: You want to do DownloadTest::SetUp(). And similarly for TearDown(). https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... File chrome/browser/download/download_commands.cc (right): https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:282: // Only sends uncommand download accept report if : *uncommon https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:286: // 4. User is not in incognito mode. Is the indentation due to clang-format?
Description was changed from ========== Track CTR of uncommon download warning. 1) When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR. 2) add browsertest in DownloadTest to capture this new feature. 3) Minor change in test fixture content/public/test/download_test_observer.* to facilitate #2) BUG=574590 ========== to ========== Track CTR of uncommon download warning. 1) When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR. 2) add browsertest in DownloadTest to capture this new feature. BUG=574590 ==========
Thanks for your thorough review, Asanka! https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:1144: InProcessBrowserTest::SetUp(); On 2016/03/16 01:29:50, asanka wrote: > Nit: You want to do DownloadTest::SetUp(). And similarly for TearDown(). Done. https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... File chrome/browser/download/download_commands.cc (right): https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:282: // Only sends uncommand download accept report if : On 2016/03/16 01:29:51, asanka wrote: > *uncommon Done. https://codereview.chromium.org/1784433003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:286: // 4. User is not in incognito mode. On 2016/03/16 01:29:50, asanka wrote: > Is the indentation due to clang-format? hmm, git cl format did funny thing again. Let me change it back.
Hi Nathan, Could you take a look? Thanks!
(trying out new codereview UI -- lemme know if this looks weird). https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:3351: IN_PROC_BROWSER_TEST_F( Do you want to extend one of the other tests to verify that it doesn't general the report if the user didn't proceed? https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_commands.cc (right): https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:289: content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT && Remind me why this is just for uncommon downloads, and not for uws/malware? https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:306: DLOG(ERROR) nit: if/else code > 1 line should have {}'s You could instead DCHECK(false) << ".." since this should make any debug tests fail quickly -- it means the proto has required fields you're not setting, which we should catch right away.
https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_commands.cc (right): https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:289: content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT && On 2016/03/17 at 17:33:12, Nathan Parker wrote: > Remind me why this is just for uncommon downloads, and not for uws/malware? Nevermind, just figured it out (only uncommon has "keep" button).
Thanks, Nathan. Your comments are all addressed. https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:3351: IN_PROC_BROWSER_TEST_F( On 2016/03/17 at 17:33:12, Nathan Parker wrote: > Do you want to extend one of the other tests to verify that it doesn't general the report if the user didn't proceed? Done https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_commands.cc (right): https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:289: content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT && On 2016/03/17 at 17:34:39, Nathan Parker wrote: > On 2016/03/17 at 17:33:12, Nathan Parker wrote: > > Remind me why this is just for uncommon downloads, and not for uws/malware? > > Nevermind, just figured it out (only uncommon has "keep" button). Yes, only uncommon has keep button. https://codereview.chromium.org/1784433003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:306: DLOG(ERROR) On 2016/03/17 at 17:33:12, Nathan Parker wrote: > nit: if/else code > 1 line should have {}'s > > You could instead DCHECK(false) << ".." since this should make any debug tests fail quickly -- it means the proto has required fields you're not setting, which we should catch right away. Done
lgtm
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1784433003/#ps200001 (title: "Add one more test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1784433003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1784433003/200001
Message was sent while issue was closed.
Description was changed from ========== Track CTR of uncommon download warning. 1) When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR. 2) add browsertest in DownloadTest to capture this new feature. BUG=574590 ========== to ========== Track CTR of uncommon download warning. 1) When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR. 2) add browsertest in DownloadTest to capture this new feature. BUG=574590 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Track CTR of uncommon download warning. 1) When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR. 2) add browsertest in DownloadTest to capture this new feature. BUG=574590 ========== to ========== Track CTR of uncommon download warning. 1) When user click through a uncommon download warning, a serialized CSD ClientSafeBrowsingReportRequest will be sent to track CTR. 2) add browsertest in DownloadTest to capture this new feature. BUG=574590 Committed: https://crrev.com/bd9ba658cc76a6112bf1327720c457251bd6ec44 Cr-Commit-Position: refs/heads/master@{#382067} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/bd9ba658cc76a6112bf1327720c457251bd6ec44 Cr-Commit-Position: refs/heads/master@{#382067}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
/b/build/slave/ClangToTLinux/build/src/third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: while linking browser_tests: symbol 'TestSafeBrowsingServiceFactory::CreateSafeBrowsingService()' defined in multiple places (possible ODR violation): ../../chrome/browser/download/download_danger_prompt_browsertest.cc:61 from obj/chrome/browser/download/browser_tests.download_danger_prompt_browsertest.o ../../chrome/browser/download/download_browsertest.cc:1121 from obj/chrome/browser/download/browser_tests.download_browsertest.o /b/build/slave/ClangToTLinux/build/src/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating warnings as errors https://codereview.chromium.org/1784433003/diff/200001/chrome/browser/downloa... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1784433003/diff/200001/chrome/browser/downloa... chrome/browser/download/download_browsertest.cc:1116: class TestSafeBrowsingServiceFactory This is 8th TestSafeBrowsingServiceFactory we have: https://code.google.com/p/chromium/codesearch#search/&q=TestSafeBrowsingServi... It's also not in an unnamed namespace and different from the others, so it causes ODR troubles in practice: https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/4650/... I don't know why the normal bots didn't catch this, but please fix. (I'll paste the error in a second, but pasting it into this text box reproducibly crashes chrome's renderer.)
Message was sent while issue was closed.
On 2016/03/21 at 16:03:42, thakis wrote: > /b/build/slave/ClangToTLinux/build/src/third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: while linking browser_tests: symbol 'TestSafeBrowsingServiceFactory::CreateSafeBrowsingService()' defined in multiple places (possible ODR violation): > ../../chrome/browser/download/download_danger_prompt_browsertest.cc:61 from obj/chrome/browser/download/browser_tests.download_danger_prompt_browsertest.o > ../../chrome/browser/download/download_browsertest.cc:1121 from obj/chrome/browser/download/browser_tests.download_browsertest.o > /b/build/slave/ClangToTLinux/build/src/third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating warnings as errors > > https://codereview.chromium.org/1784433003/diff/200001/chrome/browser/downloa... > File chrome/browser/download/download_browsertest.cc (right): > > https://codereview.chromium.org/1784433003/diff/200001/chrome/browser/downloa... > chrome/browser/download/download_browsertest.cc:1116: class TestSafeBrowsingServiceFactory > This is 8th TestSafeBrowsingServiceFactory we have: https://code.google.com/p/chromium/codesearch#search/&q=TestSafeBrowsingServi... > > It's also not in an unnamed namespace and different from the others, so it causes ODR troubles in practice: > > https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/4650/... > > I don't know why the normal bots didn't catch this, but please fix. > > (I'll paste the error in a second, but pasting it into this text box reproducibly crashes chrome's renderer.) Sorry about this problem. I'll move them to unnamed namespace in a minutes. W.R.T the 8th TestSafeBrowsingService/TestSafeBrowsingServiceFactory, https://crbug.com/594683 is filed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
