Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
173357:
Error diagnostics for Blacklist IO... (Closed)
|
Created:
11 years, 4 months ago by idanan Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionError diagnostics for Blacklist IO
For now, as with other extension errors, these are not i18n'd.
The blacklist loading error is though because it is most like
an error to be shown to end users.
BUG=16932
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25895
Patch Set 1 #
Total comments: 36
Patch Set 2 : '' #
Total comments: 9
Patch Set 3 : '' #
Total comments: 4
Patch Set 4 : '' #
Total comments: 9
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 2
Messages
Total messages: 21 (0 generated)
Implemented error reporting for blacklist errors. Pawel is waiting on the change for the extensions loading and validation for Privacy Blacklists. - Itai
Drive-by (and obviously the code I'm going to write will be interfacing with this code). It's not a thorough review, I only glanced at some things not directly related to error reporting (like the message dialog etc). http://codereview.chromium.org/173357/diff/1/8 File chrome/browser/privacy_blacklist/blacklist.h (right): http://codereview.chromium.org/173357/diff/1/8#newcode162 Line 162: bool good() const { return good_; } nit: Please name it is_good, or is_valid. http://codereview.chromium.org/173357/diff/1/8#newcode179 Line 179: bool good_; Please add a comment here. http://codereview.chromium.org/173357/diff/1/4 File chrome/browser/privacy_blacklist/blacklist_io.h (right): http://codereview.chromium.org/173357/diff/1/4#newcode30 Line 30: string16 error() const { nit: Please name it last_error. http://codereview.chromium.org/173357/diff/1/4#newcode45 Line 45: string16 error_; Please add comment here. http://codereview.chromium.org/173357/diff/1/5 File chrome/browser/privacy_blacklist/blacklist_store.cc (right): http://codereview.chromium.org/173357/diff/1/5#newcode31 Line 31: : file_(file), good_(true) { I'd prefer good initialized to false. Safer. http://codereview.chromium.org/173357/diff/1/5#newcode71 Line 71: return std::numeric_limits<uint32>::max(); I kind of don't like an API which makes uint32-max a special value. I'd prefer this method returning a bool success value, but it's not a strong opinion. http://codereview.chromium.org/173357/diff/1/5#newcode90 Line 90: : file_(file), good_(true) { Initialize good to false. http://codereview.chromium.org/173357/diff/1/6 File chrome/browser/privacy_blacklist/blacklist_store.h (right): http://codereview.chromium.org/173357/diff/1/6#newcode32 Line 32: bool good() const { return good_; } nit: Please name it is_good, or is valid. http://codereview.chromium.org/173357/diff/1/6#newcode35 Line 35: bool ReserveProviders(uint32); Please describe what the return value means. http://codereview.chromium.org/173357/diff/1/6#newcode38 Line 38: bool StoreProvider(const std::string& name, const std::string& url); Here too. http://codereview.chromium.org/173357/diff/1/6#newcode41 Line 41: bool ReserveEntries(uint32); And here. http://codereview.chromium.org/173357/diff/1/6#newcode44 Line 44: bool StoreEntry(const std::string& pattern, And here. http://codereview.chromium.org/173357/diff/1/6#newcode51 Line 51: bool WriteUInt(uint32); Here too. http://codereview.chromium.org/173357/diff/1/6#newcode52 Line 52: bool WriteString(const std::string&); And here as well. http://codereview.chromium.org/173357/diff/1/6#newcode55 Line 55: bool good_; Please make a comment what that means. http://codereview.chromium.org/173357/diff/1/6#newcode75 Line 75: bool good() const { return good_; } nit: Please make this is_good, or is_valid. http://codereview.chromium.org/173357/diff/1/6#newcode81 Line 81: bool ReadProvider(std::string* name, std::string* url); Explain the return value. http://codereview.chromium.org/173357/diff/1/6#newcode87 Line 87: bool ReadEntry(std::string* pattern, Here too. http://codereview.chromium.org/173357/diff/1/6#newcode97 Line 97: bool good_; Comment. http://codereview.chromium.org/173357/diff/1/12 File chrome/browser/views/blacklist_error_dialog.h (right): http://codereview.chromium.org/173357/diff/1/12#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. The copyright should say just 2009. http://codereview.chromium.org/173357/diff/1/12#newcode8 Line 8: #ifndef CHROME_BROWSER_BLACKLIST_ERROR_DIALOG_H__ nit: There should be exactly one underscore at the end. http://codereview.chromium.org/173357/diff/1/12#newcode59 Line 59: DISALLOW_EVIL_CONSTRUCTORS(BlacklistErrorDialog); nit: DISALLOW_COPY_AND_ASSIGN is more "politically correct" version.
http://codereview.chromium.org/173357/diff/1/10 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/173357/diff/1/10#newcode4830 Line 4830: Your privacy is no longer protected err, I can't really agree with this text. http://codereview.chromium.org/173357/diff/1/10#newcode4833 Line 4833: <!-- ProcessSingleton --> Was the change wanted? http://codereview.chromium.org/173357/diff/1/2 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/173357/diff/1/2#newcode557 Line 557: return ResultCodes::NORMAL_EXIT; alignment... http://codereview.chromium.org/173357/diff/1/4 File chrome/browser/privacy_blacklist/blacklist_io.h (right): http://codereview.chromium.org/173357/diff/1/4#newcode30 Line 30: string16 error() const { const string16& http://codereview.chromium.org/173357/diff/1/6 File chrome/browser/privacy_blacklist/blacklist_store.h (right): http://codereview.chromium.org/173357/diff/1/6#newcode51 Line 51: bool WriteUInt(uint32); Do you really care that it fails? Does anything needs to be surfaced to the user? http://codereview.chromium.org/173357/diff/1/11 File chrome/browser/views/blacklist_error_dialog.cc (right): http://codereview.chromium.org/173357/diff/1/11#newcode19 Line 19: BlacklistErrorDialog* dlg = new BlacklistErrorDialog; Is that a memory leak? http://codereview.chromium.org/173357/diff/1/11#newcode26 Line 26: const int kDialogWidth = 400; Does the user _really_ care? http://codereview.chromium.org/173357/diff/1/12 File chrome/browser/views/blacklist_error_dialog.h (right): http://codereview.chromium.org/173357/diff/1/12#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. 2009 http://codereview.chromium.org/173357/diff/1/12#newcode8 Line 8: #ifndef CHROME_BROWSER_BLACKLIST_ERROR_DIALOG_H__ _ http://codereview.chromium.org/173357/diff/1/12#newcode11 Line 11: #include "base/basictypes.h" not needed AFAIK http://codereview.chromium.org/173357/diff/1/12#newcode13 Line 13: #include "chrome/browser/shell_dialogs.h" It doesn't look like it's needed. http://codereview.chromium.org/173357/diff/1/12#newcode16 Line 16: class MessageBoxView; one vertical space http://codereview.chromium.org/173357/diff/1/12#newcode19 Line 19: } // namespace views http://codereview.chromium.org/173357/diff/1/12#newcode52 Line 52: //scoped_refptr<SelectFileDialog> select_file_dialog_; remove that
On 2009/08/25 18:35:05, Marc-Antoine Ruel wrote: > http://codereview.chromium.org/173357/diff/1/10 > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/173357/diff/1/10#newcode4830 > Line 4830: Your privacy is no longer protected > err, I can't really agree with this text. > > http://codereview.chromium.org/173357/diff/1/10#newcode4833 > Line 4833: <!-- ProcessSingleton --> > Was the change wanted? Accident. Reverted. > http://codereview.chromium.org/173357/diff/1/2 > File chrome/browser/browser_main.cc (right): > > http://codereview.chromium.org/173357/diff/1/2#newcode557 > Line 557: return ResultCodes::NORMAL_EXIT; > alignment... Done. > http://codereview.chromium.org/173357/diff/1/4 > File chrome/browser/privacy_blacklist/blacklist_io.h (right): > > http://codereview.chromium.org/173357/diff/1/4#newcode30 > Line 30: string16 error() const { > const string16& Done. > http://codereview.chromium.org/173357/diff/1/6 > File chrome/browser/privacy_blacklist/blacklist_store.h (right): > > http://codereview.chromium.org/173357/diff/1/6#newcode51 > Line 51: bool WriteUInt(uint32); > Do you really care that it fails? Does anything needs to be surfaced to the > user? Here the error is simply bubbled up to the higher layer that will report this when installing a new blacklist. > http://codereview.chromium.org/173357/diff/1/11 > File chrome/browser/views/blacklist_error_dialog.cc (right): > > http://codereview.chromium.org/173357/diff/1/11#newcode19 > Line 19: BlacklistErrorDialog* dlg = new BlacklistErrorDialog; > Is that a memory leak? No. See comment two lines above. This is copied and adapted the UserDataDirDialog class. > > http://codereview.chromium.org/173357/diff/1/11#newcode26 > Line 26: const int kDialogWidth = 400; > Does the user _really_ care? Same in UserDataDirDialog. Without this the dialog looks "tight". > http://codereview.chromium.org/173357/diff/1/12 > File chrome/browser/views/blacklist_error_dialog.h (right): > > http://codereview.chromium.org/173357/diff/1/12#newcode1 > Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. > 2009 Done. > http://codereview.chromium.org/173357/diff/1/12#newcode8 > Line 8: #ifndef CHROME_BROWSER_BLACKLIST_ERROR_DIALOG_H__ > _ Done. Double standards here? :) Same in user_data_dir_dialog.* > http://codereview.chromium.org/173357/diff/1/12#newcode11 > Line 11: #include "base/basictypes.h" > not needed AFAIK Done. > http://codereview.chromium.org/173357/diff/1/12#newcode13 > Line 13: #include "chrome/browser/shell_dialogs.h" > It doesn't look like it's needed. Done. > http://codereview.chromium.org/173357/diff/1/12#newcode16 > Line 16: class MessageBoxView; > one vertical space Done. > http://codereview.chromium.org/173357/diff/1/12#newcode19 > Line 19: } > // namespace views Done. BTW, same in user_data_dir_dialog.h > http://codereview.chromium.org/173357/diff/1/12#newcode52 > Line 52: //scoped_refptr<SelectFileDialog> select_file_dialog_; > remove that Done. All sort of comments about naming of variables from Pawel also done. Still investigating Mac/Linux failures. - Itai
Some more nits. http://codereview.chromium.org/173357/diff/1015/31 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/173357/diff/1015/31#newcode4826 Line 4826: <message name="IDS_BLACKLIST_ERROR_LOADING_TITLE" desc="Title of the dialog shown when the privacy blacklist cannot be loaeded."> nit: There is a typo here "loaeded". http://codereview.chromium.org/173357/diff/1015/27 File chrome/browser/privacy_blacklist/blacklist_store.h (right): http://codereview.chromium.org/173357/diff/1015/27#newcode34 Line 34: // Sets the number of providers stored. Return true if successful. nit: If the comment says "Sets", then it should be "Returns". Alternatively, "Set" and "Return". http://codereview.chromium.org/173357/diff/1015/27#newcode37 Line 37: // Stores a provider. Return true if successful. nit: Same here. http://codereview.chromium.org/173357/diff/1015/27#newcode41 Line 41: bool ReserveEntries(uint32); nit: Same here. http://codereview.chromium.org/173357/diff/1015/27#newcode44 Line 44: bool StoreEntry(const std::string& pattern, nit: Same here. http://codereview.chromium.org/173357/diff/1015/27#newcode50 Line 50: // Writes basic types to the stream. Return true if successful. nit: Same here. http://codereview.chromium.org/173357/diff/1015/27#newcode80 Line 80: // Reads a provider. Add "Returns true on success.". http://codereview.chromium.org/173357/diff/1015/27#newcode86 Line 86: // Reads an entry. Same here. http://codereview.chromium.org/173357/diff/1015/33 File chrome/browser/views/blacklist_error_dialog.h (right): http://codereview.chromium.org/173357/diff/1015/33#newcode57 Line 57: DISALLOW_EVIL_CONSTRUCTORS(BlacklistErrorDialog); nit: Should be DISALLOW_COPY_AND_ASSIGN. DISALLOW_EVIL_CONSTRUCTORS is deprecated. The meaning is exactly the same, see base/basictypes.h.
Good catch on the typo. All done. Will upload shortly. - Itai On Tue, Aug 25, 2009 at 14:34, <phajdan.jr@chromium.org> wrote: > Some more nits. > > > http://codereview.chromium.org/173357/diff/1015/31 > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/173357/diff/1015/31#newcode4826 > Line 4826: <message name="IDS_BLACKLIST_ERROR_LOADING_TITLE" desc="Title > of the dialog shown when the privacy blacklist cannot be loaeded."> > nit: There is a typo here "loaeded". > > http://codereview.chromium.org/173357/diff/1015/27 > File chrome/browser/privacy_blacklist/blacklist_store.h (right): > > http://codereview.chromium.org/173357/diff/1015/27#newcode34 > Line 34: // Sets the number of providers stored. Return true if > successful. > nit: If the comment says "Sets", then it should be "Returns". > Alternatively, "Set" and "Return". > > http://codereview.chromium.org/173357/diff/1015/27#newcode37 > Line 37: // Stores a provider. Return true if successful. > nit: Same here. > > http://codereview.chromium.org/173357/diff/1015/27#newcode41 > Line 41: bool ReserveEntries(uint32); > nit: Same here. > > http://codereview.chromium.org/173357/diff/1015/27#newcode44 > Line 44: bool StoreEntry(const std::string& pattern, > nit: Same here. > > http://codereview.chromium.org/173357/diff/1015/27#newcode50 > Line 50: // Writes basic types to the stream. Return true if successful. > nit: Same here. > > http://codereview.chromium.org/173357/diff/1015/27#newcode80 > Line 80: // Reads a provider. > Add "Returns true on success.". > > http://codereview.chromium.org/173357/diff/1015/27#newcode86 > Line 86: // Reads an entry. > Same here. > > http://codereview.chromium.org/173357/diff/1015/33 > File chrome/browser/views/blacklist_error_dialog.h (right): > > http://codereview.chromium.org/173357/diff/1015/33#newcode57 > Line 57: DISALLOW_EVIL_CONSTRUCTORS(BlacklistErrorDialog); > nit: Should be DISALLOW_COPY_AND_ASSIGN. DISALLOW_EVIL_CONSTRUCTORS is > deprecated. The meaning is exactly the same, see base/basictypes.h. > > http://codereview.chromium.org/173357 >
OK. Uploaded and fixed comments. Had to ifdef out for non-windows platforms usage of BlacklistErrorDialog just like the UserDataDirDialog which I used as a basic. It would be better to complete the abstraction for message loop dispatching on Linux and Mac as a separate task. - Itai
Seems like some too clever tool reformated one file... After fixing that, LGTM (but please also wait for maruel's comments). http://codereview.chromium.org/173357/diff/1019/1020 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/173357/diff/1019/1020#newcode128 Line 128: void WillInitializeMainMessageLoop(const MainFunctionParams& parameters); Oops! While did suddenly all the thing inside the namespace become indented? See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces http://codereview.chromium.org/173357/diff/1019/1020#newcode289 Line 289: parsed_command_line.GetSwitchValue(switches::kFileDescriptorLimit); Oops! It should be indented 4 spaces. http://codereview.chromium.org/173357/diff/1019/1020#newcode340 Line 340: parsed_command_line.HasSwitch(switches::kFirstRun); Here too. http://codereview.chromium.org/173357/diff/1019/1020#newcode561 Line 561: return ResultCodes::NORMAL_EXIT; This hanging return doesn't look pretty to me. Could you move bool accepted out of the ifdef with some default value, and only RunBlacklistDialog inside the ifdef? This way the entire if can be moved outside the ifdefs.
On Tue, Aug 25, 2009 at 16:23, <phajdan.jr@chromium.org> wrote: > Seems like some too clever tool reformated one file... > > After fixing that, LGTM (but please also wait for maruel's comments). > > > http://codereview.chromium.org/173357/diff/1019/1020 > File chrome/browser/browser_main.cc (right): > > http://codereview.chromium.org/173357/diff/1019/1020#newcode128 > Line 128: void WillInitializeMainMessageLoop(const MainFunctionParams& > parameters); > Oops! While did suddenly all the thing inside the namespace become > indented? See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces Damn VC! I reverted the file and applied my changes again because the damage was so bad. Over RD, I did not even notice when it did that. I changed the Editor options from "Smart" indent to "Block" indent. Hopefully this will not cause anymore problems. > http://codereview.chromium.org/173357/diff/1019/1020#newcode289 > Line 289: > parsed_command_line.GetSwitchValue(switches::kFileDescriptorLimit); > Oops! It should be indented 4 spaces. > > http://codereview.chromium.org/173357/diff/1019/1020#newcode340 > Line 340: parsed_command_line.HasSwitch(switches::kFirstRun); > Here too. > > http://codereview.chromium.org/173357/diff/1019/1020#newcode561 > Line 561: return ResultCodes::NORMAL_EXIT; > This hanging return doesn't look pretty to me. Could you move bool > accepted out of the ifdef with some default value, and only > RunBlacklistDialog inside the ifdef? This way the entire if can be moved > outside the ifdefs. > > http://codereview.chromium.org/173357 >
http://codereview.chromium.org/173357/diff/1033/48 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/173357/diff/1033/48#newcode4830 Line 4830: Your privacy is no longer protected I still think the messaging is wrong here. http://codereview.chromium.org/173357/diff/1033/40 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/173357/diff/1033/40#newcode555 Line 555: // TODO(idanan): Enable this for other platforms once the dispatching support 80 cols, you should take a look at http://dev.chromium.org/developers/how-tos/visualstudio-tricks http://codereview.chromium.org/173357/diff/1033/40#newcode560 Line 560: #endif do a if ... return ResultCodes::NORMAL_EXIT; #else return ResultCodes::NORMAL_EXIT; #endif otherwise the code alignment is messy. http://codereview.chromium.org/173357/diff/1033/45 File chrome/browser/privacy_blacklist/blacklist.cc (right): http://codereview.chromium.org/173357/diff/1033/45#newcode66 Line 66: bool Blacklist::Matches(const std::string& pattern, const std::string& url) { Shouldn't you add DCHECK(is_good()); in a few functions? http://codereview.chromium.org/173357/diff/1033/45#newcode163 Line 163: Blacklist::Blacklist(const FilePath& file) : is_good_(false) { Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't the list just be empty when the list failed to load? That's fine to have a signal to verify that but you don't cleanup providers_ when it fails latter. http://codereview.chromium.org/173357/diff/1033/46 File chrome/browser/privacy_blacklist/blacklist.h (right): http://codereview.chromium.org/173357/diff/1033/46#newcode161 Line 161: // Returns true if the blacklist object is in good health. Could be further shortened to: "Is the blacklist object in good health" But, in the end, do you really need that? See .cc comment. http://codereview.chromium.org/173357/diff/1033/41 File chrome/browser/privacy_blacklist/blacklist_io.cc (right): http://codereview.chromium.org/173357/diff/1033/41#newcode139 Line 139: last_error_ = ASCIIToUTF16("Unexpected ( in attribute parameters."); 80 cols http://codereview.chromium.org/173357/diff/1033/42 File chrome/browser/privacy_blacklist/blacklist_io.h (right): http://codereview.chromium.org/173357/diff/1033/42#newcode30 Line 30: const string16& last_error() const { nit: I think this design would badly degrade in failure mode. Imagine an overflow of blacklisted IO, that would significantly increase the memory usage if the string is large. Especially that it would always be the same string. Also, I think it's too deep to keep localized data. http://codereview.chromium.org/173357/diff/1033/43 File chrome/browser/privacy_blacklist/blacklist_store.cc (right): http://codereview.chromium.org/173357/diff/1033/43#newcode126 Line 126: if (uint32 n = ReadUInt()) { Shouldn't you put an arbitrary upper limit for a valid supported range?
On 2009/08/26 13:53:40, Marc-Antoine Ruel wrote: > http://codereview.chromium.org/173357/diff/1033/48 > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/173357/diff/1033/48#newcode4830 > Line 4830: Your privacy is no longer protected > I still think the messaging is wrong here. What would you prefer? I think this gives exactly the consequence of the error, as opposed to something like "Error loading Privacy Blacklist". > http://codereview.chromium.org/173357/diff/1033/40 > File chrome/browser/browser_main.cc (right): > > http://codereview.chromium.org/173357/diff/1033/40#newcode555 > Line 555: // TODO(idanan): Enable this for other platforms once the dispatching > support > 80 cols, you should take a look at > http://dev.chromium.org/developers/how-tos/visualstudio-tricks > > http://codereview.chromium.org/173357/diff/1033/40#newcode560 > Line 560: #endif > do a > if ... > return ResultCodes::NORMAL_EXIT; > #else > return ResultCodes::NORMAL_EXIT; > #endif > > otherwise the code alignment is messy. Done. > http://codereview.chromium.org/173357/diff/1033/45 > File chrome/browser/privacy_blacklist/blacklist.cc (right): > > http://codereview.chromium.org/173357/diff/1033/45#newcode66 > Line 66: bool Blacklist::Matches(const std::string& pattern, const std::string& > url) { > Shouldn't you add DCHECK(is_good()); in a few functions? I could but is_good is only designed to be checked once, upon initialization. The blacklist never goes from good to bad. > http://codereview.chromium.org/173357/diff/1033/45#newcode163 > Line 163: Blacklist::Blacklist(const FilePath& file) : is_good_(false) { > Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't the list just > be empty when the list failed to load? That's fine to have a signal to verify > that but you don't cleanup providers_ when it fails latter. No because it only needs to be checked once. The reason there is even an is good function is because of dependencies which meant I could not do the UI dialog right in the blacklist. > http://codereview.chromium.org/173357/diff/1033/46 > File chrome/browser/privacy_blacklist/blacklist.h (right): > > http://codereview.chromium.org/173357/diff/1033/46#newcode161 > Line 161: // Returns true if the blacklist object is in good health. > Could be further shortened to: > "Is the blacklist object in good health" > > But, in the end, do you really need that? See .cc comment. As I just said, it is needed to break the dependencies so that we can show the user when it does not load. It is extremely important that we do so because otherwise users would have a false impression about how their privacy is protected. > http://codereview.chromium.org/173357/diff/1033/41 > File chrome/browser/privacy_blacklist/blacklist_io.cc (right): > > http://codereview.chromium.org/173357/diff/1033/41#newcode139 > Line 139: last_error_ = ASCIIToUTF16("Unexpected ( in attribute parameters."); > 80 cols Done. > http://codereview.chromium.org/173357/diff/1033/42 > File chrome/browser/privacy_blacklist/blacklist_io.h (right): > > http://codereview.chromium.org/173357/diff/1033/42#newcode30 > Line 30: const string16& last_error() const { > nit: I think this design would badly degrade in failure mode. Imagine an > overflow of blacklisted IO, that would significantly increase the memory usage > if the string is large. Especially that it would always be the same string. > Also, I think it's too deep to keep localized data. Not sure I understand but you should know that this I/O code is only invoked by the extension installation code which Pawel is writing. Chrome would not check these once the the blacklists are installed and compiled. > http://codereview.chromium.org/173357/diff/1033/43 > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): > > http://codereview.chromium.org/173357/diff/1033/43#newcode126 > Line 126: if (uint32 n = ReadUInt()) { > Shouldn't you put an arbitrary upper limit for a valid supported range? I did not see a point to do anything arbitrary, but I can if you really want me too. - Itai
On 2009/08/26 16:46:07, idanan wrote: > On 2009/08/26 13:53:40, Marc-Antoine Ruel wrote: > > http://codereview.chromium.org/173357/diff/1033/48 > > File chrome/app/generated_resources.grd (right): > > > > http://codereview.chromium.org/173357/diff/1033/48#newcode4830 > > Line 4830: Your privacy is no longer protected > > I still think the messaging is wrong here. > > What would you prefer? I think this gives exactly the consequence > of the error, as opposed to something like "Error loading Privacy > Blacklist". To me, the failure to load the privacy blacklist is *AT MOST* a toolbar popup, like the 'do you want to save password' or 'set chrome as default browser'. If I want to browse the web *now* I probably don't care that the privacy blacklist failed to load, and I want even less be scared by a message telling me that my privacy is suddenly invaded by obscure stuff. The information in this message is at best misleading. I definitely want the point of view of someone in the UI team for that. In any case, the privacy list loading should be asynchronous and non blocking, so even if the first socket connections starts without the list being fully loaded, I probably wouldn't care. Since the list loading should probably be asynchronous, this doesn't deserve a synchronous popup. > > http://codereview.chromium.org/173357/diff/1033/45 > > File chrome/browser/privacy_blacklist/blacklist.cc (right): > > > > http://codereview.chromium.org/173357/diff/1033/45#newcode66 > > Line 66: bool Blacklist::Matches(const std::string& pattern, const > std::string& > > url) { > > Shouldn't you add DCHECK(is_good()); in a few functions? > I could but is_good is only designed to be checked once, upon initialization. > The blacklist never goes from good to bad. You know that. Does the function callers in 2 years will remember that? > > http://codereview.chromium.org/173357/diff/1033/45#newcode163 > > Line 163: Blacklist::Blacklist(const FilePath& file) : is_good_(false) { > > Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't the list > just > > be empty when the list failed to load? That's fine to have a signal to verify > > that but you don't cleanup providers_ when it fails latter. > > No because it only needs to be checked once. The reason there is even an > is good function is because of dependencies which meant I could not do > the UI dialog right in the blacklist. > > > http://codereview.chromium.org/173357/diff/1033/46 > > File chrome/browser/privacy_blacklist/blacklist.h (right): > > > > http://codereview.chromium.org/173357/diff/1033/46#newcode161 > > Line 161: // Returns true if the blacklist object is in good health. > > Could be further shortened to: > > "Is the blacklist object in good health" > > > > But, in the end, do you really need that? See .cc comment. > > As I just said, it is needed to break the dependencies so that we > can show the user when it does not load. It is extremely important > that we do so because otherwise users would have a false impression > about how their privacy is protected. My point is roughly that 'good' has different meaning. You probably want to know if the list is 'valid()' or not. But even though, if the list is not valid, what's the deal? Frankly, I'd want the point of view of someone else on that since I'm not a fan of surfacing too many errors. > > http://codereview.chromium.org/173357/diff/1033/42 > > File chrome/browser/privacy_blacklist/blacklist_io.h (right): > > > > http://codereview.chromium.org/173357/diff/1033/42#newcode30 > > Line 30: const string16& last_error() const { > > nit: I think this design would badly degrade in failure mode. Imagine an > > overflow of blacklisted IO, that would significantly increase the memory usage > > if the string is large. Especially that it would always be the same string. > > Also, I think it's too deep to keep localized data. > > Not sure I understand but you should know that this I/O code is only invoked > by the extension installation code which Pawel is writing. Chrome would not > check these once the the blacklists are installed and compiled. ok, but if you look at the rest of the chrome code, we usually don't surface low level errors like "file read failed" or things like that. > > http://codereview.chromium.org/173357/diff/1033/43 > > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): > > > > http://codereview.chromium.org/173357/diff/1033/43#newcode126 > > Line 126: if (uint32 n = ReadUInt()) { > > Shouldn't you put an arbitrary upper limit for a valid supported range? > > I did not see a point to do anything arbitrary, but I can if you really > want me too. Simplest path to DoS. All your cute error messages won't do a thing about the crash that will occur when you try to load an invalid file that simply gobs all the available memory. M-A
On Wed, Aug 26, 2009 at 10:14, <maruel@chromium.org> wrote: > On 2009/08/26 16:46:07, idanan wrote: >> >> On 2009/08/26 13:53:40, Marc-Antoine Ruel wrote: >> > http://codereview.chromium.org/173357/diff/1033/48 >> > File chrome/app/generated_resources.grd (right): >> > >> > http://codereview.chromium.org/173357/diff/1033/48#newcode4830 >> > Line 4830: Your privacy is no longer protected >> > I still think the messaging is wrong here. > >> What would you prefer? I think this gives exactly the consequence >> of the error, as opposed to something like "Error loading Privacy >> Blacklist". > > To me, the failure to load the privacy blacklist is *AT MOST* a toolbar > popup, like the 'do you want to save password' or 'set chrome as default > browser'. If I want to browse the web *now* I probably don't care that > the privacy blacklist failed to load, and I want even less be scared by > a message telling me that my privacy is suddenly invaded by obscure > stuff. The information in this message is at best misleading. I > definitely want the point of view of someone in the UI team for that. > > In any case, the privacy list loading should be asynchronous and non > blocking, so even if the first socket connections starts without the > list being fully loaded, I probably wouldn't care. > > Since the list loading should probably be asynchronous, this doesn't > deserve a synchronous popup. Sorry but I strongly disagree here. Every access must be checked against the blacklist, we can't load it later because if users don't want to send cookies or something, they don't want us to send them while we wait for the blacklist to be loaded. If the blacklist fails to load, which should be a rare occurrence because we are talking about the compiled blacklist here (disk corruption or incorrect command line are the two possibilities), the user must know and acknowledge that their privacy is not protected the way they chose by installing their blacklists. If you don't care about privacy and have no blacklists, none of this will every show. >> > http://codereview.chromium.org/173357/diff/1033/45 >> > File chrome/browser/privacy_blacklist/blacklist.cc (right): >> > >> > http://codereview.chromium.org/173357/diff/1033/45#newcode66 >> > Line 66: bool Blacklist::Matches(const std::string& pattern, const >> std::string& >> > url) { >> > Shouldn't you add DCHECK(is_good()); in a few functions? >> I could but is_good is only designed to be checked once, upon > > initialization. >> >> The blacklist never goes from good to bad. > > You know that. Does the function callers in 2 years will remember that? OK, fair. >> > http://codereview.chromium.org/173357/diff/1033/45#newcode163 >> > Line 163: Blacklist::Blacklist(const FilePath& file) : > > is_good_(false) { >> >> > Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't > > the list >> >> just >> > be empty when the list failed to load? That's fine to have a signal > > to verify >> >> > that but you don't cleanup providers_ when it fails latter. > >> No because it only needs to be checked once. The reason there is even > > an >> >> is good function is because of dependencies which meant I could not do > >> the UI dialog right in the blacklist. > >> > http://codereview.chromium.org/173357/diff/1033/46 >> > File chrome/browser/privacy_blacklist/blacklist.h (right): >> > >> > http://codereview.chromium.org/173357/diff/1033/46#newcode161 >> > Line 161: // Returns true if the blacklist object is in good health. >> > Could be further shortened to: >> > "Is the blacklist object in good health" >> > >> > But, in the end, do you really need that? See .cc comment. > >> As I just said, it is needed to break the dependencies so that we >> can show the user when it does not load. It is extremely important >> that we do so because otherwise users would have a false impression >> about how their privacy is protected. > > My point is roughly that 'good' has different meaning. You probably want > to know if the list is 'valid()' or not. But even though, if the list is > not valid, what's the deal? Frankly, I'd want the point of view of > someone else on that since I'm not a fan of surfacing too many errors. > > >> > http://codereview.chromium.org/173357/diff/1033/42 >> > File chrome/browser/privacy_blacklist/blacklist_io.h (right): >> > >> > http://codereview.chromium.org/173357/diff/1033/42#newcode30 >> > Line 30: const string16& last_error() const { >> > nit: I think this design would badly degrade in failure mode. > > Imagine an >> >> > overflow of blacklisted IO, that would significantly increase the > > memory usage >> >> > if the string is large. Especially that it would always be the same > > string. >> >> > Also, I think it's too deep to keep localized data. > >> Not sure I understand but you should know that this I/O code is only > > invoked >> >> by the extension installation code which Pawel is writing. Chrome > > would not >> >> check these once the the blacklists are installed and compiled. > > ok, but if you look at the rest of the chrome code, we usually don't > surface low level errors like "file read failed" or things like that. We don't except for developer messages I was told. That is why these messages are not i18n. It is mostly for the ones developing and validating the blacklists. Pawel is writing the front-end code that uses this. >> > http://codereview.chromium.org/173357/diff/1033/43 >> > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): >> > >> > http://codereview.chromium.org/173357/diff/1033/43#newcode126 >> > Line 126: if (uint32 n = ReadUInt()) { >> > Shouldn't you put an arbitrary upper limit for a valid supported > > range? > >> I did not see a point to do anything arbitrary, but I can if you > > really >> >> want me too. > > Simplest path to DoS. All your cute error messages won't do a thing > about the crash that will occur when you try to load an invalid file > that simply gobs all the available memory. OK. That's something new! - Itai > M-A > > http://codereview.chromium.org/173357 >
Hi Itai, Can you send Glen and I a screenshot of your dialog box, and explain when it appears? Thanks! -Ben
Please do not check in "Your privacy is no longer protected." It overstates the problem. For now, "Privacy blacklist failed to load" will do. I don't understand what cancel does in this case though. On Wed, Aug 26, 2009 at 10:25 AM, Itai Danan <idanan@google.com> wrote: > > On Wed, Aug 26, 2009 at 10:14, <maruel@chromium.org> wrote: > > On 2009/08/26 16:46:07, idanan wrote: > >> > >> On 2009/08/26 13:53:40, Marc-Antoine Ruel wrote: > >> > http://codereview.chromium.org/173357/diff/1033/48 > >> > File chrome/app/generated_resources.grd (right): > >> > > >> > http://codereview.chromium.org/173357/diff/1033/48#newcode4830 > >> > Line 4830: Your privacy is no longer protected > >> > I still think the messaging is wrong here. > > > >> What would you prefer? I think this gives exactly the consequence > >> of the error, as opposed to something like "Error loading Privacy > >> Blacklist". > > > > To me, the failure to load the privacy blacklist is *AT MOST* a toolbar > > popup, like the 'do you want to save password' or 'set chrome as default > > browser'. If I want to browse the web *now* I probably don't care that > > the privacy blacklist failed to load, and I want even less be scared by > > a message telling me that my privacy is suddenly invaded by obscure > > stuff. The information in this message is at best misleading. I > > definitely want the point of view of someone in the UI team for that. > > > > In any case, the privacy list loading should be asynchronous and non > > blocking, so even if the first socket connections starts without the > > list being fully loaded, I probably wouldn't care. > > > > Since the list loading should probably be asynchronous, this doesn't > > deserve a synchronous popup. > > Sorry but I strongly disagree here. Every access must be checked against > the blacklist, we can't load it later because if users don't want to > send cookies > or something, they don't want us to send them while we wait for the > blacklist > to be loaded. If the blacklist fails to load, which should be a rare > occurrence > because we are talking about the compiled blacklist here (disk corruption > or incorrect command line are the two possibilities), the user must know > and > acknowledge that their privacy is not protected the way they chose by > installing > their blacklists. If you don't care about privacy and have no > blacklists, none of > this will every show. > > >> > http://codereview.chromium.org/173357/diff/1033/45 > >> > File chrome/browser/privacy_blacklist/blacklist.cc (right): > >> > > >> > http://codereview.chromium.org/173357/diff/1033/45#newcode66 > >> > Line 66: bool Blacklist::Matches(const std::string& pattern, const > >> std::string& > >> > url) { > >> > Shouldn't you add DCHECK(is_good()); in a few functions? > >> I could but is_good is only designed to be checked once, upon > > > > initialization. > >> > >> The blacklist never goes from good to bad. > > > > You know that. Does the function callers in 2 years will remember that? > > OK, fair. > > >> > http://codereview.chromium.org/173357/diff/1033/45#newcode163 > >> > Line 163: Blacklist::Blacklist(const FilePath& file) : > > > > is_good_(false) { > >> > >> > Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't > > > > the list > >> > >> just > >> > be empty when the list failed to load? That's fine to have a signal > > > > to verify > >> > >> > that but you don't cleanup providers_ when it fails latter. > > > >> No because it only needs to be checked once. The reason there is even > > > > an > >> > >> is good function is because of dependencies which meant I could not do > > > >> the UI dialog right in the blacklist. > > > >> > http://codereview.chromium.org/173357/diff/1033/46 > >> > File chrome/browser/privacy_blacklist/blacklist.h (right): > >> > > >> > http://codereview.chromium.org/173357/diff/1033/46#newcode161 > >> > Line 161: // Returns true if the blacklist object is in good health. > >> > Could be further shortened to: > >> > "Is the blacklist object in good health" > >> > > >> > But, in the end, do you really need that? See .cc comment. > > > >> As I just said, it is needed to break the dependencies so that we > >> can show the user when it does not load. It is extremely important > >> that we do so because otherwise users would have a false impression > >> about how their privacy is protected. > > > > My point is roughly that 'good' has different meaning. You probably want > > to know if the list is 'valid()' or not. But even though, if the list is > > not valid, what's the deal? Frankly, I'd want the point of view of > > someone else on that since I'm not a fan of surfacing too many errors. > > > > > >> > http://codereview.chromium.org/173357/diff/1033/42 > >> > File chrome/browser/privacy_blacklist/blacklist_io.h (right): > >> > > >> > http://codereview.chromium.org/173357/diff/1033/42#newcode30 > >> > Line 30: const string16& last_error() const { > >> > nit: I think this design would badly degrade in failure mode. > > > > Imagine an > >> > >> > overflow of blacklisted IO, that would significantly increase the > > > > memory usage > >> > >> > if the string is large. Especially that it would always be the same > > > > string. > >> > >> > Also, I think it's too deep to keep localized data. > > > >> Not sure I understand but you should know that this I/O code is only > > > > invoked > >> > >> by the extension installation code which Pawel is writing. Chrome > > > > would not > >> > >> check these once the the blacklists are installed and compiled. > > > > ok, but if you look at the rest of the chrome code, we usually don't > > surface low level errors like "file read failed" or things like that. > > We don't except for developer messages I was told. That is why these > messages > are not i18n. It is mostly for the ones developing and validating the > blacklists. Pawel > is writing the front-end code that uses this. > > >> > http://codereview.chromium.org/173357/diff/1033/43 > >> > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): > >> > > >> > http://codereview.chromium.org/173357/diff/1033/43#newcode126 > >> > Line 126: if (uint32 n = ReadUInt()) { > >> > Shouldn't you put an arbitrary upper limit for a valid supported > > > > range? > > > >> I did not see a point to do anything arbitrary, but I can if you > > > > really > >> > >> want me too. > > > > Simplest path to DoS. All your cute error messages won't do a thing > > about the crash that will occur when you try to load an invalid file > > that simply gobs all the available memory. > > OK. That's something new! > > - Itai > > > M-A > > > > http://codereview.chromium.org/173357 > > >
OK for the error message. Cancel lets you not load Chrome and fix the problem, just like when the user profile fails to load (in case you moved the files, changed permissions too aggressively, want to restore a backup, etc). Those labels could be Continue (instead of OK) and Exit (instead of Cancel). It might be a bit clearer but even checked-in, it is not carved in stone. We will have time to refine this several times before we enable it for everyone. - Itai On Thu, Aug 27, 2009 at 14:50, Brian Rakowski<brian@chromium.org> wrote: > Please do not check in "Your privacy is no longer protected." It overstates > the problem. > For now, "Privacy blacklist failed to load" will do. I don't understand what > cancel does in this case though. > > On Wed, Aug 26, 2009 at 10:25 AM, Itai Danan <idanan@google.com> wrote: >> >> On Wed, Aug 26, 2009 at 10:14, <maruel@chromium.org> wrote: >> > On 2009/08/26 16:46:07, idanan wrote: >> >> >> >> On 2009/08/26 13:53:40, Marc-Antoine Ruel wrote: >> >> > http://codereview.chromium.org/173357/diff/1033/48 >> >> > File chrome/app/generated_resources.grd (right): >> >> > >> >> > http://codereview.chromium.org/173357/diff/1033/48#newcode4830 >> >> > Line 4830: Your privacy is no longer protected >> >> > I still think the messaging is wrong here. >> > >> >> What would you prefer? I think this gives exactly the consequence >> >> of the error, as opposed to something like "Error loading Privacy >> >> Blacklist". >> > >> > To me, the failure to load the privacy blacklist is *AT MOST* a toolbar >> > popup, like the 'do you want to save password' or 'set chrome as default >> > browser'. If I want to browse the web *now* I probably don't care that >> > the privacy blacklist failed to load, and I want even less be scared by >> > a message telling me that my privacy is suddenly invaded by obscure >> > stuff. The information in this message is at best misleading. I >> > definitely want the point of view of someone in the UI team for that. >> > >> > In any case, the privacy list loading should be asynchronous and non >> > blocking, so even if the first socket connections starts without the >> > list being fully loaded, I probably wouldn't care. >> > >> > Since the list loading should probably be asynchronous, this doesn't >> > deserve a synchronous popup. >> >> Sorry but I strongly disagree here. Every access must be checked against >> the blacklist, we can't load it later because if users don't want to >> send cookies >> or something, they don't want us to send them while we wait for the >> blacklist >> to be loaded. If the blacklist fails to load, which should be a rare >> occurrence >> because we are talking about the compiled blacklist here (disk corruption >> or incorrect command line are the two possibilities), the user must know >> and >> acknowledge that their privacy is not protected the way they chose by >> installing >> their blacklists. If you don't care about privacy and have no >> blacklists, none of >> this will every show. >> >> >> > http://codereview.chromium.org/173357/diff/1033/45 >> >> > File chrome/browser/privacy_blacklist/blacklist.cc (right): >> >> > >> >> > http://codereview.chromium.org/173357/diff/1033/45#newcode66 >> >> > Line 66: bool Blacklist::Matches(const std::string& pattern, const >> >> std::string& >> >> > url) { >> >> > Shouldn't you add DCHECK(is_good()); in a few functions? >> >> I could but is_good is only designed to be checked once, upon >> > >> > initialization. >> >> >> >> The blacklist never goes from good to bad. >> > >> > You know that. Does the function callers in 2 years will remember that? >> >> OK, fair. >> >> >> > http://codereview.chromium.org/173357/diff/1033/45#newcode163 >> >> > Line 163: Blacklist::Blacklist(const FilePath& file) : >> > >> > is_good_(false) { >> >> >> >> > Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't >> > >> > the list >> >> >> >> just >> >> > be empty when the list failed to load? That's fine to have a signal >> > >> > to verify >> >> >> >> > that but you don't cleanup providers_ when it fails latter. >> > >> >> No because it only needs to be checked once. The reason there is even >> > >> > an >> >> >> >> is good function is because of dependencies which meant I could not do >> > >> >> the UI dialog right in the blacklist. >> > >> >> > http://codereview.chromium.org/173357/diff/1033/46 >> >> > File chrome/browser/privacy_blacklist/blacklist.h (right): >> >> > >> >> > http://codereview.chromium.org/173357/diff/1033/46#newcode161 >> >> > Line 161: // Returns true if the blacklist object is in good health. >> >> > Could be further shortened to: >> >> > "Is the blacklist object in good health" >> >> > >> >> > But, in the end, do you really need that? See .cc comment. >> > >> >> As I just said, it is needed to break the dependencies so that we >> >> can show the user when it does not load. It is extremely important >> >> that we do so because otherwise users would have a false impression >> >> about how their privacy is protected. >> > >> > My point is roughly that 'good' has different meaning. You probably want >> > to know if the list is 'valid()' or not. But even though, if the list is >> > not valid, what's the deal? Frankly, I'd want the point of view of >> > someone else on that since I'm not a fan of surfacing too many errors. >> > >> > >> >> > http://codereview.chromium.org/173357/diff/1033/42 >> >> > File chrome/browser/privacy_blacklist/blacklist_io.h (right): >> >> > >> >> > http://codereview.chromium.org/173357/diff/1033/42#newcode30 >> >> > Line 30: const string16& last_error() const { >> >> > nit: I think this design would badly degrade in failure mode. >> > >> > Imagine an >> >> >> >> > overflow of blacklisted IO, that would significantly increase the >> > >> > memory usage >> >> >> >> > if the string is large. Especially that it would always be the same >> > >> > string. >> >> >> >> > Also, I think it's too deep to keep localized data. >> > >> >> Not sure I understand but you should know that this I/O code is only >> > >> > invoked >> >> >> >> by the extension installation code which Pawel is writing. Chrome >> > >> > would not >> >> >> >> check these once the the blacklists are installed and compiled. >> > >> > ok, but if you look at the rest of the chrome code, we usually don't >> > surface low level errors like "file read failed" or things like that. >> >> We don't except for developer messages I was told. That is why these >> messages >> are not i18n. It is mostly for the ones developing and validating the >> blacklists. Pawel >> is writing the front-end code that uses this. >> >> >> > http://codereview.chromium.org/173357/diff/1033/43 >> >> > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): >> >> > >> >> > http://codereview.chromium.org/173357/diff/1033/43#newcode126 >> >> > Line 126: if (uint32 n = ReadUInt()) { >> >> > Shouldn't you put an arbitrary upper limit for a valid supported >> > >> > range? >> > >> >> I did not see a point to do anything arbitrary, but I can if you >> > >> > really >> >> >> >> want me too. >> > >> > Simplest path to DoS. All your cute error messages won't do a thing >> > about the crash that will occur when you try to load an invalid file >> > that simply gobs all the available memory. >> >> OK. That's something new! >> >> - Itai >> >> > M-A >> > >> > http://codereview.chromium.org/173357 >> > > >
As long as this UI is behind a flag that we don't tell that many people about, I'm OK with including it for now. I'd rather we have Continue / Exit, though. On Thu, Aug 27, 2009 at 3:00 PM, Itai Danan <idanan@google.com> wrote: > OK for the error message. > > Cancel lets you not load Chrome and fix the problem, just like when > the user profile fails to load > (in case you moved the files, changed permissions too aggressively, > want to restore a backup, > etc). > > Those labels could be Continue (instead of OK) and Exit (instead of > Cancel). It might be a bit > clearer but even checked-in, it is not carved in stone. We will have > time to refine this several > times before we enable it for everyone. > > - Itai > > On Thu, Aug 27, 2009 at 14:50, Brian Rakowski<brian@chromium.org> wrote: >> Please do not check in "Your privacy is no longer protected." It overstates >> the problem. >> For now, "Privacy blacklist failed to load" will do. I don't understand what >> cancel does in this case though. >> >> On Wed, Aug 26, 2009 at 10:25 AM, Itai Danan <idanan@google.com> wrote: >>> >>> On Wed, Aug 26, 2009 at 10:14, <maruel@chromium.org> wrote: >>> > On 2009/08/26 16:46:07, idanan wrote: >>> >> >>> >> On 2009/08/26 13:53:40, Marc-Antoine Ruel wrote: >>> >> > http://codereview.chromium.org/173357/diff/1033/48 >>> >> > File chrome/app/generated_resources.grd (right): >>> >> > >>> >> > http://codereview.chromium.org/173357/diff/1033/48#newcode4830 >>> >> > Line 4830: Your privacy is no longer protected >>> >> > I still think the messaging is wrong here. >>> > >>> >> What would you prefer? I think this gives exactly the consequence >>> >> of the error, as opposed to something like "Error loading Privacy >>> >> Blacklist". >>> > >>> > To me, the failure to load the privacy blacklist is *AT MOST* a toolbar >>> > popup, like the 'do you want to save password' or 'set chrome as default >>> > browser'. If I want to browse the web *now* I probably don't care that >>> > the privacy blacklist failed to load, and I want even less be scared by >>> > a message telling me that my privacy is suddenly invaded by obscure >>> > stuff. The information in this message is at best misleading. I >>> > definitely want the point of view of someone in the UI team for that. >>> > >>> > In any case, the privacy list loading should be asynchronous and non >>> > blocking, so even if the first socket connections starts without the >>> > list being fully loaded, I probably wouldn't care. >>> > >>> > Since the list loading should probably be asynchronous, this doesn't >>> > deserve a synchronous popup. >>> >>> Sorry but I strongly disagree here. Every access must be checked against >>> the blacklist, we can't load it later because if users don't want to >>> send cookies >>> or something, they don't want us to send them while we wait for the >>> blacklist >>> to be loaded. If the blacklist fails to load, which should be a rare >>> occurrence >>> because we are talking about the compiled blacklist here (disk corruption >>> or incorrect command line are the two possibilities), the user must know >>> and >>> acknowledge that their privacy is not protected the way they chose by >>> installing >>> their blacklists. If you don't care about privacy and have no >>> blacklists, none of >>> this will every show. >>> >>> >> > http://codereview.chromium.org/173357/diff/1033/45 >>> >> > File chrome/browser/privacy_blacklist/blacklist.cc (right): >>> >> > >>> >> > http://codereview.chromium.org/173357/diff/1033/45#newcode66 >>> >> > Line 66: bool Blacklist::Matches(const std::string& pattern, const >>> >> std::string& >>> >> > url) { >>> >> > Shouldn't you add DCHECK(is_good()); in a few functions? >>> >> I could but is_good is only designed to be checked once, upon >>> > >>> > initialization. >>> >> >>> >> The blacklist never goes from good to bad. >>> > >>> > You know that. Does the function callers in 2 years will remember that? >>> >>> OK, fair. >>> >>> >> > http://codereview.chromium.org/173357/diff/1033/45#newcode163 >>> >> > Line 163: Blacklist::Blacklist(const FilePath& file) : >>> > >>> > is_good_(false) { >>> >> >>> >> > Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't >>> > >>> > the list >>> >> >>> >> just >>> >> > be empty when the list failed to load? That's fine to have a signal >>> > >>> > to verify >>> >> >>> >> > that but you don't cleanup providers_ when it fails latter. >>> > >>> >> No because it only needs to be checked once. The reason there is even >>> > >>> > an >>> >> >>> >> is good function is because of dependencies which meant I could not do >>> > >>> >> the UI dialog right in the blacklist. >>> > >>> >> > http://codereview.chromium.org/173357/diff/1033/46 >>> >> > File chrome/browser/privacy_blacklist/blacklist.h (right): >>> >> > >>> >> > http://codereview.chromium.org/173357/diff/1033/46#newcode161 >>> >> > Line 161: // Returns true if the blacklist object is in good health. >>> >> > Could be further shortened to: >>> >> > "Is the blacklist object in good health" >>> >> > >>> >> > But, in the end, do you really need that? See .cc comment. >>> > >>> >> As I just said, it is needed to break the dependencies so that we >>> >> can show the user when it does not load. It is extremely important >>> >> that we do so because otherwise users would have a false impression >>> >> about how their privacy is protected. >>> > >>> > My point is roughly that 'good' has different meaning. You probably want >>> > to know if the list is 'valid()' or not. But even though, if the list is >>> > not valid, what's the deal? Frankly, I'd want the point of view of >>> > someone else on that since I'm not a fan of surfacing too many errors. >>> > >>> > >>> >> > http://codereview.chromium.org/173357/diff/1033/42 >>> >> > File chrome/browser/privacy_blacklist/blacklist_io.h (right): >>> >> > >>> >> > http://codereview.chromium.org/173357/diff/1033/42#newcode30 >>> >> > Line 30: const string16& last_error() const { >>> >> > nit: I think this design would badly degrade in failure mode. >>> > >>> > Imagine an >>> >> >>> >> > overflow of blacklisted IO, that would significantly increase the >>> > >>> > memory usage >>> >> >>> >> > if the string is large. Especially that it would always be the same >>> > >>> > string. >>> >> >>> >> > Also, I think it's too deep to keep localized data. >>> > >>> >> Not sure I understand but you should know that this I/O code is only >>> > >>> > invoked >>> >> >>> >> by the extension installation code which Pawel is writing. Chrome >>> > >>> > would not >>> >> >>> >> check these once the the blacklists are installed and compiled. >>> > >>> > ok, but if you look at the rest of the chrome code, we usually don't >>> > surface low level errors like "file read failed" or things like that. >>> >>> We don't except for developer messages I was told. That is why these >>> messages >>> are not i18n. It is mostly for the ones developing and validating the >>> blacklists. Pawel >>> is writing the front-end code that uses this. >>> >>> >> > http://codereview.chromium.org/173357/diff/1033/43 >>> >> > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): >>> >> > >>> >> > http://codereview.chromium.org/173357/diff/1033/43#newcode126 >>> >> > Line 126: if (uint32 n = ReadUInt()) { >>> >> > Shouldn't you put an arbitrary upper limit for a valid supported >>> > >>> > range? >>> > >>> >> I did not see a point to do anything arbitrary, but I can if you >>> > >>> > really >>> >> >>> >> want me too. >>> > >>> > Simplest path to DoS. All your cute error messages won't do a thing >>> > about the crash that will occur when you try to load an invalid file >>> > that simply gobs all the available memory. >>> >>> OK. That's something new! >>> >>> - Itai >>> >>> > M-A >>> > >>> > http://codereview.chromium.org/173357 >>> > >> >> > -- Reminder: I will be on vacation from September 17 - 30, and will probably be out of internet range (Egypt) until the 27th (Paris).
Great thanks. I've renamed the buttons (and i18n'd them) to Continue exit and I will put in the patch shortly. - Itai PS: For some reason I did not get your reply to this in my inbox, all other ones made it, so I just found it. On 2009/09/01 22:27:11, Glen Murphy wrote: > As long as this UI is behind a flag that we don't tell that many > people about, I'm OK with including it for now. I'd rather we have > Continue / Exit, though. > > > > On Thu, Aug 27, 2009 at 3:00 PM, Itai Danan <mailto:idanan@google.com> wrote: > > OK for the error message. > > > > Cancel lets you not load Chrome and fix the problem, just like when > > the user profile fails to load > > (in case you moved the files, changed permissions too aggressively, > > want to restore a backup, > > etc). > > > > Those labels could be Continue (instead of OK) and Exit (instead of > > Cancel). It might be a bit > > clearer but even checked-in, it is not carved in stone. We will have > > time to refine this several > > times before we enable it for everyone. > > > > - Itai > > > > On Thu, Aug 27, 2009 at 14:50, Brian mailto:Rakowski<brian@chromium.org> wrote: > >> Please do not check in "Your privacy is no longer protected." It overstates > >> the problem. > >> For now, "Privacy blacklist failed to load" will do. I don't understand what > >> cancel does in this case though. > >> > >> On Wed, Aug 26, 2009 at 10:25 AM, Itai Danan <mailto:idanan@google.com> wrote: > >>> > >>> On Wed, Aug 26, 2009 at 10:14, <mailto:maruel@chromium.org> wrote: > >>> > On 2009/08/26 16:46:07, idanan wrote: > >>> >> > >>> >> On 2009/08/26 13:53:40, Marc-Antoine Ruel wrote: > >>> >> > http://codereview.chromium.org/173357/diff/1033/48 > >>> >> > File chrome/app/generated_resources.grd (right): > >>> >> > > >>> >> > http://codereview.chromium.org/173357/diff/1033/48#newcode4830 > >>> >> > Line 4830: Your privacy is no longer protected > >>> >> > I still think the messaging is wrong here. > >>> > > >>> >> What would you prefer? I think this gives exactly the consequence > >>> >> of the error, as opposed to something like "Error loading Privacy > >>> >> Blacklist". > >>> > > >>> > To me, the failure to load the privacy blacklist is *AT MOST* a toolbar > >>> > popup, like the 'do you want to save password' or 'set chrome as default > >>> > browser'. If I want to browse the web *now* I probably don't care that > >>> > the privacy blacklist failed to load, and I want even less be scared by > >>> > a message telling me that my privacy is suddenly invaded by obscure > >>> > stuff. The information in this message is at best misleading. I > >>> > definitely want the point of view of someone in the UI team for that. > >>> > > >>> > In any case, the privacy list loading should be asynchronous and non > >>> > blocking, so even if the first socket connections starts without the > >>> > list being fully loaded, I probably wouldn't care. > >>> > > >>> > Since the list loading should probably be asynchronous, this doesn't > >>> > deserve a synchronous popup. > >>> > >>> Sorry but I strongly disagree here. Every access must be checked against > >>> the blacklist, we can't load it later because if users don't want to > >>> send cookies > >>> or something, they don't want us to send them while we wait for the > >>> blacklist > >>> to be loaded. If the blacklist fails to load, which should be a rare > >>> occurrence > >>> because we are talking about the compiled blacklist here (disk corruption > >>> or incorrect command line are the two possibilities), the user must know > >>> and > >>> acknowledge that their privacy is not protected the way they chose by > >>> installing > >>> their blacklists. If you don't care about privacy and have no > >>> blacklists, none of > >>> this will every show. > >>> > >>> >> > http://codereview.chromium.org/173357/diff/1033/45 > >>> >> > File chrome/browser/privacy_blacklist/blacklist.cc (right): > >>> >> > > >>> >> > http://codereview.chromium.org/173357/diff/1033/45#newcode66 > >>> >> > Line 66: bool Blacklist::Matches(const std::string& pattern, const > >>> >> std::string& > >>> >> > url) { > >>> >> > Shouldn't you add DCHECK(is_good()); in a few functions? > >>> >> I could but is_good is only designed to be checked once, upon > >>> > > >>> > initialization. > >>> >> > >>> >> The blacklist never goes from good to bad. > >>> > > >>> > You know that. Does the function callers in 2 years will remember that? > >>> > >>> OK, fair. > >>> > >>> >> > http://codereview.chromium.org/173357/diff/1033/45#newcode163 > >>> >> > Line 163: Blacklist::Blacklist(const FilePath& file) : > >>> > > >>> > is_good_(false) { > >>> >> > >>> >> > Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't > >>> > > >>> > the list > >>> >> > >>> >> just > >>> >> > be empty when the list failed to load? That's fine to have a signal > >>> > > >>> > to verify > >>> >> > >>> >> > that but you don't cleanup providers_ when it fails latter. > >>> > > >>> >> No because it only needs to be checked once. The reason there is even > >>> > > >>> > an > >>> >> > >>> >> is good function is because of dependencies which meant I could not do > >>> > > >>> >> the UI dialog right in the blacklist. > >>> > > >>> >> > http://codereview.chromium.org/173357/diff/1033/46 > >>> >> > File chrome/browser/privacy_blacklist/blacklist.h (right): > >>> >> > > >>> >> > http://codereview.chromium.org/173357/diff/1033/46#newcode161 > >>> >> > Line 161: // Returns true if the blacklist object is in good health. > >>> >> > Could be further shortened to: > >>> >> > "Is the blacklist object in good health" > >>> >> > > >>> >> > But, in the end, do you really need that? See .cc comment. > >>> > > >>> >> As I just said, it is needed to break the dependencies so that we > >>> >> can show the user when it does not load. It is extremely important > >>> >> that we do so because otherwise users would have a false impression > >>> >> about how their privacy is protected. > >>> > > >>> > My point is roughly that 'good' has different meaning. You probably want > >>> > to know if the list is 'valid()' or not. But even though, if the list is > >>> > not valid, what's the deal? Frankly, I'd want the point of view of > >>> > someone else on that since I'm not a fan of surfacing too many errors. > >>> > > >>> > > >>> >> > http://codereview.chromium.org/173357/diff/1033/42 > >>> >> > File chrome/browser/privacy_blacklist/blacklist_io.h (right): > >>> >> > > >>> >> > http://codereview.chromium.org/173357/diff/1033/42#newcode30 > >>> >> > Line 30: const string16& last_error() const { > >>> >> > nit: I think this design would badly degrade in failure mode. > >>> > > >>> > Imagine an > >>> >> > >>> >> > overflow of blacklisted IO, that would significantly increase the > >>> > > >>> > memory usage > >>> >> > >>> >> > if the string is large. Especially that it would always be the same > >>> > > >>> > string. > >>> >> > >>> >> > Also, I think it's too deep to keep localized data. > >>> > > >>> >> Not sure I understand but you should know that this I/O code is only > >>> > > >>> > invoked > >>> >> > >>> >> by the extension installation code which Pawel is writing. Chrome > >>> > > >>> > would not > >>> >> > >>> >> check these once the the blacklists are installed and compiled. > >>> > > >>> > ok, but if you look at the rest of the chrome code, we usually don't > >>> > surface low level errors like "file read failed" or things like that. > >>> > >>> We don't except for developer messages I was told. That is why these > >>> messages > >>> are not i18n. It is mostly for the ones developing and validating the > >>> blacklists. Pawel > >>> is writing the front-end code that uses this. > >>> > >>> >> > http://codereview.chromium.org/173357/diff/1033/43 > >>> >> > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): > >>> >> > > >>> >> > http://codereview.chromium.org/173357/diff/1033/43#newcode126 > >>> >> > Line 126: if (uint32 n = ReadUInt()) { > >>> >> > Shouldn't you put an arbitrary upper limit for a valid supported > >>> > > >>> > range? > >>> > > >>> >> I did not see a point to do anything arbitrary, but I can if you > >>> > > >>> > really > >>> >> > >>> >> want me too. > >>> > > >>> > Simplest path to DoS. All your cute error messages won't do a thing > >>> > about the crash that will occur when you try to load an invalid file > >>> > that simply gobs all the available memory. > >>> > >>> OK. That's something new! > >>> > >>> - Itai > >>> > >>> > M-A > >>> > > >>> > http://codereview.chromium.org/173357 > >>> > > >> > >> > > > > > > -- > Reminder: I will be on vacation from September 17 - 30, and will > probably be out of internet range (Egypt) until the 27th (Paris).
On 2009/08/26 17:14:08, Marc-Antoine Ruel wrote: > On 2009/08/26 16:46:07, idanan wrote: > > On 2009/08/26 13:53:40, Marc-Antoine Ruel wrote: > > > http://codereview.chromium.org/173357/diff/1033/48 > > > File chrome/app/generated_resources.grd (right): > > > > > > http://codereview.chromium.org/173357/diff/1033/48#newcode4830 > > > Line 4830: Your privacy is no longer protected > > > I still think the messaging is wrong here. > > > > What would you prefer? I think this gives exactly the consequence > > of the error, as opposed to something like "Error loading Privacy > > Blacklist". > > To me, the failure to load the privacy blacklist is *AT MOST* a toolbar popup, > like the 'do you want to save password' or 'set chrome as default browser'. If I > want to browse the web *now* I probably don't care that the privacy blacklist > failed to load, and I want even less be scared by a message telling me that my > privacy is suddenly invaded by obscure stuff. The information in this message is > at best misleading. I definitely want the point of view of someone in the UI > team for that. > > In any case, the privacy list loading should be asynchronous and non blocking, > so even if the first socket connections starts without the list being fully > loaded, I probably wouldn't care. > > Since the list loading should probably be asynchronous, this doesn't deserve a > synchronous popup. > Modified according to Glen's suggestions. > > > http://codereview.chromium.org/173357/diff/1033/45 > > > File chrome/browser/privacy_blacklist/blacklist.cc (right): > > > > > > http://codereview.chromium.org/173357/diff/1033/45#newcode66 > > > Line 66: bool Blacklist::Matches(const std::string& pattern, const > > std::string& > > > url) { > > > Shouldn't you add DCHECK(is_good()); in a few functions? > > I could but is_good is only designed to be checked once, upon initialization. > > The blacklist never goes from good to bad. > > You know that. Does the function callers in 2 years will remember that? OK. Actually had to make that an if, for the case were users use "Continue". > > > http://codereview.chromium.org/173357/diff/1033/45#newcode163 > > > Line 163: Blacklist::Blacklist(const FilePath& file) : is_good_(false) { > > > Doesn't is_good_ puts unnecessary burden on the callers? Shouldn't the list > > just > > > be empty when the list failed to load? That's fine to have a signal to > verify > > > that but you don't cleanup providers_ when it fails latter. > > > > No because it only needs to be checked once. The reason there is even an > > is good function is because of dependencies which meant I could not do > > the UI dialog right in the blacklist. > > > > > http://codereview.chromium.org/173357/diff/1033/46 > > > File chrome/browser/privacy_blacklist/blacklist.h (right): > > > > > > http://codereview.chromium.org/173357/diff/1033/46#newcode161 > > > Line 161: // Returns true if the blacklist object is in good health. > > > Could be further shortened to: > > > "Is the blacklist object in good health" > > > > > > But, in the end, do you really need that? See .cc comment. > > > > As I just said, it is needed to break the dependencies so that we > > can show the user when it does not load. It is extremely important > > that we do so because otherwise users would have a false impression > > about how their privacy is protected. > > My point is roughly that 'good' has different meaning. You probably want to know > if the list is 'valid()' or not. But even though, if the list is not valid, > what's the deal? Frankly, I'd want the point of view of someone else on that > since I'm not a fan of surfacing too many errors. > > > > > http://codereview.chromium.org/173357/diff/1033/42 > > > File chrome/browser/privacy_blacklist/blacklist_io.h (right): > > > > > > http://codereview.chromium.org/173357/diff/1033/42#newcode30 > > > Line 30: const string16& last_error() const { > > > nit: I think this design would badly degrade in failure mode. Imagine an > > > overflow of blacklisted IO, that would significantly increase the memory > usage > > > if the string is large. Especially that it would always be the same string. > > > Also, I think it's too deep to keep localized data. > > > > Not sure I understand but you should know that this I/O code is only invoked > > by the extension installation code which Pawel is writing. Chrome would not > > check these once the the blacklists are installed and compiled. > > ok, but if you look at the rest of the chrome code, we usually don't surface low > level errors like "file read failed" or things like that. > > > > > http://codereview.chromium.org/173357/diff/1033/43 > > > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): > > > > > > http://codereview.chromium.org/173357/diff/1033/43#newcode126 > > > Line 126: if (uint32 n = ReadUInt()) { > > > Shouldn't you put an arbitrary upper limit for a valid supported range? > > > > I did not see a point to do anything arbitrary, but I can if you really > > want me too. > > Simplest path to DoS. All your cute error messages won't do a thing about the > crash that will occur when you try to load an invalid file that simply gobs all > the available memory. Done.
lgtm for my side http://codereview.chromium.org/173357/diff/4005/5021 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/173357/diff/4005/5021#newcode4960 Line 4960: Your privacy blacklists failed to load. Is 'Your' necessary? http://codereview.chromium.org/173357/diff/4005/5016 File chrome/browser/privacy_blacklist/blacklist_store.cc (right): http://codereview.chromium.org/173357/diff/4005/5016#newcode16 Line 16: const char cookie[] = "GCPBL100"; const char* const cookie = "..";
On Thu, Sep 10, 2009 at 14:59, <maruel@chromium.org> wrote: > lgtm for my side > > > http://codereview.chromium.org/173357/diff/4005/5021 > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/173357/diff/4005/5021#newcode4960 > Line 4960: Your privacy blacklists failed to load. > Is 'Your' necessary? I'll let the experts find better messages in a future date. I prefer the your in there, it does not sound so good without it. > http://codereview.chromium.org/173357/diff/4005/5016 > File chrome/browser/privacy_blacklist/blacklist_store.cc (right): > > http://codereview.chromium.org/173357/diff/4005/5016#newcode16 > Line 16: const char cookie[] = "GCPBL100"; > const char* const cookie = ".."; Can't. Using sizeof. - Itai |
