Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
187393005:
Make it possible to read CLD data from a file (Closed)
|
Created:
6 years, 9 months ago by Andrew Hayden (chromium.org) Modified:
6 years, 9 months ago Reviewers:
hajimehoshi, MAD, Philippe, jochen (gone - plz use gerrit), Peter Beverloo, Takashi Toyoshima, bulach, andrewhayden, gubalav, palmer, ulas CC:
chromium-reviews, aurimas (slooooooooow), Miguel Garcia Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake it possible to read CLD data from a file.
Note that this change DOES NOT "opt-in" any platform at this time, it merely
makes it possible to read the CLD data in from a file if cld_version=2 and
cld_dynamic=1. Subsequent changes will be required for platforms to "opt-in"
to using this approach; e.g., each platform will have to generate their data
file and package it appropriately before turning this feature on in compile
flags.
IMPORTANT: Can't be enabled on on Android until the SIGBUS in CLD is fixed:
https://code.google.com/p/cld2/issues/detail?id=11
BUG=326023
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259900
Patch Set 1 #Patch Set 2 : Incrementally approaching a working implementation. #Patch Set 3 : Now fully functional and debugged, but we need to package the file. #
Total comments: 27
Patch Set 4 : Rebase onto the CLD2 deps roll change, update readme #
Total comments: 10
Patch Set 5 : Address Marcus' and Jochen's comments #
Total comments: 39
Patch Set 6 : Fix typo in gyp file #Patch Set 7 : Address Palmer's comments #Patch Set 8 : toyoshim@'s comments #Patch Set 9 : Hack around lack of common IsTranslatableURL #
Total comments: 15
Patch Set 10 : Rebase atop the cld_2 DEPS roll #Patch Set 11 : Rebase onto latest master #
Total comments: 1
Patch Set 12 : Instantiate file lock lazily #Patch Set 13 : Moar threadsafety #Patch Set 14 : Rebase onto latest master (incl CLD2 size options commit) #Patch Set 15 : Fix gyp grammar and missing comma #Patch Set 16 : Rebase onto latest master #
Total comments: 43
Patch Set 17 : Rebase onto latest master #Patch Set 18 : Address Marcus' comments #
Total comments: 40
Patch Set 19 : Add offset/length at Ulas' request #Patch Set 20 : Address bulach@'s and palmer@'s comments #Patch Set 21 : Use LazyInstance::Leaky for the mmap pointer #
Total comments: 4
Patch Set 22 : Marcus' comments, and "cld_dynamic"->"cld2_dynamic" #Patch Set 23 : Guard static initialization with #ifdef #
Total comments: 3
Patch Set 24 : Consistently guard #includes with #ifdef #Patch Set 25 : Final rebase #Messages
Total messages: 56 (0 generated)
https://codereview.chromium.org/187393005/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/187393005/diff/40001/build/common.gypi#newcod... build/common.gypi:633: 'cld_dynamic%': 1, Make sure to disable dynamic mode on all platforms prior to checkin. We want to start enabling this platform-by-platform as we get the packaging of the CLD data file done, but we don't need to wait on file packaging to check the functionality in behind disabled compiler flags.
Hi folks, This patch is almost ready to land, and is now ready for some eyes on it. Whilst the review process gets underway I'll work on the CLD2 roll and figuring out how to package the CLD2 data file with the application binary. Thanks!
thanks andrew! a few initial comments below. I also added palmer@, better get the security aspects validated sooner. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:147: static base::PlatformFile cached_platform_file = 0; nit: avoid using function-scoped static, make it either a class-level or in a namespace. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:155: NOTREACHED(); nit: unindent. also, in some other places in this file doesn't use {} for single-line statement, probably best to keep consistency. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:165: if (!base::PathExists(path)) { ditto for {} https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:170: int flags = base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ; nit: probably simpler to just pass this inline rather than keeping a local? https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:173: cached_platform_file = base::CreatePlatformFile( I think this is arriving at the IO thread, we should open the file in the FILE thread. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:187: IPC::PlatformFileForTransit ipcPlatformFile = IPC::GetFileHandleForProcess( nit: s/ipcPlatformFile/ipc_platform_file/ https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:192: Send(new ChromeViewMsg_CLDDataAvailable(routing_id, ipcPlatformFile)); I suppose if this is in the IO thread, we wouldn't want to send the reply from within the same message. depending on how 173 is solved, it may be ok though (if this function ends up being executed in the FILE thread). https://codereview.chromium.org/187393005/diff/40001/chrome/common/render_mes... File chrome/common/render_messages.h (right): https://codereview.chromium.org/187393005/diff/40001/chrome/common/render_mes... chrome/common/render_messages.h:413: // Informs the renderer process that Compact Language Detector (CLD) data is probably best to keep close together with other translate messages? also, the name could imply that, something like: ChromeViewMsg_TranslateCLDDataAvailable, or something.. https://codereview.chromium.org/187393005/diff/40001/chrome/common/render_mes... chrome/common/render_messages.h:782: // Informs the browser process that Compact Language Detector (CLD) data is ditto https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:99: static bool startedTranslationPolling = false; ditto for function-scoped static. also, hacker_style https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:175: void SendCLD2DataFileRequest(int delayMillis, int nextDelayMillis); nit: s/camelCase/hacker_style/ https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:182: #endif nit: add a \n would it make sense to split this into its own class?
https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:166: void DeferPageCaptured(const int page_id, const base::string16& contents); Needs a comment explaining what this does.
FWIW here is the review for the CLD2 roll, which is a prerequisite for landing this patch: https://codereview.chromium.org/197123002/
+jochen for changes to constants
constant changes lgtm also some nits https://codereview.chromium.org/187393005/diff/60001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/187393005/diff/60001/build/common.gypi#newcod... build/common.gypi:622: # 'cld_version%': 1, what's this comment supposed to tell me? https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:155: NOTREACHED(); i don't think this if () { NOTREACHED(); } adds much value here https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:166: return; no { } https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:175: DCHECK(created == false); !created https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:178: DCHECK(false) << "Failed to open CLD data file, error code=" << error; is it really necessary to crash here?
Addressing Marcus' comments, and will upload a new rev shortly. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:147: static base::PlatformFile cached_platform_file = 0; On 2014/03/11 18:56:10, bulach wrote: > nit: avoid using function-scoped static, make it either a class-level or in a > namespace. Done. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:155: NOTREACHED(); On 2014/03/11 18:56:10, bulach wrote: > nit: unindent. > also, in some other places in this file doesn't use {} for single-line > statement, probably best to keep consistency. Done. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:165: if (!base::PathExists(path)) { On 2014/03/11 18:56:10, bulach wrote: > ditto for {} Done. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:170: int flags = base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ; On 2014/03/11 18:56:10, bulach wrote: > nit: probably simpler to just pass this inline rather than keeping a local? Done. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:173: cached_platform_file = base::CreatePlatformFile( On 2014/03/11 18:56:10, bulach wrote: > I think this is arriving at the IO thread, we should open the file in the FILE > thread. I have moved this to the blocking pool. It adds a bit of complexity, but it seems to work fine! Thanks for the suggestion. I added some comments to make it clear what is going on, as we do need to bring in locks at this point. I believe the resulting code is as non-blocking as it can be, and should perform well. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:187: IPC::PlatformFileForTransit ipcPlatformFile = IPC::GetFileHandleForProcess( On 2014/03/11 18:56:10, bulach wrote: > nit: s/ipcPlatformFile/ipc_platform_file/ Done. https://codereview.chromium.org/187393005/diff/40001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:192: Send(new ChromeViewMsg_CLDDataAvailable(routing_id, ipcPlatformFile)); On 2014/03/11 18:56:10, bulach wrote: > I suppose if this is in the IO thread, we wouldn't want to send the reply from > within the same message. > depending on how 173 is solved, it may be ok though (if this function ends up > being executed in the FILE thread). We still respond from the IO thread, but I see no danger here. We're talking to a different process and we're not sending messages synchronously. I'm leaving this as-is unless there's a strong reason to object. I've added a comment noting this for maintainers. https://codereview.chromium.org/187393005/diff/40001/chrome/common/render_mes... File chrome/common/render_messages.h (right): https://codereview.chromium.org/187393005/diff/40001/chrome/common/render_mes... chrome/common/render_messages.h:413: // Informs the renderer process that Compact Language Detector (CLD) data is On 2014/03/11 18:56:10, bulach wrote: > probably best to keep close together with other translate messages? > also, the name could imply that, something like: > ChromeViewMsg_TranslateCLDDataAvailable, or something.. Proximity: The ChromeViewMsg and ChromeViewHostMsg chunks seem to be divided intentionally, which is why I've got them separated. Am I incorrect? I prefer not to include the word Translate. Yes, it's true that Translate is currently the only consumer of CLD data, but CLD is independent of Translate and other features could conceivably want CLD capabilities in the future (admittedly unlikely). If you feel strongly, let me know. https://codereview.chromium.org/187393005/diff/40001/chrome/common/render_mes... chrome/common/render_messages.h:782: // Informs the browser process that Compact Language Detector (CLD) data is On 2014/03/11 18:56:10, bulach wrote: > ditto See other response. https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:99: static bool startedTranslationPolling = false; On 2014/03/11 18:56:10, bulach wrote: > ditto for function-scoped static. also, hacker_style Done. https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:166: void DeferPageCaptured(const int page_id, const base::string16& contents); On 2014/03/12 09:15:48, Andrew Hayden wrote: > Needs a comment explaining what this does. Done. https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:175: void SendCLD2DataFileRequest(int delayMillis, int nextDelayMillis); On 2014/03/11 18:56:10, bulach wrote: > nit: s/camelCase/hacker_style/ Done. https://codereview.chromium.org/187393005/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:182: #endif On 2014/03/11 18:56:10, bulach wrote: > nit: add a \n > > would it make sense to split this into its own class? Added the \n. I considered making this its own class, but TBH I think it makes more sense to keep this stuff all together. Especially given the linkage between PageCaptured, DeferPageCaptured, and OnCLDDataAvailable. Let me know if you disagree. Added comments to make this connection clear.
Addressing jochen@'s comments. https://codereview.chromium.org/187393005/diff/60001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/187393005/diff/60001/build/common.gypi#newcod... build/common.gypi:622: # 'cld_version%': 1, On 2014/03/12 13:08:27, jochen wrote: > what's this comment supposed to tell me? I'll take this out. It's just there for me to conveniently switch things on and off while I generate size estimates and answer questions people have been asking :) https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:155: NOTREACHED(); On 2014/03/12 13:08:27, jochen wrote: > i don't think this if () { NOTREACHED(); } adds much value here Done. https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:166: return; On 2014/03/12 13:08:27, jochen wrote: > no { } Done. https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:175: DCHECK(created == false); On 2014/03/12 13:08:27, jochen wrote: > !created Done. https://codereview.chromium.org/187393005/diff/60001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:178: DCHECK(false) << "Failed to open CLD data file, error code=" << error; On 2014/03/12 13:08:27, jochen wrote: > is it really necessary to crash here? No, it isn't. I just put a comment in the code instead. Thanks.
I have addressed all the issues raised and modified the commit comment quite heavily to reflect the current state of affairs. This last patchset makes sure to DISABLE dynamic mode on ALL platforms for now, since I want to make the actual "switching on" a simple commit that can be trivially reverted if necessary. PTAL. I need some folks from translate specifically, since they are the owners.
I should note that the DEPS change is present here because I'm rebased on top of an old version of https://codereview.chromium.org/197123002. I will rebase onto the latest master once the roll to r155 has been completed, and the DEPS change will then go away.
https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:157: if (!s_file_lock_.Try()) return; // We'll get another request later, whatevs! Nit: Style for C++ is to put "return;" on its own line. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:196: base::AutoLock lock(s_file_lock_); // will release when we return This comment is superfluous. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:197: if (s_cached_platform_file_ != 0) return; // already done, duplicate request Style: "return;" on its own line. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:201: return; // Chrome isn't properly installed. DCHECK? CHECK or Warn the user in release builds? https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:205: // right now), then we will do nothing. It is up to the renderer process Rather than repeat the comments to this effect, I would just document this "caller must retry" protocol in the .h file once. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:209: return; // File doesn't exist This comment is superfluous. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:221: return; // File exists, but we can't open it. This comment is superfluous. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:225: lock.~AutoLock(); Not a problem, I don't think, but it "feels" a little odd to explicitly call this. I wonder if it perhaps indicates a "bump" in the API. https://codereview.chromium.org/187393005/diff/80001/chrome/common/render_mes... File chrome/common/render_messages.h (right): https://codereview.chromium.org/187393005/diff/80001/chrome/common/render_mes... chrome/common/render_messages.h:418: IPC::PlatformFileForTransit) Nit: Style is to put the variable name in a /* comment */ afterward. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:85: deferred_page_id_ = -1; Put these up above in the : initialization list. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:99: if (cld2_data_file_polling_started) return; Style nit. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:115: const base::string16& contents) { Nit: indentation. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:570: if (cld2_data_file_polling_canceled) return; Style (everywhere this occurs). https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:158: #if defined(CLD2_DYNAMIC_MODE) The IPC declaration and handlers should also be behind this #ifdef.
https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:62: // finished.cld2_data_file = NULL; paste it mistakenly? https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:98: #if defined(CLD2_DYNAMIC_MODE) you may want to reset deferred_* here? https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:104: if (url.SchemeIs(extensions::kExtensionScheme)) return; TranslateManager::IsTranslatableURL() is the function exactly you may want to use here. But the function is for browser process. Can we move the function to components/translate/core/common/translate_util.h|cc, and use it from both TranslateManager and TranslateHelper? WDYT? I guess this check do not have to be so strict. So, this current check may work for you. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:115: const base::string16& contents) { wrong indent https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:119: deferred_contents_.append(contents); Just a question. Why don't you simply assign it, but call clean() and append()? https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:138: // TODO(andrewhayden): UMA insertion point here: Track if data is available. Just a question: How long time does it take from calling SendCLD2DataFileRequest() to file mmap being completed. Do you have any data? IIUC, it affects only against the first page loading, so the latency does not matter. But just I'm interested in. If you do not have any data, please ignore this comment.
toyoshim@: Thanks for finding IsTranslatableURL! That's exactly what I want, you're right. I see no reason why that couldn't move into common, as long as all the constants it depends upon are already in common as well. I know this is true in the case of the extensions prefix, and I'll check the others. The impact of this is very minor but it will be nice to keep it clean.
Addressed Palmer's comments. Next up toyoshim@'s comments. Thanks for the attention to detail, guys. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:157: if (!s_file_lock_.Try()) return; // We'll get another request later, whatevs! On 2014/03/13 17:59:50, Chromium Palmer wrote: > Nit: Style for C++ is to put "return;" on its own line. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:196: base::AutoLock lock(s_file_lock_); // will release when we return On 2014/03/13 17:59:50, Chromium Palmer wrote: > This comment is superfluous. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:197: if (s_cached_platform_file_ != 0) return; // already done, duplicate request On 2014/03/13 17:59:50, Chromium Palmer wrote: > Style: "return;" on its own line. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:201: return; // Chrome isn't properly installed. On 2014/03/13 17:59:50, Chromium Palmer wrote: > DCHECK? CHECK or Warn the user in release builds? Done. I took the DCHECK out because of other comments, but a WARN is appropriate. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:205: // right now), then we will do nothing. It is up to the renderer process On 2014/03/13 17:59:50, Chromium Palmer wrote: > Rather than repeat the comments to this effect, I would just document this > "caller must retry" protocol in the .h file once. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:209: return; // File doesn't exist On 2014/03/13 17:59:50, Chromium Palmer wrote: > This comment is superfluous. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:221: return; // File exists, but we can't open it. On 2014/03/13 17:59:50, Chromium Palmer wrote: > This comment is superfluous. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/browser/translate... chrome/browser/translate/translate_tab_helper.cc:225: lock.~AutoLock(); On 2014/03/13 17:59:50, Chromium Palmer wrote: > Not a problem, I don't think, but it "feels" a little odd to explicitly call > this. I wonder if it perhaps indicates a "bump" in the API. I wanted to release the lock as quickly as possible, rather than holding it while we send the response. Since the code on the other side uses a non-blocking Try(), it's not a big deal; if there is a strong desire to avoid the destructor manual invocation, that's cool. I'll add a comment here noting this so its clear, let me know if you want it changed. https://codereview.chromium.org/187393005/diff/80001/chrome/common/render_mes... File chrome/common/render_messages.h (right): https://codereview.chromium.org/187393005/diff/80001/chrome/common/render_mes... chrome/common/render_messages.h:418: IPC::PlatformFileForTransit) On 2014/03/13 17:59:50, Chromium Palmer wrote: > Nit: Style is to put the variable name in a /* comment */ afterward. Done. Sorry, I should have noticed that in this file. Thanks for pointing it out. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:85: deferred_page_id_ = -1; On 2014/03/13 17:59:50, Chromium Palmer wrote: > Put these up above in the : initialization list. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:99: if (cld2_data_file_polling_started) return; On 2014/03/13 17:59:50, Chromium Palmer wrote: > Style nit. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:115: const base::string16& contents) { On 2014/03/13 17:59:50, Chromium Palmer wrote: > Nit: indentation. Done. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:570: if (cld2_data_file_polling_canceled) return; On 2014/03/13 17:59:50, Chromium Palmer wrote: > Style (everywhere this occurs). Done. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:158: #if defined(CLD2_DYNAMIC_MODE) On 2014/03/13 17:59:50, Chromium Palmer wrote: > The IPC declaration and handlers should also be behind this #ifdef. Done.
Everything from toyoshim@'s comments EXCEPT using isTranslatableURL. I'll get that momentarily. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:62: // finished.cld2_data_file = NULL; On 2014/03/13 18:37:37, Takashi Toyoshima (chromium) wrote: > paste it mistakenly? Done. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:98: #if defined(CLD2_DYNAMIC_MODE) On 2014/03/13 18:37:37, Takashi Toyoshima (chromium) wrote: > you may want to reset deferred_* here? Hmm, good thought. It's SAFE to leave it alone, because we will check the page id before we resurrect the language detection request, but IIUC we could end up showing the translate bar after the user has begun navigation to another page but before that page has actually loaded. So yeah... we should do the reset for maximum sanity. Good catch. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:104: if (url.SchemeIs(extensions::kExtensionScheme)) return; On 2014/03/13 18:37:37, Takashi Toyoshima (chromium) wrote: > TranslateManager::IsTranslatableURL() is the function exactly you may want to > use here. But the function is for browser process. Can we move the function to > components/translate/core/common/translate_util.h|cc, and use it from both > TranslateManager and TranslateHelper? > > WDYT? I guess this check do not have to be so strict. So, this current check may > work for you. For clarity I'll submit a separate patchset in this review with just this change, so holding off for a few minutes. https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:119: deferred_contents_.append(contents); On 2014/03/13 18:37:37, Takashi Toyoshima (chromium) wrote: > Just a question. > Why don't you simply assign it, but call clean() and append()? I'll assign it instead. I was concerned about the difference between operator= and assign, but after reading more about std::string (the underlying construct for string16) it seems this should do the right thing. I wanted to make sure I wasn't going to be left with a string pointing to the memory in the original contents reference, but it looks like operator= will definitely copy. Sorry, my C++ is a bit rusty :) https://codereview.chromium.org/187393005/diff/80001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:138: // TODO(andrewhayden): UMA insertion point here: Track if data is available. On 2014/03/13 18:37:37, Takashi Toyoshima (chromium) wrote: > Just a question: > How long time does it take from calling SendCLD2DataFileRequest() to file mmap > being completed. Do you have any data? IIUC, it affects only against the first > page loading, so the latency does not matter. But just I'm interested in. > If you do not have any data, please ignore this comment. I don't have data but I'd be happy to measure it :)
toyoshim@: Can you explain why in IsTranslatableURL we only exclude extensions if CHROMEOS is defined? Do we really offer translation service to extension pages on non-ChromeOS, and if so why? I'm loathe to change the logic in that function as part of this change (in case it causes a regression); but as it stands right now my code explicitly ignores extension pages, which seems correct to me. Can I move that check outside the ifdef?
So, I looked into the IsTranslatableURL work. It will require not just moving the function but also moving code in multiple unit tests and changing #includes in several places. I'm not comfortable doing that work as part of this change, so I'll have to do it in a separate CL. I'll get an issue filed and a CL posted shortly.
Filed https://code.google.com/p/chromium/issues/detail?id=352578 to track the refactor of IsTranslatableURL, which can be independent of this change.
> toyoshim@: Can you explain why in IsTranslatableURL we only exclude extensions > if CHROMEOS is defined? Do we really offer translation service to extension > pages on non-ChromeOS, and if so why? That check excluded only Files App which is default file manager on Chrome OS. Before Apps v2, Chrome Translate supports extensions. But, now Apps v2 does not have inforbar service, so Translate will not work. It is reasonable to change it to exclude all extension schemes in all platform now.
Ah, if you want to discuss IsTranslatableURL() in separate CL, this change LGTM on translate.
Great! Thanks to everyone for the time and attention on this. I've submitted the CLD2 roll to the CQ, and will rebase this change onto the new CLD2 when it lands. Then I'll get this one rolling while I work on the patch for consolidating IsTranslatableURL, and then I'll do one final patch to change the behavior of IsTranslatableURL to exclude all extensions as discussed with toyoshim@. Whew! Thanks.
I note that Palmer didn't LGTM, so I'll also wait for a review from security before submitting.
https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:39: base::PlatformFile TranslateTabHelper::s_cached_platform_file_ = 0; nit: NULL is probably cleaerer https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:164: if (!s_file_lock_.Try()) nit: use base::AutoLock, is a more common pattern. https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:169: if (handle == 0) { nit: NULL https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:176: weak_pointer_factory_.GetWeakPtr())); hmm... WeakPtr can't be dereferenced across threads. :( it looks like HandleCLDDataRequest is only dealing with static, right? in that case, make that function static so that this closure won't need any param? https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:204: base::AutoLock lock(s_file_lock_); locks are expensive, make them scoped: { base::AutoLock auto_lock(s_file_lock_); if (s_cached_platform_file_ != 0) return; } ... .. .. { base::AutoLock auto_lock(s_file_lock_); if (s_cached_platform_file_ == 0) s_cached_platform_file_ = base::CreatePlatformFile(....); } https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:235: lock.~AutoLock(); // Release the lock now rather than after responding as above, create smaller scopes instead.. https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.h (right): https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.h:126: static base::Lock s_file_lock_; statics can only be used for PODs, this need to be a "LazyInstance"..
https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:176: weak_pointer_factory_.GetWeakPtr())); On 2014/03/14 15:30:58, bulach wrote: > hmm... WeakPtr can't be dereferenced across threads. :( > it looks like HandleCLDDataRequest is only dealing with static, right? > in that case, make that function static so that this closure won't need any > param? btw, I forgot to mention "Post(BlockingPool)TaskAndReply". the idea being: - blocking pool will be just a static method, not dealing with this object at all. - the reply is a callback with WeakPtr: it'll be executed back in the caller thread's task runner so it won't have any problems since it won't be dereferenced across different threads.
Partial answer to Marcus' comments about the locking. Regarding the weakptr stuff, I'll reconsider based on your most recent comment about using a different mechanism for making the call to the blocking pool. https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:164: if (!s_file_lock_.Try()) On 2014/03/14 15:30:58, bulach wrote: > nit: use base::AutoLock, is a more common pattern. I explicitly chose Lock.try() instead. AutoLock will call acquire(). I don't want this thread to block in the event that the file is currently being opened; it's not worth it. The renderer will call again later. Make sense? https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:176: weak_pointer_factory_.GetWeakPtr())); On 2014/03/14 15:30:58, bulach wrote: > hmm... WeakPtr can't be dereferenced across threads. :( > it looks like HandleCLDDataRequest is only dealing with static, right? > in that case, make that function static so that this closure won't need any > param? But I was under the impression that the weakptr was necessary to provide the object reference upon which the target method is invoked. Guess I need to do some more reading. But yeah, static all the way, so I guess? The method itself could presumably be static, too. https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:204: base::AutoLock lock(s_file_lock_); On 2014/03/14 15:30:58, bulach wrote: > locks are expensive, make them scoped: > { > base::AutoLock auto_lock(s_file_lock_); > if (s_cached_platform_file_ != 0) > return; > } > ... > .. > .. > { > base::AutoLock auto_lock(s_file_lock_); > if (s_cached_platform_file_ == 0) > s_cached_platform_file_ = base::CreatePlatformFile(....); > } The only other code that depends upon this lock is the Try() call in OnCLDDataRequested, which should make this into a non-issue. It's simpler to keep it scoped the way it is, and it's ok if it ends up running long because nothing else will attempt to Acquire() the lock. https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:235: lock.~AutoLock(); // Release the lock now rather than after responding On 2014/03/14 15:30:58, bulach wrote: > as above, create smaller scopes instead.. See reply above.
https://codereview.chromium.org/187393005/diff/190001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.h (right): https://codereview.chromium.org/187393005/diff/190001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.h:132: static base::Lock s_file_lock_; JFYI, this static member would generate a static initializer. You would have here to use a LazyInstance for your global state in translate_tab_helper.cc :) The LazyInstance will also give you thread-safe initialization which allows you to get rid of the mutex. You can have the file descriptor const and initialize it once as in: struct DataFile { DataFile() : fd(OpenFile()) {} const int fd; private: static int OpenFile() { int fd = open(...); // ... return fd; } }; base::LazyInstance<DataFile>::Leaky g_file = LAZY_INSTANCE_INITIALIZER; I haven't looked at the full picture though so please ignore this comment if it doesn't make sense :)
Marcus's remaining comments from previous patchsets around lazy pointer. https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:39: base::PlatformFile TranslateTabHelper::s_cached_platform_file_ = 0; On 2014/03/14 15:30:58, bulach wrote: > nit: NULL is probably cleaerer The compiler disagrees with you. I shall side with the compiler :P ../../chrome/browser/translate/translate_tab_helper.cc:40:66: error: converting to non-pointer type 'int' from NULL [-Werror=conversion-null] https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:169: if (handle == 0) { On 2014/03/14 15:30:58, bulach wrote: > nit: NULL See previous comment, re: compiler. https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.h (right): https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.h:126: static base::Lock s_file_lock_; On 2014/03/14 15:30:58, bulach wrote: > statics can only be used for PODs, this need to be a "LazyInstance".. Done.
On 2014/03/19 16:42:34, Philippe wrote: > https://codereview.chromium.org/187393005/diff/190001/chrome/browser/translat... > File chrome/browser/translate/translate_tab_helper.h (right): > > https://codereview.chromium.org/187393005/diff/190001/chrome/browser/translat... > chrome/browser/translate/translate_tab_helper.h:132: static base::Lock > s_file_lock_; > JFYI, this static member would generate a static initializer. You would have > here to use a LazyInstance for your global state in translate_tab_helper.cc :) > > The LazyInstance will also give you thread-safe initialization which allows you > to get rid of the mutex. You can have the file descriptor const and initialize > it once as in: > > struct DataFile { > DataFile() : fd(OpenFile()) {} > > const int fd; > > private: > static int OpenFile() { > int fd = open(...); > // ... > return fd; > } > }; > > base::LazyInstance<DataFile>::Leaky g_file = LAZY_INSTANCE_INITIALIZER; > > I haven't looked at the full picture though so please ignore this comment if it > doesn't make sense :) Thanks for the suggestion, Philippe. Unfortunately I don't think this will work, as we have no guarantee that the file will in fact be available when we get called. That's part of the problem, it could be instantly available or it could be something that gets downloaded in the background, we don't know. The constructor might have to abort, and that would presumably break this approach. Otherwise, yes :)
On 2014/03/19 11:18:50, bulach wrote: > https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... > File chrome/browser/translate/translate_tab_helper.cc (right): > > https://codereview.chromium.org/187393005/diff/150001/chrome/browser/translat... > chrome/browser/translate/translate_tab_helper.cc:176: > weak_pointer_factory_.GetWeakPtr())); > On 2014/03/14 15:30:58, bulach wrote: > > hmm... WeakPtr can't be dereferenced across threads. :( > > it looks like HandleCLDDataRequest is only dealing with static, right? > > in that case, make that function static so that this closure won't need any > > param? > > btw, I forgot to mention "Post(BlockingPool)TaskAndReply". > the idea being: > - blocking pool will be just a static method, not dealing with this object at > all. > - the reply is a callback with WeakPtr: it'll be executed back in the caller > thread's task runner so it won't have any problems since it won't be > dereferenced across different threads. Yikes. OK, I'll do that. WeakPtr... (╯°□°）╯︵ ┻━┻
Threadsafety issues proactively addressed. Thanks, Marcus. OK, as far as I'm concerned at this point all outstanding issues are resolved. PTAL.
Important note: This patch will work on Linux desktops, but it will crash when cld_dynamic is turned on for Android due to https://code.google.com/p/cld2/issues/detail?id=11. We'll need yet another CLD2 roll before we actually turn this on in Android.
https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:212: handle, Pls also allow offset+length for flexibility. Perhaps the cld2 data is part of a larger "downloaded data" blob.
thanks andrew! I'm not an owner in any of these files, but a few more nits and suggestions below. I think we're real close, but there should probably be some more owners reviewing this... https://codereview.chromium.org/187393005/diff/280001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/187393005/diff/280001/build/common.gypi#newco... build/common.gypi:428: # Only evaluated if cld_version == 2 or if building the CLD2 dynamic data tool explicitly. nit: there's no strict limit, but from 414-418, try to keep <80cols https://codereview.chromium.org/187393005/diff/280001/build/common.gypi#newco... build/common.gypi:437: # 1: dynamic, language scoring tables live in a data file that must be loaded at runtime nit: <80cols https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:41: base::PlatformFile TranslateTabHelper::s_cached_platform_file_ = 0; nit: sorry, I just found that it should be kInvalidPlatformFileValue rather than my previous NULL suggestion :) https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:177: if (handle == 0) { nit: s/0/kInvalidPlatformFileValue/ I've seen the following pattern in some other places (i.e., early return and no else): if (handle != kInvalidPlatformFileValue) { SendCLDDataAvailable(handle); return; } // No data file yet, queue an asynchronous caching attempt. // ... https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:178: // We don't have the data file yet. Queue an asynchronous caching attempt. nit: I've got reviewers asking to not have pronouns, like my suggestion above.. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:196: if (!s_file_lock_.Get().Try()) I don't quite follow why it's just trying rather than actually acquiring the lock.. the AutoLock seems a fairly common pattern, so if this has extra requirements preventing it from using, it should probably be documented? https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:205: void TranslateTabHelper::SendCLDDataAvailable(const base::PlatformFile handle) { nit: can this be a const &? https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:207: // blocking pool. Do not use the synchronous invocation style. not clear, synchronous invocation style of what? https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:226: base::AutoLock lock(s_file_lock_.Get()); nit: use a separate block here: { base::AutoLock lock(s_file_lock_.get())); .... } https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:245: s_cached_platform_file_ = base::CreatePlatformFile( then here, use a local variable. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:254: } so finally, only here use the lock again to actually set the platform_file. if (platform_file) { base::AutoLock auto_lock(...); // Another blocking pool request got here first, close the file. if (s_cached_platform_file_) { ClosePlatformFile(platform_file); return; } // Set the global file. s_cached_platform_file_ = platform_file; } https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:87: deferred_contents_(ASCIIToUTF16("")) nit: I think string16 default initializes to empty. also, should be indented to match 78-81 https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:109: // other function calls as wlel, so for now we replicate the logic here. nit: s/wlel/well/ https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:111: return; nit: here and all "return" clauses below, unindent https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:616: if (deferred_page_capture_) { nit: remove one level: if (deferred_page_capture_ && CLD2::isDataLoaded()) { ... } https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:647: static base::MemoryMappedFile* mmap = NULL; no function-scoped statics... what's the lifetime of this class? could it be a regular scoped_ptr member of this class itself? https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:653: base::File basicFile(platformFile); nit: s/basicFile/basic_file/ https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:657: bool loadedOk = mmap->Initialize(basicFile.Pass()); nit: s/loadedOk/loaded_ok/ perhaps "initialized" would be clearer? https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:166: bool cld2_data_file_polling_canceled; nit: this and the one above, append a "_"
All of Marcus' comments addressed. Marcus, PTYAL. https://codereview.chromium.org/187393005/diff/280001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/187393005/diff/280001/build/common.gypi#newco... build/common.gypi:428: # Only evaluated if cld_version == 2 or if building the CLD2 dynamic data tool explicitly. On 2014/03/23 20:57:02, bulach wrote: > nit: there's no strict limit, but from 414-418, try to keep <80cols Done. https://codereview.chromium.org/187393005/diff/280001/build/common.gypi#newco... build/common.gypi:437: # 1: dynamic, language scoring tables live in a data file that must be loaded at runtime On 2014/03/23 20:57:02, bulach wrote: > nit: <80cols Done. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:41: base::PlatformFile TranslateTabHelper::s_cached_platform_file_ = 0; On 2014/03/23 20:57:02, bulach wrote: > nit: sorry, I just found that it should be kInvalidPlatformFileValue rather than > my previous NULL suggestion :) Done. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:177: if (handle == 0) { On 2014/03/23 20:57:02, bulach wrote: > nit: s/0/kInvalidPlatformFileValue/ > > I've seen the following pattern in some other places (i.e., early return and no > else): > > if (handle != kInvalidPlatformFileValue) { > SendCLDDataAvailable(handle); > return; > } > // No data file yet, queue an asynchronous caching attempt. > // ... Done. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:178: // We don't have the data file yet. Queue an asynchronous caching attempt. On 2014/03/23 20:57:02, bulach wrote: > nit: I've got reviewers asking to not have pronouns, like my suggestion above.. Done, and reviewed the other .h/.cc files as well to try and clean up the grammar. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:196: if (!s_file_lock_.Get().Try()) On 2014/03/23 20:57:02, bulach wrote: > I don't quite follow why it's just trying rather than actually acquiring the > lock.. the AutoLock seems a fairly common pattern, so if this has extra > requirements preventing it from using, it should probably be documented? As discussed offline, because in terms of performance the "try" with the widely scoped blocking in the blocking pool mitigates performance concerns. We decided offline to more narrowly scope the lock in the blocking pool, at which point there should be no reason to need the try. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:205: void TranslateTabHelper::SendCLDDataAvailable(const base::PlatformFile handle) { On 2014/03/23 20:57:02, bulach wrote: > nit: can this be a const &? It's a file descriptor (integer) under the covers. Is there a strong reason to prefer a reference? It seems simpler to me to just leave it as-is. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:207: // blocking pool. Do not use the synchronous invocation style. On 2014/03/23 20:57:02, bulach wrote: > not clear, synchronous invocation style of what? Cleared up, I hope. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:212: handle, On 2014/03/21 21:08:32, ulas wrote: > Pls also allow offset+length for flexibility. Perhaps the cld2 data is part of a > larger "downloaded data" blob. Will do this shortly, after I address Marcus' larger-scope comments. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:226: base::AutoLock lock(s_file_lock_.Get()); On 2014/03/23 20:57:02, bulach wrote: > nit: use a separate block here: > { > base::AutoLock lock(s_file_lock_.get())); > .... > } > Done. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:245: s_cached_platform_file_ = base::CreatePlatformFile( On 2014/03/23 20:57:02, bulach wrote: > then here, use a local variable. Done. https://codereview.chromium.org/187393005/diff/280001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:254: } On 2014/03/23 20:57:02, bulach wrote: > so finally, only here use the lock again to actually set the platform_file. > > if (platform_file) { > base::AutoLock auto_lock(...); > // Another blocking pool request got here first, close the file. > if (s_cached_platform_file_) { > ClosePlatformFile(platform_file); > return; > } > // Set the global file. > s_cached_platform_file_ = platform_file; > } Done. https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:87: deferred_contents_(ASCIIToUTF16("")) On 2014/03/23 20:57:02, bulach wrote: > nit: I think string16 default initializes to empty. > also, should be indented to match 78-81 Done. https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:109: // other function calls as wlel, so for now we replicate the logic here. On 2014/03/23 20:57:02, bulach wrote: > nit: s/wlel/well/ Done. https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:111: return; On 2014/03/23 20:57:02, bulach wrote: > nit: here and all "return" clauses below, unindent Done. https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:616: if (deferred_page_capture_) { On 2014/03/23 20:57:02, bulach wrote: > nit: remove one level: > > if (deferred_page_capture_ && CLD2::isDataLoaded()) { > ... > } Done. https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:647: static base::MemoryMappedFile* mmap = NULL; On 2014/03/23 20:57:02, bulach wrote: > no function-scoped statics... what's the lifetime of this class? could it be a > regular scoped_ptr member of this class itself? Yes. I'll make that change. https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:653: base::File basicFile(platformFile); On 2014/03/23 20:57:02, bulach wrote: > nit: s/basicFile/basic_file/ Done. https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:657: bool loadedOk = mmap->Initialize(basicFile.Pass()); On 2014/03/23 20:57:02, bulach wrote: > nit: s/loadedOk/loaded_ok/ > perhaps "initialized" would be clearer? Done. https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:166: bool cld2_data_file_polling_canceled; On 2014/03/23 20:57:02, bulach wrote: > nit: this and the one above, append a "_" Done.
Compiled this all up and verified that it continues to work as expected on Android and Linux.
IPC security LGTM but some concerns remain. Why the src/internal? https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:200: // Else, we don't have the data file yet. Queue a caching 0attempt. is "0attempt" a typo? https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:272: DCHECK(!created); I wonder if this should be a cause for LOG and return, rather than just a DCHECK. https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:281: s_file_lock_.Get().Release(); This, and line 288, make me think you want an Autolock. https://codereview.chromium.org/187393005/diff/320001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/187393005/diff/320001/chrome/common/render_me... chrome/common/render_messages.h:798: // should go unanswered (the renderer will ask again later) Nit: Puncutation. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:589: // Terminate immediately if told to stop polling Nit: Punctuate. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:601: // chain of polling that will last until the pointer stops being null, Nit: NULL https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:630: deferred_page_id_ = -1; // Clean up for sanity Nit: punctuation, but really these comments are superfluous. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:637: // Terminate immediately if told to stop polling Nit: Punctuate. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:648: DCHECK(platform_file != base::kInvalidPlatformFileValue) This should be a production, run-time check, not a DCHECK. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:182: // The mmap. The mmap dstructor will unmap the memory segment and close the Nit: typo "destructor". https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:212: #endif Nit: Should be a blank line after this. https://codereview.chromium.org/187393005/diff/320001/third_party/cld_2/cld_2... File third_party/cld_2/cld_2.gyp (right): https://codereview.chromium.org/187393005/diff/320001/third_party/cld_2/cld_2... third_party/cld_2/cld_2.gyp:27: 'src/internal/cld2_dynamic_data.h', How is this code getting tested? Is there a fuzzer? Et c.
lgtm but I'm not in the OWNERS, thanks andrew! mostly nits and some more suggestions below. https://codereview.chromium.org/187393005/diff/280001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/187393005/diff/280001/build/common.gypi#newco... build/common.gypi:428: # Only evaluated if cld_version == 2 or if building the CLD2 dynamic data tool explicitly. On 2014/03/24 15:18:24, Andrew Hayden wrote: > On 2014/03/23 20:57:02, bulach wrote: > > nit: there's no strict limit, but from 414-418, try to keep <80cols > > Done. sorry to keep pinging here, but it still looks >80 cols (and 437 too) https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/280001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:87: deferred_contents_(ASCIIToUTF16("")) On 2014/03/24 15:18:24, Andrew Hayden wrote: > On 2014/03/23 20:57:02, bulach wrote: > > nit: I think string16 default initializes to empty. > > also, should be indented to match 78-81 > > Done. sorry, how about the string16 initialization, is it needed? https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:42: = base::kInvalidPlatformFileValue; nit: move the = to the previous line, and indent by 4 since its a continuation (same on 44 below). https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:190: s_file_lock_.Get().Acquire(); AutoLock in its own scope? https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:214: if (!s_file_lock_.Get().Try()) since the scope for the locks are much narrower now, using autolocks throughout makes it simpler to follow.. https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:219: return; // Nope, not ready yet in this case, the early return is not buying much. could do: if (handle != base::kInvalidPlatformFileValue) SendCLDDataAvailable(handle); https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:281: s_file_lock_.Get().Release(); On 2014/03/24 19:01:47, Chromium Palmer wrote: > This, and line 288, make me think you want an Autolock. yep, something like: { AutoLock(...); if (s_cached_platform_file_ != base::kInvalidPlatformFileValue) s_cached_platform_file_ = file; } if (file != s_cached_platform_file_) { // Idempotence: racing another request on the blocking pool. base::ClosePlatformFile(file); } https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:124: && url.DomainIs(file_manager::kFileManagerAppId)) nit: && in the previous line https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:183: // file handle; it must be held throughought the lifetime of the process. I think valgrind and friends will complain about this object being leaked. LazyInstance::Leaky is the pattern used for this kind of objects, it annotates correctly so that the leak detection tools won't flag them. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:184: static base::MemoryMappedFile* s_cld2_data_file_mmap_; nit: also, please move the fields (164-184) after the methods.
Answering Palmer's and Marcus' comments. Outstanding issues: 1. Do we need a fuzzer for CLD2? 2. LazyInstance:Leaky pointer for mmap to avoid getting flagged as leaking memory https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... File chrome/browser/translate/translate_tab_helper.cc (right): https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:42: = base::kInvalidPlatformFileValue; On 2014/03/25 09:29:26, bulach wrote: > nit: move the = to the previous line, and indent by 4 since its a continuation > (same on 44 below). Done. https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:190: s_file_lock_.Get().Acquire(); On 2014/03/25 09:29:26, bulach wrote: > AutoLock in its own scope? Done. https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:200: // Else, we don't have the data file yet. Queue a caching 0attempt. On 2014/03/24 19:01:47, Chromium Palmer wrote: > is "0attempt" a typo? yes, fixed. https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:214: if (!s_file_lock_.Get().Try()) On 2014/03/25 09:29:26, bulach wrote: > since the scope for the locks are much narrower now, using autolocks throughout > makes it simpler to follow.. Done. https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:219: return; // Nope, not ready yet On 2014/03/25 09:29:26, bulach wrote: > in this case, the early return is not buying much. could do: > > if (handle != base::kInvalidPlatformFileValue) > SendCLDDataAvailable(handle); Done. https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:272: DCHECK(!created); On 2014/03/24 19:01:47, Chromium Palmer wrote: > I wonder if this should be a cause for LOG and return, rather than just a > DCHECK. Probably not. We're attempting to open a file for read-only access. Any creation of the file by the opening attempt is a serious programming error that would affect tons of code. The DCHECK is for sanity. https://codereview.chromium.org/187393005/diff/320001/chrome/browser/translat... chrome/browser/translate/translate_tab_helper.cc:281: s_file_lock_.Get().Release(); On 2014/03/25 09:29:26, bulach wrote: > On 2014/03/24 19:01:47, Chromium Palmer wrote: > > This, and line 288, make me think you want an Autolock. > > yep, something like: > > { > AutoLock(...); > if (s_cached_platform_file_ != base::kInvalidPlatformFileValue) > s_cached_platform_file_ = file; > } > > if (file != s_cached_platform_file_) { > // Idempotence: racing another request on the blocking pool. > base::ClosePlatformFile(file); > } Done. https://codereview.chromium.org/187393005/diff/320001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/187393005/diff/320001/chrome/common/render_me... chrome/common/render_messages.h:798: // should go unanswered (the renderer will ask again later) On 2014/03/24 19:01:47, Chromium Palmer wrote: > Nit: Puncutation. Done. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:124: && url.DomainIs(file_manager::kFileManagerAppId)) On 2014/03/25 09:29:26, bulach wrote: > nit: && in the previous line Done. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:589: // Terminate immediately if told to stop polling On 2014/03/24 19:01:47, Chromium Palmer wrote: > Nit: Punctuate. Done. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:601: // chain of polling that will last until the pointer stops being null, On 2014/03/24 19:01:47, Chromium Palmer wrote: > Nit: NULL Done. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:605: // It's only while downloading the file that we expect this to chain for a Grammar police, don't use pronouns in comments. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:630: deferred_page_id_ = -1; // Clean up for sanity On 2014/03/24 19:01:47, Chromium Palmer wrote: > Nit: punctuation, but really these comments are superfluous. Deleted. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:637: // Terminate immediately if told to stop polling On 2014/03/24 19:01:47, Chromium Palmer wrote: > Nit: Punctuate. Done. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:648: DCHECK(platform_file != base::kInvalidPlatformFileValue) On 2014/03/24 19:01:47, Chromium Palmer wrote: > This should be a production, run-time check, not a DCHECK. Done. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:182: // The mmap. The mmap dstructor will unmap the memory segment and close the On 2014/03/24 19:01:47, Chromium Palmer wrote: > Nit: typo "destructor". Done. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:183: // file handle; it must be held throughought the lifetime of the process. On 2014/03/25 09:29:26, bulach wrote: > I think valgrind and friends will complain about this object being leaked. > LazyInstance::Leaky is the pattern used for this kind of objects, it annotates > correctly so that the leak detection tools won't flag them. So.... LazyInstance:Leaky of a pointer? I'll talk to you offline, the rabbit hole is too deep for a comment thread. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:184: static base::MemoryMappedFile* s_cld2_data_file_mmap_; On 2014/03/25 09:29:26, bulach wrote: > nit: also, please move the fields (164-184) after the methods. Done. https://codereview.chromium.org/187393005/diff/320001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:212: #endif On 2014/03/24 19:01:47, Chromium Palmer wrote: > Nit: Should be a blank line after this. Done. https://codereview.chromium.org/187393005/diff/320001/third_party/cld_2/cld_2... File third_party/cld_2/cld_2.gyp (right): https://codereview.chromium.org/187393005/diff/320001/third_party/cld_2/cld_2... third_party/cld_2/cld_2.gyp:27: 'src/internal/cld2_dynamic_data.h', On 2014/03/24 19:01:47, Chromium Palmer wrote: > How is this code getting tested? Is there a fuzzer? Et c. The testing is done via unit tests in CLD2, which were modified to fully exercise the dynamic data use cases. All unit tests in CLD2 can be built in either dynamic or static mode. Of course, they all pass. There's no fuzzer. My stance so far has been that... 1. On Android, this file will be part of a signed APK that we generated and that lives in the application's private directory. Therefore it is secure, and shouldn't be any more of a security concern than any other data files we might have. 2. On other platforms, the file is as well protected as the OS makes it, neither more or less secure than anything else Chromium deals with. It's not abundantly clear to me what a fuzzer would be able to do here, since the data within the frozen structures is all critical to the correct functioning of the code (e.g., shifts and offsets in arrays of characters). Fuzzing anything in the data set would immediately and completely break the code in weird ways. IANAE on this, but if you have docs on chromium fuzzers that I could somehow adapt I'd be happy to take a look. I guess my point is that right now I think we're as (in)secure as we are with non-dynamic CLD1; anyone able to access the filesystem can corrupt the CLD data, be that within the readonly block in the .so or within the standalone data file. CLD has not and does not have any hardening against attacks of this type.
Marcus, Palmer, PTAL. I believe I've addressed all your concerns now.
PS - thanks to Marcus for the excellent example on using LazyInstance::Leaky. This code is patterned after this pattern: https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util.cc
still lgtm, just tiny nits.. https://codereview.chromium.org/187393005/diff/280001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/187393005/diff/280001/build/common.gypi#newco... build/common.gypi:428: # Only evaluated if cld_version == 2 or if building the CLD2 dynamic data tool explicitly. On 2014/03/25 09:29:26, bulach wrote: > On 2014/03/24 15:18:24, Andrew Hayden wrote: > > On 2014/03/23 20:57:02, bulach wrote: > > > nit: there's no strict limit, but from 414-418, try to keep <80cols > > > > Done. > > sorry to keep pinging here, but it still looks >80 cols (and 437 too) ping? https://codereview.chromium.org/187393005/diff/400001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/187393005/diff/400001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.cc:665: if (((uint64) (data_length + data_offset) > s_cld_mmap_.Get().value->length()) nit: static_cast<uint64> instead of (c-style)? https://codereview.chromium.org/187393005/diff/400001/chrome/renderer/transla... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/187393005/diff/400001/chrome/renderer/transla... chrome/renderer/translate/translate_helper.h:166: static base::MemoryMappedFile* s_cld2_data_file_mmap_; remove it?
And the >80 cols thing was fixed till I "git checkout"'d my common.gypi file again, duhr. Fixed now. https://chromiumcodereview.appspot.com/187393005/diff/400001/chrome/renderer/... File chrome/renderer/translate/translate_helper.cc (right): https://chromiumcodereview.appspot.com/187393005/diff/400001/chrome/renderer/... chrome/renderer/translate/translate_helper.cc:665: if (((uint64) (data_length + data_offset) > s_cld_mmap_.Get().value->length()) On 2014/03/26 19:06:46, bulach wrote: > nit: static_cast<uint64> instead of (c-style)? Even better, use uint64 for all the offsets and sizes since a negative value would be nonsensical. Done! https://chromiumcodereview.appspot.com/187393005/diff/400001/chrome/renderer/... File chrome/renderer/translate/translate_helper.h (right): https://chromiumcodereview.appspot.com/187393005/diff/400001/chrome/renderer/... chrome/renderer/translate/translate_helper.h:166: static base::MemoryMappedFile* s_cld2_data_file_mmap_; On 2014/03/26 19:06:46, bulach wrote: > remove it? Whoops! thanks.
https://chromiumcodereview.appspot.com/187393005/diff/440001/chrome/renderer/... File chrome/renderer/translate/translate_helper.cc (right): https://chromiumcodereview.appspot.com/187393005/diff/440001/chrome/renderer/... chrome/renderer/translate/translate_helper.cc:666: uint64 max_int32 = (static_cast<uint64>(1) << 31) - 1; Nit: std::numeric_limits<int32>::max() is your fiend :) http://www.cplusplus.com/reference/limits/numeric_limits/
https://chromiumcodereview.appspot.com/187393005/diff/440001/chrome/renderer/... File chrome/renderer/translate/translate_helper.cc (right): https://chromiumcodereview.appspot.com/187393005/diff/440001/chrome/renderer/... chrome/renderer/translate/translate_helper.cc:666: uint64 max_int32 = (static_cast<uint64>(1) << 31) - 1; On 2014/03/27 09:52:26, Philippe wrote: > Nit: std::numeric_limits<int32>::max() is your fiend :) > > http://www.cplusplus.com/reference/limits/numeric_limits/ Oops, s/fiend/friend :)
https://chromiumcodereview.appspot.com/187393005/diff/440001/chrome/renderer/... File chrome/renderer/translate/translate_helper.cc (right): https://chromiumcodereview.appspot.com/187393005/diff/440001/chrome/renderer/... chrome/renderer/translate/translate_helper.cc:666: uint64 max_int32 = (static_cast<uint64>(1) << 31) - 1; On 2014/03/27 09:52:58, Philippe wrote: > On 2014/03/27 09:52:26, Philippe wrote: > > Nit: std::numeric_limits<int32>::max() is your fiend :) > > > > http://www.cplusplus.com/reference/limits/numeric_limits/ > > Oops, s/fiend/friend :) Done.
Final version, as far as I'm concerned. Everything appears to work on Android and Linux, all the includes are ifdef's for consistency, and we should be good to go here. I believe I have LGTMs from all owners as well as security (also got an LGTM from security via email), so I believe we're ready to commit here. Again, for any sheriffs and for posterity, this change DOES NOT change any of the builds to use dynamic mode; it merely enables it. There should be no functional changes from this change as-is, on any platform.
The CQ bit was checked by andrewhayden@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/187393005/47...
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by andrewhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/187393005/47...
Message was sent while issue was closed.
Change committed as 259900 |
