Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1528963002:
Quirks Client for downloading and providing display profiles (Closed)
|
Created:
5 years ago by Greg Levin Modified:
4 years, 9 months ago CC:
chromium-reviews, kalyank, oshima+watch_chromium.org, sadrul, davemoore+watch_chromium.org, asvitkine+watch_chromium.org, tbuckley, abodeha_chromium.org, stevenjb Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreating a "Quirks Client" to download icc files and other display info
("quirks") from a Quirks Server. These provide display-specific tuning
(e.g. gamma ramps) on a per-monitor basis.
Each Quirks Client handles downloading and writing a single file. The
single Quirks Manager handles external requests for file paths, creates
clients when downloads are needed, and manages their life cycle. For
more info, see go/cros-quirks-client-dd (particularly the "Code Design"
section).
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server, check
that file is downloaded to /var/cache/display_profiles. At next startup,
the gamma correction in the icc file should be applied to the display
(this will only be visible to the degree that the gamma correction is
large enough to be noticeable; the correct functioning of the Quirks
Client is primarily determined by the appearance of the file).
Committed: https://crrev.com/5dd01a7d4eecdf094fad69b07040fdf9f7ce9721
Cr-Commit-Position: refs/heads/master@{#382962}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add thread checks, restore illegal includes #Patch Set 3 : Move quirks client from chromeos to components, but no longer working #
Total comments: 2
Patch Set 4 : Remove hacks for clean, but non-working, draft #
Total comments: 3
Patch Set 5 : Potentially clean and working draft #Patch Set 6 : Fix DEPS #
Total comments: 11
Patch Set 7 : Proofreading and cleanup #Patch Set 8 : Merge #Patch Set 9 : Add line to histograms.xml for flag #Patch Set 10 : Refactor manager and delegate #
Total comments: 55
Patch Set 11 : First round of review fixes, separate QCManager into separate file #
Total comments: 66
Patch Set 12 : Add browser test #Patch Set 13 : Second round of review fixes, incomplete #
Total comments: 7
Patch Set 14 : Merge #
Total comments: 1
Patch Set 15 : Fix BUILD.gn after merge #Patch Set 16 : Third round of review fixes, try to fix GN build #
Total comments: 42
Patch Set 17 : Fourth round of review fixes #
Total comments: 37
Patch Set 18 : Fifth round of review fixes (address most of bauerb's comments) #Patch Set 19 : Simplify naming of files and classes #Patch Set 20 : Refactor to move some functionality from client to manager #
Total comments: 49
Patch Set 21 : Sixth round of review fixes, sorting out thread issues #
Total comments: 2
Patch Set 22 : Refactor, clean up threading #Patch Set 23 : Fix ash_unittests #
Total comments: 34
Patch Set 24 : Seventh round of review fixes, fix GN build #
Total comments: 4
Patch Set 25 : Fix threading in delegate #Patch Set 26 : Small cleanup #
Total comments: 11
Patch Set 27 : Minor review fixes, delay download until after login #
Total comments: 4
Patch Set 28 : Latest review fixes, fix tests, remove UMA shell code #Patch Set 29 : Merge, rename flag #Patch Set 30 : Fix histograms.xml #Patch Set 31 : Fix histograms.xml #
Total comments: 14
Patch Set 32 : bauerb@'s latest code review #
Total comments: 14
Patch Set 33 : stevenjb@'s latest code review (small tweaks) #
Total comments: 5
Patch Set 34 : Small DCHECK tweak, delay downloaded icc's on internal display #
Total comments: 36
Patch Set 35 : oshima@'s latest code review #
Total comments: 4
Patch Set 36 : A few small tweaks #Messages
Total messages: 154 (52 generated)
glevin@chromium.org changed reviewers: + oshima@chromium.org
This is just a draft to facilitate discussion. It is not ready for submitting, or even thorough code review. At a minimum, it needs: 1. A refactoring, with some delegate code moved into chrome/. 2. Fixing any TODO comments I've added that don't have (glevin) attached (the ones that do are probably acceptable for submitting in a first CL). 3. Remove a few lines of code that are only for development/testing. 4. Write some unit tests. The code works on my machine, but a few lines were changed for presubmit and security reasons before I uploaded this CL, so the code doesn't quite run as seen in this draft. https://codereview.chromium.org/1528963002/diff/1/ash/display/display_color_m... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/1/ash/display/display_color_m... ash/display/display_color_manager_chromeos.cc:31: I'll move or remove this before submitting. It's just here for desktop testing for now. https://codereview.chromium.org/1528963002/diff/1/chromeos/chromeos_pref_names.h File chromeos/chromeos_pref_names.h (right): https://codereview.chromium.org/1528963002/diff/1/chromeos/chromeos_pref_name... chromeos/chromeos_pref_names.h:18: CHROMEOS_EXPORT extern const char kQuirksClientLastServerCheck[]; Should rename this to something more general, since I turned this from an int to a dictionary.
Moved code from /chromeos to /components, but it stopped working. Uploaded this draft to get help figuring out why. QuirksClient is accessed from chrome_browser_main_chromeos.cc and display_color_manager_chromeos.cc. When QuirksClient was in /chromeos, g_delegate was the same inside of both calls; after moving to /components, the two calls into QuirksClient get different g_delegate's. And it is just code location, as the hack in cras_audio_handler indicates (moved g_delegate to a random, temporary location in /chromeos, and both calls are again accessing the same global). Even with that fix, base::MessageLoopForUI::current() is not valid when the code is in /components, while it was when the code was in /chromeos. So how is code location affecting these two items?
https://codereview.chromium.org/1528963002/diff/40001/components/quirks_clien... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/40001/components/quirks_clien... components/quirks_client/quirks_client.cc:386: base::ThreadTaskRunnerHandle::IsSet(); Since moving QC to components/, each of base::MessageLoopForUI::IsCurrent() local_state_->CalledOnValidThread() base::ThreadTaskRunnerHandle::IsSet() returns false. When this code was in chromeos/, these all returned true. The locations and timing of when and where QC code is being called hasn't changed. https://codereview.chromium.org/1528963002/diff/40001/components/quirks_clien... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/40001/components/quirks_clien... components/quirks_client/quirks_client.h:31: class QuirksClient : public net::URLFetcherDelegate { Added QUIRKS_CLIENT_EXPORT to QuirksClient and QuirksClientDelegate (much like https://code.google.com/p/chromium/codesearch#chromium/src/components/bitmap_...). I haven't uploaded this change yet, but it fixed the problem with two g_delegate's (all external calls now access the same one).
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1528963002/diff/80001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/1528963002/diff/80001/ash/shell.cc#newcode871 ash/shell.cc:871: display_color_manager_->LoadCalibrationForDisplayQuirksTest(); This line launches the Quirks Client for testing from any environment. It uses LoadCalibrationForDisplayQuirksTest() and FakeDisplaySnapshot from display_color_manager_chromeos.cc. These are (I think) the only hacks in this draft of the code. https://codereview.chromium.org/1528963002/diff/80001/components/quirks_clien... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/80001/components/quirks_clien... components/quirks_client/quirks_client.cc:351: LOG(ERROR) << " *** HandleIsSet() = " << base::ThreadTaskRunnerHandle::IsSet(); These logging statements demonstrate what is currently broken. They should all be returning true.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/1528963002/diff/80001/ash/display/display_col... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/80001/ash/display/display_col... ash/display/display_color_manager_chromeos.cc:153: blocking_pool_, FROM_HERE, request, I don't understand how this was ever correct. This is calling |request| on |blocking_pool|, so: ParseDisplayProfile -> quirks_client::QuirksClient::RequestIccProfilePath -> g_delegate->Validate() Shell::blocking_pool_ != base::MessageLoopForUI, so base::MessageLoopForUI::IsCurrent() will return false.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Hey guys. Here's a working and, as far as I can tell, clean draft. There are still some TODOs, some of which will need to be dealt with before flipping the flag. Also, the linux_chromium_gn_chromeos_rel test is failing. Is this a BUILD.gn problem, and if so, is there a good reference for these files? I'm having trouble figuring out exactly how to arrange the includes for my new code. I'm working on browser tests, but might want to submit those separately so testing can begin ASAP on this CL. Finally, I'll probably need other reviewers for the various pieces, but I thought you guys could take a look first, so we could deal with any major structural problems before I bring in others and have to start changing their pieces too much. Thanks, and please let me know if you have any questions! https://codereview.chromium.org/1528963002/diff/120001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/1528963002/diff/120001/ash/ash.gyp#newcode1027 ash/ash.gyp:1027: '../ui/display/display.gyp:display_types', Remove this line. It was needed during development, but is not used by final code. https://codereview.chromium.org/1528963002/diff/120001/chromeos/chromeos_paths.h File chromeos/chromeos_paths.h (right): https://codereview.chromium.org/1528963002/diff/120001/chromeos/chromeos_path... chromeos/chromeos_paths.h:48: // are downloaded to from Quirks Server. Reword comment https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... File components/quirks_client.gypi (right): https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client.gypi:14: '../google_apis/google_apis.gyp:google_apis', Remove this line; apis code moved elsewhere. https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client.gypi:27: 'quirks_client/quirks_client.h', 'quirks_client/quirks_client_export.h', (NOTE: Appears to build fine w/o this line, but seems like it should be here anyway) https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client/quirks_client.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client/quirks_client.h:6: #define CHROMEOS_QUIRKS_CLIENT_QUIRKS_CLIENT_H_ COMPONENTS_QUIRKS_CLIENT_QUIRKS_CLIENT_H_ https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client/quirks_client.h:56: void OnURLFetchComplete(const net::URLFetcher* source) override; Make private https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... File components/quirks_client/quirks_client_export.h (right): https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client/quirks_client_export.h:6: #define COMPONENTS_QUIRKS_CLIENT_QUIRKS_CLIENT_H_ COMPONENTS_QUIRKS_CLIENT_QUIRKS_CLIENT_EXPORT_H_ https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... File components/quirks_client/switches.cc (right): https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client/switches.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... File components/quirks_client/switches.h (right): https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client/switches.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1528963002/diff/120001/components/quirks_clie... components/quirks_client/switches.h:11: // All switch in alphabetical order. The switches should be documented Should probably be "switches", even though there's only one :-P
Hey guys. Sorry for the change after the review request, but I think this makes more sense. The old "delegate" is now the "manager", and the new delegate is now a nested pure virtual class (implemented like before to provide functionality from within chrome/browser/etc). I think "manager" will be more accurate, since my eventual plan is to potentially have a client for each display, and have the manager keep a list of them.
https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:37: quirks_client::QuirksClient::DownloadFinishedCallback* callback = nullptr; do you need this variable? https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:39: // profile as soon as it's downloaded? instead, explain why we're not doing now, and why we may want to do so. https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:130: scoped_ptr<ColorCalibrationData> data(new ColorCalibrationData()); can't you do scoped_ptr<Data> ParseProfile() UpdateCalibrationData(..., scoped_ptr<Data>) instead of creating the result here and pass to both? https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_client_delegate_impl.h (right): https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 same for other files https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:16: ~QuirksClientDelegateImpl() override {} = default; https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:19: std::string api_key_quirks() override; GetApiKeyQuirks() https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:21: int GetDaysSinceOobe() override; can these be const? (just a q) https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:22: }; private: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client.gypi (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client.gypi:10: 'type': 'static_library', why not '<(component)' ? https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/DEPS (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/DEPS:5: "+net/url_request", indent https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:32: QUIRKS_CLIENT_EXPORT QuirksClientManager* g_manager = nullptr; its weird to export stuff in anonymous namespace. what is this for? you may just want to call manager (or probably quirks_client_manager) as g_ is for global. (this is file scope) https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:46: const double kMaxRetrySleepSeconds = 6 * 3600; // 6 hours why not just use hours instead of seconds? https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:49: std::string id2str(int64_t product_id) { IdToHexString https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:295: void QuirksClientManager::Initialize(QuirksClientManager* manager) { consider using scope_ptr and move https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:53: void Start(); or StartDownload() and remove comment. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:122: class QUIRKS_CLIENT_EXPORT QuirksClientManager { define this in a separate file. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:128: virtual std::string api_key_quirks() = 0; GetApiKeyQuirks() https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:130: virtual int GetDaysSinceOobe() = 0; can/should these be const?(just asking) https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:148: void SetLastServerCheck(int64_t product_id, base::Time last_check); const base::Time& https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:165: base::MessageLoopForUI* message_loop_ui_; // To run QC on ui thread. do you need this? can't you just use ::current() ?
https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:45: << " for display id: " << display_id; Since PathExists() will fail for an empty path, just return false here and elim the else. https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_client_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.cc:20: // Returns the path to the writable display profile directory. This comment should be in the header. https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.cc:23: // Either directory must be created beforehand. s/must be created beforehand/must already exist/ https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:32: QUIRKS_CLIENT_EXPORT QuirksClientManager* g_manager = nullptr; On 2016/01/28 20:16:31, oshima wrote: > its weird to export stuff in anonymous namespace. what is this for? > > you may just want to call manager (or probably quirks_client_manager) as g_ is > for global. (this is file scope) I'm a little confused as to the relationship between QuirksClient and QuirksClientManager right now. I think that splitting these into separate files will help a lot. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:56: // This is called in tests to modify (lower) retry delay. If we don't use these yet we should remove them until we do (but it would be preferable to have at least basic tests with the initial CL). https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:131: }; + private: DISALLOW_ASSIGN
Please have another look! https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:37: quirks_client::QuirksClient::DownloadFinishedCallback* callback = nullptr; On 2016/01/28 20:16:30, oshima wrote: > do you need this variable? No, was just a placeholder for the TODO comment. https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:39: // profile as soon as it's downloaded? On 2016/01/28 20:16:30, oshima wrote: > instead, explain why we're not doing now, and why we may want to do so. Done. https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:45: << " for display id: " << display_id; On 2016/01/28 21:05:58, stevenjb wrote: > Since PathExists() will fail for an empty path, just return false here and elim > the else. Done. (Also changed "if (!PathExists) return" to DCHECK, since the QC should only return a non-empty path if the file actually exists.) https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:130: scoped_ptr<ColorCalibrationData> data(new ColorCalibrationData()); On 2016/01/28 20:16:30, oshima wrote: > can't you do > > scoped_ptr<Data> ParseProfile() > > UpdateCalibrationData(..., scoped_ptr<Data>) > > instead of creating the result here and pass to both? Seems to work. (Wasn't my code, but still a good simplification.) https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_client_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.cc:20: // Returns the path to the writable display profile directory. On 2016/01/28 21:05:58, stevenjb wrote: > This comment should be in the header. Done. https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.cc:23: // Either directory must be created beforehand. On 2016/01/28 21:05:58, stevenjb wrote: > s/must be created beforehand/must already exist/ Done. https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_client_delegate_impl.h (right): https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/28 20:16:30, oshima wrote: > 2016 > > same for other files Done. https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:16: ~QuirksClientDelegateImpl() override {} On 2016/01/28 20:16:30, oshima wrote: > = default; Done. https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:19: std::string api_key_quirks() override; On 2016/01/28 20:16:30, oshima wrote: > GetApiKeyQuirks() Done. https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:21: int GetDaysSinceOobe() override; On 2016/01/28 20:16:30, oshima wrote: > can these be const? (just a q) They can be (and now are). Is this preferable, even when the class has no members to alter? https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:22: }; On 2016/01/28 20:16:30, oshima wrote: > private: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client.gypi (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client.gypi:10: 'type': 'static_library', On 2016/01/28 20:16:31, oshima wrote: > why not '<(component)' ? I'm not actually clear on the functional difference. When I switched to '<(component)', I got the following link errors: quirks_client.cc:118: error: undefined reference to 'net::URLFetcherDelegate::~URLFetcherDelegate()' quirks_client.cc:174: error: undefined reference to 'GURL::GURL(base::BasicStringPiece<std::string>)' quirks_client.cc:174: error: undefined reference to 'net::URLFetcher::Create(GURL const&, ...)' quirks_client.cc:174: error: undefined reference to 'GURL::~GURL()' Should I switch to '<(component)' and try to fix those errors? https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/DEPS (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/DEPS:5: "+net/url_request", On 2016/01/28 20:16:31, oshima wrote: > indent Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:32: QUIRKS_CLIENT_EXPORT QuirksClientManager* g_manager = nullptr; On 2016/01/28 20:16:31, oshima wrote: > its weird to export stuff in anonymous namespace. what is this for? Without this, ChromeBrowserMainPartsChromeos and DisplayColorManager (the two external accesses of QuirksClient) end up getting different instances of g_manager. Specifically, DisplayColorManager gets NULL instead of the one set up by ChromeBrowserMainPartsChromeos. > you may just want to call manager (or probably quirks_client_manager) as g_ is > for global. (this is file scope) I've seen g_ used for a lot of file-scope-global variables. Is that incorrect? https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:32: QUIRKS_CLIENT_EXPORT QuirksClientManager* g_manager = nullptr; On 2016/01/28 21:05:58, stevenjb wrote: > On 2016/01/28 20:16:31, oshima wrote: > > its weird to export stuff in anonymous namespace. what is this for? > > > > you may just want to call manager (or probably quirks_client_manager) as g_ > > is for global. (this is file scope) > > I'm a little confused as to the relationship between QuirksClient and > QuirksClientManager right now. I think that splitting these into separate > files will help a lot. QuirksClientManager was originally QuirksClientDelegate. It was basically code to allow QC to access various stuff outside of components/. But when I created the current QuirksClientManager::Delegate, with a pure virtual interface and impl code that actually needed to live outide components/, I renamed it. The current manager still holds stuff from outside components/ (pointers to msg loops, prefs, UMA stats), and also starts the client running. When I add code to support multiple monitors and have multiple clients running, manager is where I'd keep track of them. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:46: const double kMaxRetrySleepSeconds = 6 * 3600; // 6 hours On 2016/01/28 20:16:31, oshima wrote: > why not just use hours instead of seconds? In QuirksClient::Retry(), this value is compared directly with kRetrySleepSeconds (which is of second-level granularity) to compute delay_seconds. If the conversion isn't done here, it would have to be done there. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:49: std::string id2str(int64_t product_id) { On 2016/01/28 20:16:31, oshima wrote: > IdToHexString Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:295: void QuirksClientManager::Initialize(QuirksClientManager* manager) { On 2016/01/28 20:16:31, oshima wrote: > consider using scope_ptr and move I have some questions about this approach... could we discuss it in person? https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:53: void Start(); On 2016/01/28 20:16:31, oshima wrote: > or StartDownload() and remove comment. Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:56: // This is called in tests to modify (lower) retry delay. On 2016/01/28 21:05:58, stevenjb wrote: > If we don't use these yet we should remove them until we do (but it would be > preferable to have at least basic tests with the initial CL). Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:122: class QUIRKS_CLIENT_EXPORT QuirksClientManager { On 2016/01/28 20:16:31, oshima wrote: > define this in a separate file. Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:128: virtual std::string api_key_quirks() = 0; On 2016/01/28 20:16:31, oshima wrote: > GetApiKeyQuirks() > Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:130: virtual int GetDaysSinceOobe() = 0; On 2016/01/28 20:16:31, oshima wrote: > can/should these be const?(just asking) Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:131: }; On 2016/01/28 21:05:59, stevenjb wrote: > + private: DISALLOW_ASSIGN Is this needed / desirable in a pure virtual class? I added it to QuirksClientDelegateImpl... is that sufficient? https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:148: void SetLastServerCheck(int64_t product_id, base::Time last_check); On 2016/01/28 20:16:31, oshima wrote: > const base::Time& Done. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.h:165: base::MessageLoopForUI* message_loop_ui_; // To run QC on ui thread. On 2016/01/28 20:16:31, oshima wrote: > do you need this? can't you just use ::current() ? Do you mean base::MessageLoop::current()? From this call stack: ParseDisplayProfile() (in display_color_manager_chromeos.cc) QuirksClient::RequestIccProfilePath() QuirksClientManager::RunClient() I call message_loop_ui_->PostTask(...RunClientOnUIThread...) All the above is run on the io thread, and from there, base::MessageLoop::current() returns NULL.
https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_client_delegate_impl.h (right): https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_delegate_impl.h:21: int GetDaysSinceOobe() override; On 2016/02/01 23:17:43, Greg Levin wrote: > On 2016/01/28 20:16:30, oshima wrote: > > can these be const? (just a q) > > They can be (and now are). Is this preferable, even when the class has no > members to alter? Yes, in general (having member or not is implementation details from client point of view) unless you're sure that it will never have a state in the future. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:32: QUIRKS_CLIENT_EXPORT QuirksClientManager* g_manager = nullptr; On 2016/02/01 23:17:43, Greg Levin wrote: > On 2016/01/28 21:05:58, stevenjb wrote: > > On 2016/01/28 20:16:31, oshima wrote: > > > its weird to export stuff in anonymous namespace. what is this for? > > > > > > you may just want to call manager (or probably quirks_client_manager) as g_ > > > is for global. (this is file scope) > > > > I'm a little confused as to the relationship between QuirksClient and > > QuirksClientManager right now. I think that splitting these into separate > > files will help a lot. > > QuirksClientManager was originally QuirksClientDelegate. It was basically code > to allow QC to access various stuff outside of components/. But when I created > the current QuirksClientManager::Delegate, with a pure virtual interface and > impl code that actually needed to live outide components/, I renamed it. > > The current manager still holds stuff from outside components/ (pointers to msg > loops, prefs, UMA stats), and also starts the client running. When I add code > to support multiple monitors and have multiple clients running, manager is where > I'd keep track of them. It'll be solved by making this shared lib. https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... components/quirks_client/quirks_client.cc:46: const double kMaxRetrySleepSeconds = 6 * 3600; // 6 hours On 2016/02/01 23:17:43, Greg Levin wrote: > On 2016/01/28 20:16:31, oshima wrote: > > why not just use hours instead of seconds? > > In QuirksClient::Retry(), this value is compared directly with > kRetrySleepSeconds (which is of second-level granularity) to compute > delay_seconds. If the conversion isn't done here, it would have to be done > there. TimeDelta supports mathematical operations, including min/max, so you could have done them in TimeDelta instead. I'll leave it to you though. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client.gypi (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client.gypi:13: '../base/base.gyp:base_prefs', looks like you need dependency to net and that's probably why you were getting linkage errors.
https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:20: typedef scoped_ptr<DisplayColorManager::ColorCalibrationData> ColorDataPtr; typedefing scoped_ptr<> like this is uncommon in Chrome, I would avoid it. (Also, we prefer 'using' to 'typedef' now). https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:25: // icc file as soon as it's downloaded. For now, we'll just apply it on the nit: only one space between sentences https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:26: // next restart. The wording is a little confusing, it sounds like this code is involved with the restart process, but I assume you just mean something like 'For now we only apply the icc file if it exists at startup.' https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:28: quirks_client::QuirksClient::RequestIccProfilePath(product_id, nullptr); See other comments, but rather than passing null as a callback, create a local DoNothing method (or use base::DoNothing if the callback has no args). https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:65: ColorDataPtr data; I would expect this to crash since data is not being allocated here...? https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:74: return data; You may need to used data.Pass() here, although I know we've been adding more C++11 functionality to scoped_ptr so I'm not longer certain where we need that. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:119: base::Callback<ColorDataPtr(void)> request(base::Bind( I would typedef the callback instead of the scoped_ptr, e.g.: using CalibrationDataCallback = base::Callback(scoped_ptr<ColorCalibrationData>(void)>; https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:384: new quirks_client::QuirksClientManager( Rather than call Initialize with a new implementation of the class being initialized, just pass the construction parameters to Initialize() itself. That way it is more clear that QuirksClientManager owns its own singleton. https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:386: base::MessageLoopForUI::current(), This can be called from Initialize() instead of being passed in. https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:387: content::BrowserThread::GetBlockingPool(), This too. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/OWNERS (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/OWNERS:1: glevin@chromium.org Maybe add oshima@ also? I think it's better to have at least two owners in case one is OOO. A secondary owner review is better than a TBR (and can ask for the committer to wait a few days if necessary). https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.cc:41: QuirksClientManager* manager() { Use CamelCase for functions, e.g. GetManager(). https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.cc:42: return QuirksClientManager::Get(); This is potentially unsafe, see comments in q_c_m.cc. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.cc:107: manager()->RunClient(product_id, on_download_finished); It seems like this entire method should be part of QCM? Having a static QC method call into QCM which then instantiates a QC instance is very confusing. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.cc:184: if (ParseResult(response, &data)) { nit: Invert and early exit https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:35: typedef base::Callback<void(base::FilePath)> DownloadFinishedCallback; using https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:44: DownloadFinishedCallback* on_download_finished); callbacks should be passed as const& (and input references and pointers should always be const). https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:51: DownloadFinishedCallback* on_download_finished); const& https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:59: void StartDownload(); non-static methods generally proceed static methods. Also, public methods should have a comment. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:85: const DownloadFinishedCallback* on_download_finished_; Storing raw pointers in general is dangerous, particularly to callbacks. Ether this class should own the callback (which should be bound using WeakPtr), or we should use a Delegate or Observer model. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:36: // TODO: Why does this work, but ::current() doesn't? ? https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:44: if (time_since >= base::TimeDelta::FromDays(kDaysBetweenServerChecks)) { invert and early exit https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:49: * shutdown, while the delegate is invalidating, it might cause trouble. */ (No C style comments) Generally try to avoid unowned instances, but if they are used, they should be careful about not making any assumptions about lifetimes. In this case, since we would like to abort any pending file i/o during shutdown, I would make this a member function and have QCM own the QC instance. That way the instance can be destroyed on shutdown. QCM can be passed to the QC instance as a Delegate and informed when it is finished. Also, that way QCM can own and call on_download_finished instead of QC, and we can avoid having QC access the global QCM instance, it can call into its Delegate instead (which will be QCM, and which will always be safe since QCM will own the QC instance). https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:128: if (last_check == 0.0) { invert and early exit https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client_manager.h (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:33: virtual ~Delegate() {}; = default https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:36: virtual int GetDaysSinceOobe() const = 0; private: DISALLOW_ASSIGN
Patch Set 12 adds a browser test, and makes only the needed code changes to accommodate it. I hope to get to the next round of revisions by Monday. Thanks! https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:36: // TODO: Why does this work, but ::current() doesn't? Remove comment
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Fixed most comments. There are still 3 old ones (as noted in comments in new patch) that need to be addressed. I'd like to discuss these in person. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:20: typedef scoped_ptr<DisplayColorManager::ColorCalibrationData> ColorDataPtr; On 2016/02/02 23:45:50, stevenjb wrote: > typedefing scoped_ptr<> like this is uncommon in Chrome, I would avoid it. > (Also, we prefer 'using' to 'typedef' now). Done. (Do you have any alternate suggestions for mitigating the slight formatting mess caused by using "scoped_ptr<DisplayColorManager::ColorCalibrationData>" in multiple places?) https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:25: // icc file as soon as it's downloaded. For now, we'll just apply it on the On 2016/02/02 23:45:50, stevenjb wrote: > nit: only one space between sentences Done. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:26: // next restart. On 2016/02/02 23:45:50, stevenjb wrote: > The wording is a little confusing, it sounds like this code is involved with the > restart process, but I assume you just mean something like 'For now we only > apply the icc file if it exists at startup.' Done. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:28: quirks_client::QuirksClient::RequestIccProfilePath(product_id, nullptr); On 2016/02/02 23:45:50, stevenjb wrote: > See other comments, but rather than passing null as a callback, create a local > DoNothing method (or use base::DoNothing if the callback has no args). Done. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:65: ColorDataPtr data; On 2016/02/02 23:45:50, stevenjb wrote: > I would expect this to crash since data is not being allocated here...? > Done. (Ugh, thanks for saving me from debugging that >_<) https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:74: return data; On 2016/02/02 23:45:50, stevenjb wrote: > You may need to used data.Pass() here, although I know we've been adding more > C++11 functionality to scoped_ptr so I'm not longer certain where we need that. It doesn't seem to be necessary anymore. The code seems to work without it (and, in fact, data.Pass() doesn't even compile: "error: no member named 'Pass' in 'scoped_ptr<...>"). Also, the current version of https://www.chromium.org/developers/smart-pointer-guidelines seems to indicate that it's not needed. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:119: base::Callback<ColorDataPtr(void)> request(base::Bind( On 2016/02/02 23:45:50, stevenjb wrote: > I would typedef the callback instead of the scoped_ptr, e.g.: > > using CalibrationDataCallback = > base::Callback(scoped_ptr<ColorCalibrationData>(void)>; > Happy to make the change, but for my benefit, what do you see as the advantage? I typdef'd the scoped_ptr because it was used in 4 places, and made the code cleaner in each of them. I think this typedef'd Callback would only get used in this one place, and so would actually increase overall code length. If this is a desirable change, I'd like to understand why for future reference. Thanks! https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:384: new quirks_client::QuirksClientManager( On 2016/02/02 23:45:50, stevenjb wrote: > Rather than call Initialize with a new implementation of the class being > initialized, just pass the construction parameters to Initialize() itself. That > way it is more clear that QuirksClientManager owns its own singleton. > Done. https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:386: base::MessageLoopForUI::current(), On 2016/02/02 23:45:51, stevenjb wrote: > This can be called from Initialize() instead of being passed in. Done. https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:387: content::BrowserThread::GetBlockingPool(), On 2016/02/02 23:45:50, stevenjb wrote: > This too. Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client.gypi (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client.gypi:13: '../base/base.gyp:base_prefs', On 2016/02/02 18:29:13, oshima wrote: > looks like you need dependency to net and that's probably why you were getting > linkage errors. There are still some build issues that we'll need to work out in the next patch. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/OWNERS (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/OWNERS:1: glevin@chromium.org On 2016/02/02 23:45:51, stevenjb wrote: > Maybe add oshima@ also? I think it's better to have at least two owners in case > one is OOO. A secondary owner review is better than a TBR (and can ask for the > committer to wait a few days if necessary). Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.cc:41: QuirksClientManager* manager() { On 2016/02/02 23:45:51, stevenjb wrote: > Use CamelCase for functions, e.g. GetManager(). Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.cc:42: return QuirksClientManager::Get(); On 2016/02/02 23:45:51, stevenjb wrote: > This is potentially unsafe, see comments in q_c_m.cc. Acknowledged. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.cc:107: manager()->RunClient(product_id, on_download_finished); On 2016/02/02 23:45:51, stevenjb wrote: > It seems like this entire method should be part of QCM? Having a static QC > method call into QCM which then instantiates a QC instance is very confusing. I agree that the back-and-forth is confusing. But in the way I've separated the responsibilities of the two classes, this makes more sense. QC: Retrieve and provide display profiles. QCM: Track and manage QCs and provide thread/browser services. Display code should ideally only interact with QC. The only external (non-QC) interaction with QCM should be creating/destroying it at startup/shutdown. At least, that's the model I've been using. I suppose I could refactor and rename all of this with: QCM -> QuirksClient: Single persistent object that handles all external interactions. QC -> QuirksClientWorker: Handles the busywork of downloading new profiles. (Or possibly QCM -> QuirksProvider, and QC keeps old name, as it's still the class that's talking to the Quirks Server.) Under this model, I would move RequestIccProfilePath() and a few utilities to (new) QC/Provider (formerly QCM). Let's discuss in person, and I'll address this in the next patch. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.cc:184: if (ParseResult(response, &data)) { On 2016/02/02 23:45:51, stevenjb wrote: > nit: Invert and early exit Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:35: typedef base::Callback<void(base::FilePath)> DownloadFinishedCallback; On 2016/02/02 23:45:51, stevenjb wrote: > using Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:44: DownloadFinishedCallback* on_download_finished); On 2016/02/02 23:45:51, stevenjb wrote: > callbacks should be passed as const& (and input references and pointers should > always be const). Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:51: DownloadFinishedCallback* on_download_finished); On 2016/02/02 23:45:51, stevenjb wrote: > const& Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:59: void StartDownload(); On 2016/02/02 23:45:51, stevenjb wrote: > non-static methods generally proceed static methods. Also, public methods should > have a comment. > Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:85: const DownloadFinishedCallback* on_download_finished_; On 2016/02/02 23:45:51, stevenjb wrote: > Storing raw pointers in general is dangerous, particularly to callbacks. > > Ether this class should own the callback (which should be bound using WeakPtr), > or we should use a Delegate or Observer model. How about this, just storing the callback by "value"? I've seen this approach with numerous other class members. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:36: // TODO: Why does this work, but ::current() doesn't? On 2016/02/04 20:51:11, Greg Levin wrote: Was note to self. Resolved question, forgot to remove comment before upload. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:44: if (time_since >= base::TimeDelta::FromDays(kDaysBetweenServerChecks)) { On 2016/02/02 23:45:51, stevenjb wrote: > invert and early exit Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:49: * shutdown, while the delegate is invalidating, it might cause trouble. */ On 2016/02/02 23:45:51, stevenjb wrote: > (No C style comments) > > Generally try to avoid unowned instances, but if they are used, they should be > careful about not making any assumptions about lifetimes. > > In this case, since we would like to abort any pending file i/o during shutdown, > I would make this a member function and have QCM own the QC instance. That way > the instance can be destroyed on shutdown. QCM can be passed to the QC instance > as a Delegate and informed when it is finished. Also, that way QCM can own and > call on_download_finished instead of QC, and we can avoid having QC access the > global QCM instance, it can call into its Delegate instead (which will be QCM, > and which will always be safe since QCM will own the QC instance). > > Acknowledged... punting larger architectural changes to next patch, would like to discuss this one in person. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:128: if (last_check == 0.0) { On 2016/02/02 23:45:51, stevenjb wrote: > invert and early exit Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client_manager.h (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:33: virtual ~Delegate() {}; On 2016/02/02 23:45:51, stevenjb wrote: > = default Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:36: virtual int GetDaysSinceOobe() const = 0; On 2016/02/02 23:45:52, stevenjb wrote: > private: DISALLOW_ASSIGN Done. What are the pros/cons of putting DISALLOW_ASSIGN (or DISALLOW_COPY_AND_ASSIGN) on a pure virtual class? About half the pure virtual classes I found didn't include this macro. https://codereview.chromium.org/1528963002/diff/260001/components/quirks_clie... File components/quirks_client.gypi (right): https://codereview.chromium.org/1528963002/diff/260001/components/quirks_clie... components/quirks_client.gypi:13: '../base/base.gyp:base_prefs', On 2016/02/02 18:29:13, oshima wrote: > looks like you need dependency to net and that's probably why you were getting > linkage errors. TODO in next patch https://codereview.chromium.org/1528963002/diff/260001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/260001/components/quirks_clie... components/quirks_client/quirks_client.cc:133: GetManager()->RunClient(product_id, on_download_finished); On 2016/02/02 23:45:51, stevenjb wrote: > It seems like this entire method should be part of QCM? Having a static QC > method call into QCM which then instantiates a QC instance is very confusing. TODO: Possible refactor needed in next patch. https://codereview.chromium.org/1528963002/diff/260001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/260001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:52: // to use the manager after its been destroyed. On 2016/02/02 23:45:51, stevenjb wrote: > (No C style comments) > > Generally try to avoid unowned instances, but if they are used, they should be > careful about not making any assumptions about lifetimes. > > In this case, since we would like to abort any pending file i/o during shutdown, > I would make this a member function and have QCM own the QC instance. That way > the instance can be destroyed on shutdown. QCM can be passed to the QC instance > as a Delegate and informed when it is finished. Also, that way QCM can own and > call on_download_finished instead of QC, and we can avoid having QC access the > global QCM instance, it can call into its Delegate instead (which will be QCM, > and which will always be safe since QCM will own the QC instance). TODO in next patch
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sorry, I should have responded earlier. In light of mus-t-ash work, it's better to minimize dependency to MessageLoopForUI/content. https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:386: base::MessageLoopForUI::current(), On 2016/02/02 23:45:51, stevenjb wrote: > This can be called from Initialize() instead of being passed in. And please use base::ThreadTaskRunnerHandle::Get() instead of MessageLoopForUI to minimize dependency to MessageLoopForUI. You can check if the handle is the same as one that's obtained in Initialize to make sure if the task is posed on the right thread, if that's important. https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:387: content::BrowserThread::GetBlockingPool(), On 2016/02/09 18:56:37, Greg Levin wrote: > On 2016/02/02 23:45:50, stevenjb wrote: > > This too. > > Done. This one should stay, because we should minimize the dependency to content.
Patchset #16 (id:320001) has been deleted
Sorry, forgot to publish these. Some might be out of date now. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:20: typedef scoped_ptr<DisplayColorManager::ColorCalibrationData> ColorDataPtr; On 2016/02/09 18:56:37, Greg Levin wrote: > On 2016/02/02 23:45:50, stevenjb wrote: > > typedefing scoped_ptr<> like this is uncommon in Chrome, I would avoid it. > > (Also, we prefer 'using' to 'typedef' now). > > Done. (Do you have any alternate suggestions for mitigating the slight > formatting mess caused by using > "scoped_ptr<DisplayColorManager::ColorCalibrationData>" in multiple places?) DisplayColorManager:: isn't needed in several places so you can remove that. otherwise I think the extra verosity is fine. clang-format is your friend. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:74: return data; On 2016/02/09 18:56:37, Greg Levin wrote: > On 2016/02/02 23:45:50, stevenjb wrote: > > You may need to used data.Pass() here, although I know we've been adding more > > C++11 functionality to scoped_ptr so I'm not longer certain where we need > that. > > It doesn't seem to be necessary anymore. The code seems to work without it > (and, in fact, data.Pass() doesn't even compile: "error: no member named 'Pass' > in 'scoped_ptr<...>"). > Also, the current version of > https://www.chromium.org/developers/smart-pointer-guidelines seems to indicate > that it's not needed. Cool. Cheers. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:119: base::Callback<ColorDataPtr(void)> request(base::Bind( On 2016/02/09 18:56:37, Greg Levin wrote: > On 2016/02/02 23:45:50, stevenjb wrote: > > I would typedef the callback instead of the scoped_ptr, e.g.: > > > > using CalibrationDataCallback = > > base::Callback(scoped_ptr<ColorCalibrationData>(void)>; > > > > Happy to make the change, but for my benefit, what do you see as the advantage? > I typdef'd the scoped_ptr because it was used in 4 places, and made the code > cleaner in each of them. I think this typedef'd Callback would only get used in > this one place, and so would actually increase overall code length. If this is > a desirable change, I'd like to understand why for future reference. Thanks! Using scoped_ptr explicitly makes it more clear that the callback takes a familiar scoped_ptr. It's worth the extra verbosity. Also, looking at the code again, I would personally just inline the callback in PostTaskAndReplyWithResult() the same as you did for the 'reply' parameter instead of using a local variable at all. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:85: const DownloadFinishedCallback* on_download_finished_; On 2016/02/09 18:56:38, Greg Levin wrote: > On 2016/02/02 23:45:51, stevenjb wrote: > > Storing raw pointers in general is dangerous, particularly to callbacks. > > > > Ether this class should own the callback (which should be bound using > WeakPtr), > > or we should use a Delegate or Observer model. > > How about this, just storing the callback by "value"? I've seen this approach > with numerous other class members. Yes, that should be fine. https://codereview.chromium.org/1528963002/diff/260001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/260001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:121: base::Callback<scoped_ptr<DisplayColorManager::ColorCalibrationData>(void)> The DisplayColorManager:: is not needed here. https://codereview.chromium.org/1528963002/diff/260001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:133: scoped_ptr<DisplayColorManager::ColorCalibrationData> data) { DisplayColorManager:: not needed
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
glevin@chromium.org changed reviewers: + bauerb@chromium.org, blundell@chromium.org, isherman@chromium.org, pauljensen@chromium.org
Need reviews for smaller pieces, please take a look! (For context on this CL, see go/cros-quirks-client-dd) bauerb@: chrome/browser/prefs/browser_prefs.cc: Include '+components/prefs' in components/quirks_client/DEPS blundell@: components/components.gyp components/quirks_client.gypi (added) isherman@: tools/metrics/histograms/histograms.xml pauljensen@: Include various '+net/' in components/quirks_client/DEPS https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:386: base::MessageLoopForUI::current(), On 2016/02/10 01:34:00, oshima wrote: > On 2016/02/02 23:45:51, stevenjb wrote: > > This can be called from Initialize() instead of being passed in. > > And please use base::ThreadTaskRunnerHandle::Get() instead of MessageLoopForUI > to minimize dependency to MessageLoopForUI. You can check if the handle is the > same as one that's obtained in Initialize to make sure if the task is posed on > the right thread, if that's important. > Done. https://codereview.chromium.org/1528963002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:387: content::BrowserThread::GetBlockingPool(), On 2016/02/10 01:34:00, oshima wrote: > On 2016/02/09 18:56:37, Greg Levin wrote: > > On 2016/02/02 23:45:50, stevenjb wrote: > > > This too. > > > > Done. > > This one should stay, because we should minimize the dependency to content. Done. https://codereview.chromium.org/1528963002/diff/280001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1528963002/diff/280001/chrome/browser/about_f... chrome/browser/about_flags.cc:858: SINGLE_DISABLE_VALUE_TYPE(ash::switches::kAshDisableScreenOrientationLock), Fix format from merge
Description was changed from
==========
Working but incomplete draft of Quirks Client
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server,
check that file is downloaded to local directory. More details to
follow....
==========
to
==========
Creating a "Quirks Client" to download icc files and other display info
("quirks") from a Quirks Server. These provide display-specific tuning (e.g.
gamma ramps) on a per-monitor basis. For more info, see
go/cros-quirks-client-dd.
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server,
check that file is downloaded to local directory. More details to
follow....
==========
On 2016/02/11 17:30:25, Greg Levin wrote: > pauljensen@: > Include various '+net/' in components/quirks_client/DEPS I've never seen such specific dependencies, most components just specify "+net" and call it a day. Anyhow, looks fine to me.
https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:384: new quirks_client::QuirksClientDelegateImpl(), pass as scoped_ptr? https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_client_browsertest.cc (right): https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:18: base::FilePath g_icc_path; // Path to icc file if found or downloaded. can't you move this to QuirksClientBrowserTest instead? https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:77: QuirksClientBrowserTest() {} = default https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:80: ~QuirksClientBrowserTest() override {} ditto https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:107: g_EndMessageLoop = message_loop_runner->QuitClosure(); Can you use RunLoop instead? https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:133: ReadFile(path, data, 32); sizeof https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/prefs/b... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/prefs/b... chrome/browser/prefs/browser_prefs.cc:373: chromeos::TimeZoneResolver::RegisterPrefs(registry); Is this change necessary? https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client.cc:38: const double kMaxRetrySleepSeconds = 6 * 3600; // 6 hours any reason to use double? https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client.cc:146: return base::StringPrintf("%08" PRIx64, product_id); maybe std::hex, plus uppercase/lowercase to make it more explicit ? https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client.cc:158: response_code < (net::HTTP_INTERNAL_SERVER_ERROR + 100)); probabe better to define HTTP_INTERNAL_SERVER_ERROR_LAST const here, and I think it should be 99. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:48: // TODO!!! Before committing, we need to deal with ownership and lifecycle TODO(glevin) https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:95: QuirksClientManager* QuirksClientManager::Get() { DCHECK(g_manager) ? https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:110: is_new_device_ = (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks); isn't it better to check if the data exist to check now? https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:158: void QuirksClientManager::RecordFileFoundUmaStat(bool success) {} In general, I'd recommend to add these methods when you need it. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... File components/quirks_client/quirks_client_manager.h (right): https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:35: typedef base::Callback<scoped_ptr<net::URLFetcher>(const GURL&, using https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:48: DISALLOW_ASSIGN(Delegate); just fyi. You may omit this for pure interface, but may leave it as is. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:61: static void Shutdown(); new lines between methods. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:72: void RecordFileFoundUmaStat(bool success); document these. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:82: base::SingleThreadTaskRunner* TaskRunner(); task_manager() { return task_manager.get(); } https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:93: // browser/thread components needed for client. The comment isn't clear. What does this mean? https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... File components/quirks_client/switches.cc (right): https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/switches.cc:14: } // namespace proximity_auth namespace quirks_client
histograms.xml lgtm
On 2016/02/11 18:13:39, pauljensen wrote: > On 2016/02/11 17:30:25, Greg Levin wrote: > > pauljensen@: > > Include various '+net/' in components/quirks_client/DEPS > I've never seen such specific dependencies, most components just specify "+net" > and call it a day. Anyhow, looks fine to me. Probably got it copying code like https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor... I'll go ahead and simplify it. Could I get an official "lgtm"?
Here's the latest, with QCManager now owning and managing clients. Please have another look! https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:20: typedef scoped_ptr<DisplayColorManager::ColorCalibrationData> ColorDataPtr; On 2016/02/11 00:05:55, stevenjb wrote: > On 2016/02/09 18:56:37, Greg Levin wrote: > > On 2016/02/02 23:45:50, stevenjb wrote: > > > typedefing scoped_ptr<> like this is uncommon in Chrome, I would avoid it. > > > (Also, we prefer 'using' to 'typedef' now). > > > > Done. (Do you have any alternate suggestions for mitigating the slight > > formatting mess caused by using > > "scoped_ptr<DisplayColorManager::ColorCalibrationData>" in multiple places?) > > DisplayColorManager:: isn't needed in several places so you can remove that. > otherwise I think the extra verosity is fine. clang-format is your friend. Done. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:119: base::Callback<ColorDataPtr(void)> request(base::Bind( On 2016/02/11 00:05:55, stevenjb wrote: > On 2016/02/09 18:56:37, Greg Levin wrote: > > On 2016/02/02 23:45:50, stevenjb wrote: > > > I would typedef the callback instead of the scoped_ptr, e.g.: > > > > > > using CalibrationDataCallback = > > > base::Callback(scoped_ptr<ColorCalibrationData>(void)>; > > > > > > > Happy to make the change, but for my benefit, what do you see as the > advantage? > > I typdef'd the scoped_ptr because it was used in 4 places, and made the code > > cleaner in each of them. I think this typedef'd Callback would only get used > in > > this one place, and so would actually increase overall code length. If this > is > > a desirable change, I'd like to understand why for future reference. Thanks! > > Using scoped_ptr explicitly makes it more clear that the callback takes a > familiar scoped_ptr. It's worth the extra verbosity. > > Also, looking at the code again, I would personally just inline the callback in > PostTaskAndReplyWithResult() the same as you did for the 'reply' parameter > instead of using a local variable at all. > Done. https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... components/quirks_client/quirks_client.h:85: const DownloadFinishedCallback* on_download_finished_; On 2016/02/11 00:05:55, stevenjb wrote: > On 2016/02/09 18:56:38, Greg Levin wrote: > > On 2016/02/02 23:45:51, stevenjb wrote: > > > Storing raw pointers in general is dangerous, particularly to callbacks. > > > > > > Ether this class should own the callback (which should be bound using > > WeakPtr), > > > or we should use a Delegate or Observer model. > > > > How about this, just storing the callback by "value"? I've seen this approach > > with numerous other class members. > > Yes, that should be fine. Done. https://codereview.chromium.org/1528963002/diff/260001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/260001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:121: base::Callback<scoped_ptr<DisplayColorManager::ColorCalibrationData>(void)> On 2016/02/11 00:05:55, stevenjb wrote: > The DisplayColorManager:: is not needed here. |request| variable removed and callback inlined, as per previous comment. https://codereview.chromium.org/1528963002/diff/260001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:133: scoped_ptr<DisplayColorManager::ColorCalibrationData> data) { On 2016/02/11 00:05:55, stevenjb wrote: > DisplayColorManager:: not needed Done. https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:384: new quirks_client::QuirksClientDelegateImpl(), On 2016/02/11 20:05:51, oshima wrote: > pass as scoped_ptr? Done... does this look ok? make_scoped_ptr wasn't working because of the QuirksClientDelegateImpl vs QuirksClientManager::Delegate type mismatch. https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_client_browsertest.cc (right): https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:18: base::FilePath g_icc_path; // Path to icc file if found or downloaded. On 2016/02/11 20:05:51, oshima wrote: > can't you move this to QuirksClientBrowserTest instead? Tried, aborted due to weak_ptr crash. I did modify RunQuirksClientOnIoThread() to remove race condition. https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:77: QuirksClientBrowserTest() {} On 2016/02/11 20:05:51, oshima wrote: > = default Done. https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:80: ~QuirksClientBrowserTest() override {} On 2016/02/11 20:05:51, oshima wrote: > ditto Done. https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:107: g_EndMessageLoop = message_loop_runner->QuitClosure(); On 2016/02/11 20:05:51, oshima wrote: > Can you use RunLoop instead? Done. https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_client_browsertest.cc:133: ReadFile(path, data, 32); On 2016/02/11 20:05:51, oshima wrote: > sizeof Done. https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/prefs/b... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/prefs/b... chrome/browser/prefs/browser_prefs.cc:373: chromeos::TimeZoneResolver::RegisterPrefs(registry); On 2016/02/11 20:05:51, oshima wrote: > Is this change necessary? No, I just thought I'd put the item in alphabetical order like the rest of this list. Should I undo it? https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client.cc:38: const double kMaxRetrySleepSeconds = 6 * 3600; // 6 hours On 2016/02/11 20:05:51, oshima wrote: > any reason to use double? > Not that I know of... I was just copying code from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Maybe to avoid int overflow when computing delay_seconds down below, in case the device is left running for 10 years? :-P I've changed these to unsigned's. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client.cc:146: return base::StringPrintf("%08" PRIx64, product_id); On 2016/02/11 20:05:51, oshima wrote: > maybe std::hex, plus uppercase/lowercase to make it more explicit ? Discussed off-line, leaving as is. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client.cc:158: response_code < (net::HTTP_INTERNAL_SERVER_ERROR + 100)); On 2016/02/11 20:05:51, oshima wrote: > probabe better to define HTTP_INTERNAL_SERVER_ERROR_LAST const here, and I think > it should be 99. Done. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:48: // TODO!!! Before committing, we need to deal with ownership and lifecycle On 2016/02/11 20:05:51, oshima wrote: > TODO(glevin) I was using the TODO!!! to denote things I wanted to fix before first version landed. I intended to mention that in a CL comment, but I can't seem to find that anywhere. In any case, the issue has been resolved, and the TODO removed. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:95: QuirksClientManager* QuirksClientManager::Get() { On 2016/02/11 20:05:51, oshima wrote: > DCHECK(g_manager) ? Done. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:110: is_new_device_ = (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks); On 2016/02/11 20:05:51, oshima wrote: > isn't it better to check if the data exist to check now? Discussed off-line https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:158: void QuirksClientManager::RecordFileFoundUmaStat(bool success) {} On 2016/02/11 20:05:51, oshima wrote: > In general, I'd recommend to add these methods when you need it. Acknowledged. In this case, I wanted placeholders to develop around so that they were accommodated by my current design. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... File components/quirks_client/quirks_client_manager.h (right): https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:35: typedef base::Callback<scoped_ptr<net::URLFetcher>(const GURL&, On 2016/02/11 20:05:51, oshima wrote: > using Done. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:48: DISALLOW_ASSIGN(Delegate); On 2016/02/11 20:05:51, oshima wrote: > just fyi. You may omit this for pure interface, but may leave it as is. :-) https://codereview.chromium.org/1528963002/diff/200001/components/quirks_clie... https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:61: static void Shutdown(); On 2016/02/11 20:05:51, oshima wrote: > new lines between methods. Done. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:72: void RecordFileFoundUmaStat(bool success); On 2016/02/11 20:05:51, oshima wrote: > document these. Done. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:82: base::SingleThreadTaskRunner* TaskRunner(); On 2016/02/11 20:05:51, oshima wrote: > task_manager() { return task_manager.get(); } Done. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:93: // browser/thread components needed for client. On 2016/02/11 20:05:51, oshima wrote: > The comment isn't clear. What does this mean? Edited. https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... File components/quirks_client/switches.cc (right): https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... components/quirks_client/switches.cc:14: } // namespace proximity_auth On 2016/02/11 20:05:51, oshima wrote: > namespace quirks_client Done.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
top-level //components lgtm Please add per-file OWNERS of the new gypi file to //components/OWNERS https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/DEPS (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/DEPS:2: "+chromeos/chromeos_paths.h", You're missing this dependency in the gypi.
Usage of preferences is mostly fine here, but I found some issues with thread safety that seem rather worrying... https://codereview.chromium.org/1528963002/diff/360001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/360001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:20: void DoNothing(base::FilePath) {} Take a const reference, so the path doesn't have to be copied just to then be ignored? 😃 https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client.cc:82: if (!GetManager()) { Wait, you call this method on a background thread? That seems very dangerous, concurrency-wise. How do you know that the object won't be destroyed under you on its origin thread? I think you will have to go through the QuirksClientManager first (on the UI thread), grab everything you need there, and then post a task to the background thread to do the file access and if necessary post a task back to the origin thread. In general, making objects operate on one single thread (or sequence) and doing background stuff exclusively using static methods is a very useful pattern if you want to avoid these kinds of issues. An alternative might be to use locking, but that's a) not quite as encouraged in Chrome as single-threadedness, and b) would also require a bit of refactoring to be able to do everything under a single lock. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client.cc:186: if (!ParseResult(response, &data)) So if we can't parse the result, we'll just never finish? https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client.h:99: unsigned retries_; You could use net::BackoffEntry for this. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:20: QUIRKS_CLIENT_EXPORT QuirksClientManager* g_manager = nullptr; Wait, do you need to export a symbol from an anonymous namespace? https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:46: delete delegate_; Make |delegate_| a scoped_ptr? https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:86: weak_ptr_factory_.GetWeakPtr(), product_id, This is extremely subtle. Assuming that accessing |this| is safe here on a background thread (see my other comment above), you can get a weak pointer from the factory, but you can't do _anything_ with it apart from passing it to the thread where it will be dereferenced (that includes checking it for validity!). So, if you keeps this, I think it would at least warrant a comment, to make sure that this is not going to be just blindly copied into a situation where it is actually incorrect. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:102: if (dict) This isn't necessary -- GetDictionary() is guaranteed to return a non-null pointer. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:137: DCHECK(base::ThreadTaskRunnerHandle::IsSet()); Do you want to check that you are on your origin thread here? If so, use task_runner_->RunsTasksOnCurrentThread(). This just checks that there is _a_ thread task runner for the current thread (which in practice means there is a MessageLoop), but doesn't say anything about which thread it is. (It kind of works for checking you're _not_ on a worker pool thread, because those don't have a message loop or a ThreadTaskRunnerHandle, but that's just an artifact of how SequencedWorkerPool is implemented.) https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:153: fake_quirks_fetcher_creator_.is_null() Nit: I think I would split this up into two return statements: if (!fake_quirks_fetcher_creator_.is_null()) return fake_quirks_fetcher_creator_.Run(); return net::URLFetcher::Create(...); https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:183: base::ThreadTaskRunnerHandle::IsSet() && task_runner_ && It's a bit strange that you have ThreadTaskRunnerHandle::IsSet() as a precondition, but then check it here in code. If it has to be true (i.e. it being false means a bug), you shouldn't have to check it again. See also https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC.... https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client_manager.h (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:47: virtual int GetDaysSinceOobe() const = 0; Maybe add comments which threads this might be called on? https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:53: QuirksClientManager(Delegate* delegate, If this class is meant to be created and destroyed via ::Initialize() and ::Shutdown(), make constructor and destructor private? https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:54: base::SequencedWorkerPool* blocking_pool, If you keep a reference to this, pass it as a scoped_refptr. Same for the |url_context_getter|. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:57: virtual ~QuirksClientManager(); This class doesn't have any other virtual methods, so the destructor doesn't need to be virtual either. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:71: void DeleteClient(QuirksClient* client); So, this is a bit of a strange interface... This class owns the clients, but someone else can go and delete one of the clients? That kind of contradicts the concept of ownership. Could the caller (i.e. the QuirksClient) just delete itself? Then this class could just keep a set of raw pointers, and if it is destroyed, it would call Shutdown(false) on all the pending clients. Or if you want to keep this class owning the clients, I would at least rename this method to something like ClientFinished(), so the fact that this class chooses to delete a finished client would not be part of the interface. You could also store the clients in a std::set<scoped_ptr<>> and use a custom predicate to look an entry up by raw pointer; I think that would still look nicer than a map from raw pointer to scoped pointer. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:96: void SetFakeQuirksFetcherCreator(const FakeQuirksFetcherCreator& creator) { ...ForTesting? https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:106: bool ValidateOnUiThread(); I don't quite get the point of this method. If you want to check that the UI thread is correctly set up, returning (not DCHECKing!) ThreadTaskRunnerHandle::IsSet() would be sufficient, but if that was not the case, you would have already DCHECKed in the constructor, because ThreadTaskRunnerHandle::Get() has that as a precondition.
Will address largest three comments from last review in subsequent patch. https://codereview.chromium.org/1528963002/diff/360001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/360001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:20: void DoNothing(base::FilePath) {} On 2016/02/12 14:51:21, Bernhard Bauer wrote: > Take a const reference, so the path doesn't have to be copied just to then be > ignored? 😃 Done. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client.cc:186: if (!ParseResult(response, &data)) On 2016/02/12 14:51:21, Bernhard Bauer wrote: > So if we can't parse the result, we'll just never finish? Done (added Shutdown()). https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client.h:99: unsigned retries_; On 2016/02/12 14:51:21, Bernhard Bauer wrote: > You could use net::BackoffEntry for this. Done. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:20: QUIRKS_CLIENT_EXPORT QuirksClientManager* g_manager = nullptr; On 2016/02/12 14:51:21, Bernhard Bauer wrote: > Wait, do you need to export a symbol from an anonymous namespace? I did until recently. Without it, calls into Quirks code from ui and file threads would find different instances of this global if I didn't use the EXPORT. But it seems that the recent switch from target type = 'static_library' to '<(component)' has solved that. So EXPORT removed. Thanks! https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:46: delete delegate_; On 2016/02/12 14:51:21, Bernhard Bauer wrote: > Make |delegate_| a scoped_ptr? Done. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:102: if (dict) On 2016/02/12 14:51:21, Bernhard Bauer wrote: > This isn't necessary -- GetDictionary() is guaranteed to return a non-null > pointer. Done. Thanks for the clarifying link below. I saw a lot of examples of checking GetDictionary() != NULL, so followed that pattern out of caution. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:137: DCHECK(base::ThreadTaskRunnerHandle::IsSet()); On 2016/02/12 14:51:21, Bernhard Bauer wrote: > Do you want to check that you are on your origin thread here? If so, use > task_runner_->RunsTasksOnCurrentThread(). This just checks that there is _a_ > thread task runner for the current thread (which in practice means there is a > MessageLoop), but doesn't say anything about which thread it is. > > (It kind of works for checking you're _not_ on a worker pool thread, because > those don't have a message loop or a ThreadTaskRunnerHandle, but that's just an > artifact of how SequencedWorkerPool is implemented.) It doesn't need to be the same thread (as far as I know). I just have that here because the Get/SetPref code calls CalledOnValidThread(). https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:153: fake_quirks_fetcher_creator_.is_null() On 2016/02/12 14:51:21, Bernhard Bauer wrote: > Nit: I think I would split this up into two return statements: > > if (!fake_quirks_fetcher_creator_.is_null()) > return fake_quirks_fetcher_creator_.Run(); > > return net::URLFetcher::Create(...); Done. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:183: base::ThreadTaskRunnerHandle::IsSet() && task_runner_ && On 2016/02/12 14:51:21, Bernhard Bauer wrote: > It's a bit strange that you have ThreadTaskRunnerHandle::IsSet() as a > precondition, but then check it here in code. If it has to be true (i.e. it > being false means a bug), you shouldn't have to check it again. See also > https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC.... Done. I've been going back and forth on whether to just remove this function. I added it for debugging, but with the current code, I don't know of any circumstance under which it could fail. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client_manager.h (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:47: virtual int GetDaysSinceOobe() const = 0; On 2016/02/12 14:51:22, Bernhard Bauer wrote: > Maybe add comments which threads this might be called on? Done... added comment to GetDaysSinceOobe(). The others don't have thread restrictions (that I'm aware of). https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:53: QuirksClientManager(Delegate* delegate, On 2016/02/12 14:51:22, Bernhard Bauer wrote: > If this class is meant to be created and destroyed via ::Initialize() and > ::Shutdown(), make constructor and destructor private? Done. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:54: base::SequencedWorkerPool* blocking_pool, On 2016/02/12 14:51:22, Bernhard Bauer wrote: > If you keep a reference to this, pass it as a scoped_refptr. Same for the > |url_context_getter|. Done. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:57: virtual ~QuirksClientManager(); On 2016/02/12 14:51:22, Bernhard Bauer wrote: > This class doesn't have any other virtual methods, so the destructor doesn't > need to be virtual either. Done. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:96: void SetFakeQuirksFetcherCreator(const FakeQuirksFetcherCreator& creator) { On 2016/02/12 14:51:21, Bernhard Bauer wrote: > ...ForTesting? Done. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:106: bool ValidateOnUiThread(); On 2016/02/12 14:51:22, Bernhard Bauer wrote: > I don't quite get the point of this method. If you want to check that the UI > thread is correctly set up, returning (not DCHECKing!) > ThreadTaskRunnerHandle::IsSet() would be sufficient, but if that was not the > case, you would have already DCHECKed in the constructor, because > ThreadTaskRunnerHandle::Get() has that as a precondition. I've simplified this method and removed its DCHECK.
Patch #19 simplifies the naming of Quirks classes and files as follows: Directory quirks_client/ -> quirks/ namespace quirks_client -> quirks Class QuirksClientManager -> QuirksManager Class QuirksClientDelegateImpl -> QuirksManagerDelegateImpl with files and a few define#s also renamed to match. This is done ahead of a small refactor which will slightly shift the roles of the client and manager classes.
Moved external access functions RequestIccProfilePath() and IdToHexString() from client to manager file. QuirksClient is no longer exposed outside of components/, and is now just used for downloading files. All other bits have been moved to QuirksManager and quirks namespace. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client.cc:82: if (!GetManager()) { On 2016/02/12 14:51:21, Bernhard Bauer wrote: > Wait, you call this method on a background thread? That seems very dangerous, > concurrency-wise. How do you know that the object won't be destroyed under you > on its origin thread? > > I think you will have to go through the QuirksClientManager first (on the UI > thread), grab everything you need there, and then post a task to the background > thread to do the file access and if necessary post a task back to the origin > thread. In general, making objects operate on one single thread (or sequence) > and doing background stuff exclusively using static methods is a very useful > pattern if you want to avoid these kinds of issues. > > An alternative might be to use locking, but that's a) not quite as encouraged in > Chrome as single-threadedness, and b) would also require a bit of refactoring to > be able to do everything under a single lock. Keeping the manager on one thread is tricky, as it needs info from both ui and file threads that is only available once it has a product id, which it doesn't have at initialization time. If the file already exists, the path must be returned immediately, and can only be provided by the delegate object. I *could* cache that path as a file-global at startup, so that code from deletable objects would only run in posted tasks. I know this only mitigates the problem and doesn't solve it, but I've refactored to limit the concurrency danger by moving most of this function into a method, and only using if (g_manager) return g_manager->RequestIccFilePath() to access it. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client_manager.cc (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.cc:86: weak_ptr_factory_.GetWeakPtr(), product_id, On 2016/02/12 14:51:21, Bernhard Bauer wrote: > This is extremely subtle. Assuming that accessing |this| is safe here on a > background thread (see my other comment above), you can get a weak pointer from > the factory, but you can't do _anything_ with it apart from passing it to the > thread where it will be dereferenced (that includes checking it for validity!). > So, if you keeps this, I think it would at least warrant a comment, to make sure > that this is not going to be just blindly copied into a situation where it is > actually incorrect. See comment on related comment, but I'll keep working on this. Any additional advice would be appreciated. https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... File components/quirks_client/quirks_client_manager.h (right): https://codereview.chromium.org/1528963002/diff/360001/components/quirks_clie... components/quirks_client/quirks_client_manager.h:71: void DeleteClient(QuirksClient* client); On 2016/02/12 14:51:22, Bernhard Bauer wrote: > So, this is a bit of a strange interface... This class owns the clients, but > someone else can go and delete one of the clients? That kind of contradicts the > concept of ownership. Could the caller (i.e. the QuirksClient) just delete > itself? Then this class could just keep a set of raw pointers, and if it is > destroyed, it would call Shutdown(false) on all the pending clients. > > Or if you want to keep this class owning the clients, I would at least rename > this method to something like ClientFinished(), so the fact that this class > chooses to delete a finished client would not be part of the interface. You > could also store the clients in a std::set<scoped_ptr<>> and use a custom > predicate to look an entry up by raw pointer; I think that would still look > nicer than a map from raw pointer to scoped pointer. Done (went with second option: renamed function, switched to std::set).
I agree that there are some issues here with threading that I think can be cleaned up by changing the way quirks::RequestIccProfilePath is called. https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:41: DCHECK(base::PathExists(path)); // Quirks should ensure this. Don't do file operations here. https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:121: blocking_pool_, FROM_HERE, We shouldn't be assuming that this blocking pool is the same as the QuirksManager blocking pool. See my suggestions in quirks_manager.cc. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/DEPS File components/quirks/DEPS (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/DEPS... components/quirks/DEPS:2: "+chromeos/chromeos_paths.h", I don't think we should add this dependency, we can pass the path into the constructor (or add a method to the delegate). https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:41: } Not needed (see comment below) https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:55: IdToHexString(product_id) + ".icc")), .icc should be defined as a const char[] in the anon namespace https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:67: // the Quirks Server is not currently using this value. We should fix this now or do something else in the meanwhile, it's very confusing. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:91: net::HTTP_INTERNAL_SERVER_ERROR + 99; Where does 99 come from? https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:141: manager_->SetLastServerCheck(product_id_, base::Time::Now()); Maybe make this part of ClientFinished? https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:159: // "weak_ptrs can only bind to methods without return values". This shouldn't need to be a static member at all, it can just be a file local method since all it is doing is writing data to a file. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:163: GetManager()->blocking_pool()->RunsTasksOnCurrentThread()); Why do we need this DCHECK? If it is important that the manager still exists, then writing the file should be handled by the manager. Otherwise this function should not know or care about the manager. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.h:61: QuirksManager* manager_; Add a comment that this is a weak pointer guaranteed to outlive this class. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.h:69: // Why are we making this server call? Don't phrase the comment as a question. 'Reason for this server call.' https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:47: // Must be called on file thread. Requiring this to be called on the file thread is confusing and adds unnecessary complexity to the caller. It also makes the assumption that the "file thread" is the same as blocking_pool_ which is confusing. Instead, the API should look something like: quirks::RequestIccProfilePath(id, callback) { if (!g_manager) callback.Run(base::FilePath()); g_manager->RequestIccFilePath(id, callback); } And the implementation something like: QuirksManager::RequestIccFilePath(id, callback) { CHECK(thread_checker_.CalledOnValidThread()); blocking_pool_->PostTaskAndReply( Bind(GetIccFilePathOnBlockingPool, id, callback, any other args), Bind(QuirksManager::CompleteRequestIccFilePath, weak ptr, id, callback); } QuirksManager::CompleteRequestIccFilePath(id, callback, result) { callback.Run(result); } https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:116: PathService::Get(chromeos::DIR_DEVICE_COLOR_CALIBRATION_PROFILES, &path)); nit: Rather than adding a src/chromeos dependency for just this line, make icc_filepath a constructor arg. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:91: scoped_ptr<net::URLFetcher> CreateURLFetcher( ForTest? https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:98: } blank line https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:101: } blank line https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:104: } blank line Also, do these all need to be accessible for anything other than tests? Anything that is needed for tests should be protected with the test class(es) declared as friends. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:122: bool ValidateOnUiThread(); Replace any references to 'ui thread' with 'client task runner' (and rename 'task_runner_' 'client_task_runner_')
https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:47: // Must be called on file thread. On 2016/02/16 20:07:51, stevenjb wrote: > Requiring this to be called on the file thread is confusing and adds unnecessary > complexity to the caller. It also makes the assumption that the "file thread" is > the same as blocking_pool_ which is confusing. > > > Instead, the API should look something like: > > quirks::RequestIccProfilePath(id, callback) { > if (!g_manager) > callback.Run(base::FilePath()); > g_manager->RequestIccFilePath(id, callback); > } > > And the implementation something like: > > QuirksManager::RequestIccFilePath(id, callback) { > CHECK(thread_checker_.CalledOnValidThread()); > > blocking_pool_->PostTaskAndReply( > Bind(GetIccFilePathOnBlockingPool, id, callback, any other args), > Bind(QuirksManager::CompleteRequestIccFilePath, weak ptr, id, callback); > } > > QuirksManager::CompleteRequestIccFilePath(id, callback, result) { > callback.Run(result); > } > > Also, CompleteRequestIccFilePath() could create the new QuirksClient if the result is empty. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:152: if (it != clients_.end()) I think I would DCHECK this instead of silently failing. This should never be called with an unregistered client.
https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:41: DCHECK(base::PathExists(path)); // Quirks should ensure this. On 2016/02/16 20:07:51, stevenjb wrote: > Don't do file operations here. Done. Why not? https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:121: blocking_pool_, FROM_HERE, On 2016/02/16 20:07:51, stevenjb wrote: > We shouldn't be assuming that this blocking pool is the same as the > QuirksManager blocking pool. See my suggestions in quirks_manager.cc. Still fuzzy on this issue... hopefully discussion in quirks_manager.cc will provide some clarification. https://codereview.chromium.org/1528963002/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_browsertest.cc (right): https://codereview.chromium.org/1528963002/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:67: QuirksManager::Get()->task_runner()->PostTask(FROM_HERE, g_EndMessageLoop); Is there a better way to do this? https://codereview.chromium.org/1528963002/diff/420001/components/quirks/DEPS File components/quirks/DEPS (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/DEPS... components/quirks/DEPS:2: "+chromeos/chromeos_paths.h", On 2016/02/16 20:07:51, stevenjb wrote: > I don't think we should add this dependency, we can pass the path into the > constructor (or add a method to the delegate). Done (added to delegate). https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:41: } On 2016/02/16 20:07:51, stevenjb wrote: > Not needed (see comment below) Deleted! https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:55: IdToHexString(product_id) + ".icc")), On 2016/02/16 20:07:51, stevenjb wrote: > .icc should be defined as a const char[] in the anon namespace Done... created IdToFileName() to go with IdToHexString() in quirks_manager.cc, using const char[] there. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:67: // the Quirks Server is not currently using this value. On 2016/02/16 20:07:51, stevenjb wrote: > We should fix this now or do something else in the meanwhile, it's very > confusing. Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:91: net::HTTP_INTERNAL_SERVER_ERROR + 99; On 2016/02/16 20:07:51, stevenjb wrote: > Where does 99 come from? It was originally copied from here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... with HTTP_INTERNAL_SERVER_ERROR_LAST defined as per comment: https://codereview.chromium.org/1528963002/diff2/340001:360001/components/qui... Codes are defined here: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stat... The 500-599 range is for server errors: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:141: manager_->SetLastServerCheck(product_id_, base::Time::Now()); On 2016/02/16 20:07:51, stevenjb wrote: > Maybe make this part of ClientFinished? Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:159: // "weak_ptrs can only bind to methods without return values". On 2016/02/16 20:07:51, stevenjb wrote: > This shouldn't need to be a static member at all, it can just be a file local > method since all it is doing is writing data to a file. Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:163: GetManager()->blocking_pool()->RunsTasksOnCurrentThread()); On 2016/02/16 20:07:51, stevenjb wrote: > Why do we need this DCHECK? If it is important that the manager still exists, > then writing the file should be handled by the manager. Otherwise this function > should not know or care about the manager. > Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.h:61: QuirksManager* manager_; On 2016/02/16 20:07:51, stevenjb wrote: > Add a comment that this is a weak pointer guaranteed to outlive this class. > Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.h:69: // Why are we making this server call? On 2016/02/16 20:07:51, stevenjb wrote: > Don't phrase the comment as a question. 'Reason for this server call.' Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:47: // Must be called on file thread. Ok, this is just my incomplete understanding of the blocking pool system, but... > Requiring this to be called on the file thread is confusing and adds unnecessary > complexity to the caller. I'm not sure where the confusion comes from. This function obviously needs to run on the file thread, as it's looking for a file. Similarly, I would assume that any function calling it would also be running on the file thread, as the next step after calling it is to open the requested file. I see it as a more sophisticated version of PathExists(). You call it from the file thread because you know it needs to run on the file thread, and very likely you're about to use the file. The fact that the QM will go off on its own and get the file later if it's not found isn't a detail that the caller needs to be aware of. > It also makes the assumption that the "file thread" is > the same as blocking_pool_ which is confusing. Is this assumption just in DCHECK(blocking_pool()->RunsTasksOnCurrentThread()), or is it being made somewhere else that I'm not seeing? The DCHECK is just meant to validate that it's running on the file thread, if there's a better way to do that. I could remove the DCHECK if it's confusion / misused. > Instead, the API should look something like: > > quirks::RequestIccProfilePath(id, callback) { > if (!g_manager) > callback.Run(base::FilePath()); > g_manager->RequestIccFilePath(id, callback); > } > > And the implementation something like: > > QuirksManager::RequestIccFilePath(id, callback) { > CHECK(thread_checker_.CalledOnValidThread()); > > blocking_pool_->PostTaskAndReply( > Bind(GetIccFilePathOnBlockingPool, id, callback, any other args), > Bind(QuirksManager::CompleteRequestIccFilePath, weak ptr, id, callback); > } > > QuirksManager::CompleteRequestIccFilePath(id, callback, result) { > callback.Run(result); > } This seems like it adds to the complexity of the caller, forcing it to implement a callback, which the current system does not. The callback is currently optional, only if the system wants to use the downloaded file ASAP. It would certainly complicate the code in display_color_manager_chromeos.cc. Currently, we just have PostTaskAndReplyWithResult(ParseDisplayProfile, UpdateCalibrationData) With the proposed scheme, we'd have to bounce through ParseDisplayProfile -> RequestIccProfilePath -> GetIccFilePathOnBlockingPool -> CompleteRequestIccFilePath -> ParseDisplayProfileCallback -> UpdateCalibrationData I'm not sure how to do this with the PostTaskAndReplyWithResult model. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:116: PathService::Get(chromeos::DIR_DEVICE_COLOR_CALIBRATION_PROFILES, &path)); On 2016/02/16 20:07:51, stevenjb wrote: > nit: Rather than adding a src/chromeos dependency for just this line, make > icc_filepath a constructor arg. Moved it to delegate instead (as per suggestion in DEPS comment) https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:152: if (it != clients_.end()) On 2016/02/17 17:27:40, Bernhard Bauer wrote: > I think I would DCHECK this instead of silently failing. This should never be > called with an unregistered client. Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:91: scoped_ptr<net::URLFetcher> CreateURLFetcher( On 2016/02/16 20:07:51, stevenjb wrote: > ForTest? Well, URLFetcher::Create() was moved in here from QC so I could create this function to accomodate tests, but this function isn't exclusively for tests; it will create a real URLFetcher for real downloads. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:98: } On 2016/02/16 20:07:52, stevenjb wrote: > blank line Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:101: } On 2016/02/16 20:07:51, stevenjb wrote: > blank line Done. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:104: } On 2016/02/16 20:07:51, stevenjb wrote: > blank line > > Also, do these all need to be accessible for anything other than tests? Anything > that is needed for tests should be protected with the test class(es) declared as > friends. I did this for SetFakeQuirksFetcherCreatorForTests(), but task_runner() is accessed by a file local function in quirks_browser_test.cc. Actually, is there any better way to access a task runner from RunQuirksClientOnBlockingPool() in that file than what I'm currently doing there? https://codereview.chromium.org/1528963002/diff/420001/chrome/browser/chromeo... https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.h:122: bool ValidateOnUiThread(); On 2016/02/16 20:07:51, stevenjb wrote: > Replace any references to 'ui thread' with 'client task runner' (and rename > 'task_runner_' 'client_task_runner_') Done.
https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:41: DCHECK(base::PathExists(path)); // Quirks should ensure this. On 2016/02/17 23:01:50, Greg Levin wrote: > On 2016/02/16 20:07:51, stevenjb wrote: > > Don't do file operations here. > > Done. Why not? Then we have to guarantee that this runs on a blocking pool task runner when there is really no need for that (see other comments). https://codereview.chromium.org/1528963002/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_browsertest.cc (right): https://codereview.chromium.org/1528963002/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:19: base::FilePath g_icc_path; // Path to icc file if found or downloaded. I am going to focus on the non test code for this pass, but we should be able to test this without resorting to using file local variables (which should be named s_foo_bar btw). https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_client.cc:91: net::HTTP_INTERNAL_SERVER_ERROR + 99; On 2016/02/17 23:01:51, Greg Levin wrote: > On 2016/02/16 20:07:51, stevenjb wrote: > > Where does 99 come from? > > It was originally copied from here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > with HTTP_INTERNAL_SERVER_ERROR_LAST defined as per comment: > https://codereview.chromium.org/1528963002/diff2/340001:360001/components/qui... > > Codes are defined here: > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stat... > > The 500-599 range is for server errors: > https://en.wikipedia.org/wiki/List_of_HTTP_status_codes I see. Ugh, OK. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:47: // Must be called on file thread. On 2016/02/17 23:01:51, Greg Levin wrote: > Ok, this is just my incomplete understanding of the blocking pool system, but... > > > Requiring this to be called on the file thread is confusing and adds > unnecessary > > complexity to the caller. > > I'm not sure where the confusion comes from. This function obviously needs to > run on the file thread, as it's looking for a file. Similarly, I would assume > that any function calling it would also be running on the file thread, as the > next step after calling it is to open the requested file. I see it as a more > sophisticated version of PathExists(). You call it from the file thread because > you know it needs to run on the file thread, and very likely you're about to use > the file. The fact that the QM will go off on its own and get the file later if > it's not found isn't a detail that the caller needs to be aware of. > > > It also makes the assumption that the "file thread" is > > the same as blocking_pool_ which is confusing. > > Is this assumption just in > DCHECK(blocking_pool()->RunsTasksOnCurrentThread()), > or is it being made somewhere else that I'm not seeing? The DCHECK is just meant > to validate that it's running on the file thread, if there's a better way to do > that. I could remove the DCHECK if it's confusion / misused. > > > Instead, the API should look something like: > > > > quirks::RequestIccProfilePath(id, callback) { > > if (!g_manager) > > callback.Run(base::FilePath()); > > g_manager->RequestIccFilePath(id, callback); > > } > > > > And the implementation something like: > > > > QuirksManager::RequestIccFilePath(id, callback) { > > CHECK(thread_checker_.CalledOnValidThread()); > > > > blocking_pool_->PostTaskAndReply( > > Bind(GetIccFilePathOnBlockingPool, id, callback, any other args), > > Bind(QuirksManager::CompleteRequestIccFilePath, weak ptr, id, callback); > > } > > > > QuirksManager::CompleteRequestIccFilePath(id, callback, result) { > > callback.Run(result); > > } > > This seems like it adds to the complexity of the caller, forcing it to implement > a callback, which the current system does not. The callback is currently > optional, only if the system wants to use the downloaded file ASAP. It would > certainly complicate the code in display_color_manager_chromeos.cc. Currently, > we just have > > PostTaskAndReplyWithResult(ParseDisplayProfile, UpdateCalibrationData) > > With the proposed scheme, we'd have to bounce through > > ParseDisplayProfile -> > RequestIccProfilePath -> > GetIccFilePathOnBlockingPool -> > CompleteRequestIccFilePath -> > ParseDisplayProfileCallback -> > UpdateCalibrationData > > I'm not sure how to do this with the PostTaskAndReplyWithResult model. Perhaps we need to discuss this in a meeting, however the fundamental problem for me is that "Request...Path" is not the same as "PathExists": * The name does not suggest to me that it needs to be run from a blocking pool. * The operation can actually be quite complex if the file does not already exist. * As written following the threads is challenging: caller (blocking pool) -> QM::RequestIccFilePath (*QM* blocking pool) -> QM::RunClientOnTaskRunner (client task runner) -> on_download_finished (client task runner?) It is particularly confusing that RequestIccProfilePath is expected to be called from the *same blocking pool* that is passed into QM(), and that on_download_finished is called on *client task runner*. Perhaps it would help all of us if you put together a doc that showed the relationship between the various methods and the threads / task runners they are expected to be run on? https://codereview.chromium.org/1528963002/diff/440001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/440001/components/quirks/quir... components/quirks/quirks_manager.h:88: // Switch to fake URLFetcher creator for tests. Clarify comment - this sounds like it is only for tests.
https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:47: // Must be called on file thread. On 2016/02/17 23:01:51, Greg Levin wrote: > Ok, this is just my incomplete understanding of the blocking pool system, but... > > > Requiring this to be called on the file thread is confusing and adds > unnecessary > > complexity to the caller. > > I'm not sure where the confusion comes from. This function obviously needs to > run on the file thread, as it's looking for a file. Similarly, I would assume > that any function calling it would also be running on the file thread, as the > next step after calling it is to open the requested file. I see it as a more > sophisticated version of PathExists(). You call it from the file thread because > you know it needs to run on the file thread, and very likely you're about to use > the file. The fact that the QM will go off on its own and get the file later if > it's not found isn't a detail that the caller needs to be aware of. > > > It also makes the assumption that the "file thread" is > > the same as blocking_pool_ which is confusing. > > Is this assumption just in > DCHECK(blocking_pool()->RunsTasksOnCurrentThread()), > or is it being made somewhere else that I'm not seeing? The DCHECK is just meant > to validate that it's running on the file thread, if there's a better way to do > that. I could remove the DCHECK if it's confusion / misused. > > > Instead, the API should look something like: > > > > quirks::RequestIccProfilePath(id, callback) { > > if (!g_manager) > > callback.Run(base::FilePath()); > > g_manager->RequestIccFilePath(id, callback); > > } > > > > And the implementation something like: > > > > QuirksManager::RequestIccFilePath(id, callback) { > > CHECK(thread_checker_.CalledOnValidThread()); > > > > blocking_pool_->PostTaskAndReply( > > Bind(GetIccFilePathOnBlockingPool, id, callback, any other args), > > Bind(QuirksManager::CompleteRequestIccFilePath, weak ptr, id, callback); > > } > > > > QuirksManager::CompleteRequestIccFilePath(id, callback, result) { > > callback.Run(result); > > } > > This seems like it adds to the complexity of the caller, forcing it to implement > a callback, which the current system does not. The callback is currently > optional, only if the system wants to use the downloaded file ASAP. It would > certainly complicate the code in display_color_manager_chromeos.cc. Currently, > we just have > > PostTaskAndReplyWithResult(ParseDisplayProfile, UpdateCalibrationData) > > With the proposed scheme, we'd have to bounce through > > ParseDisplayProfile -> > RequestIccProfilePath -> > GetIccFilePathOnBlockingPool -> > CompleteRequestIccFilePath -> > ParseDisplayProfileCallback -> > UpdateCalibrationData > > I'm not sure how to do this with the PostTaskAndReplyWithResult model. You are correct that this doesn't really work with PostTaskAndReply(WithResult), but that's just a convenience for running a _synchronous_ method on a background thread. You can get the same behavior with continuation-passing style: just pass a callback and a task runner to ParseDisplayProfile. And yes, this would make the callback non-optional, but I'd consider that a feature 😃 -- it would allow you to use the same callback regardless of where the file came from (i.e. it would already address the TODO you have in there).
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/460001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Getting close. https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:115: on_finished_for_test_.Run(); This is awkward. A cleaner way to do this is to make this method a protected virtual and override it in a derived class in the test. That way we avoid test code in production code. https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:141: on_finished_for_test_.Run(); ditto. The derived DisplayColorManagerForTest class could override both of these. https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... ash/display/display_color_manager_chromeos.h:55: void FinishLoadCalibrationForDisplay(int64_t display_id, Does this need to be public? Public methods should always be commented. https://codereview.chromium.org/1528963002/diff/480001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.h (right): https://codereview.chromium.org/1528963002/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.h:23: // Quirks Client provided them. These comments should be in quirks_manager.h in the delegate definition. Any implementation specific comments should be in the .cc file. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_client.cc:42: VLOG(1) << "Failed to write: " << file_path.value() << ", err = " << errno; Should this ever happen? If not, LOG(ERROR). (If, e.g., this is expected to fail on a linux-chromeos build, then we should check for that explicitly and do something different). https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_client.h:18: // TODO: Can this path reference disappear during the callback? No. If a reference is bound to a callback using Bind, it will actually be copied. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:47: delegate->CheckDaysSinceOobe(); Calling into a delegate from the blocking pool is unusual and error prone. This could be done before calling this. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:52: base::FilePath path = delegate->GetBuiltInDisplayProfileDirectory().Append( delegate->GetBuiltInDisplayProfileDirectory() could be passed to this. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:65: path = delegate->GetDownloadDisplayProfileDirectory().Append( delegate->GetDownloadDisplayProfileDirectory() could be passed to this. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:177: if (it != clients_.end()) Use CHECK or DCHECK when something should be logically impossible, and then skip any logical test. In this case, since accessing an invalid iterator could be dangerous, use CHECK and remove the if(). https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:201: DCHECK(base::ThreadTaskRunnerHandle::IsSet()); nit: Put this first so that it is easier to find. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:222: if (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks) This is running on the ui thread, not the blocking pool, so we would be calling into the delegate from multiple threads, which could be a problem if the member was being written to from the blocking pool when this is called. It is cleaner/safer to make all calls to a delegate_ from a single thread (usually the construction thread where the delegate is passed in unless otherwise necessary and clearly documented). https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.h:57: class Delegate : public base::RefCounted<Delegate> { Why do we need to refcount the delegate? That's very unusual, please explain in the comment if necessary. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.h:58: public: Delegate methods should be commented here.
stevenjb, here are some changes and answers to your questions. Please take a look! https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:41: DCHECK(base::PathExists(path)); // Quirks should ensure this. On 2016/02/17 23:38:54, stevenjb wrote: > On 2016/02/17 23:01:50, Greg Levin wrote: > > On 2016/02/16 20:07:51, stevenjb wrote: > > > Don't do file operations here. > > > > Done. Why not? > > Then we have to guarantee that this runs on a blocking pool task runner when > there is really no need for that (see other comments). > Acknowledged. https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quir... components/quirks/quirks_manager.cc:47: // Must be called on file thread. On 2016/02/18 12:17:07, Bernhard Bauer wrote: > On 2016/02/17 23:01:51, Greg Levin wrote: > > Ok, this is just my incomplete understanding of the blocking pool system, > but... > > > > > Requiring this to be called on the file thread is confusing and adds > > unnecessary > > > complexity to the caller. > > > > I'm not sure where the confusion comes from. This function obviously needs to > > run on the file thread, as it's looking for a file. Similarly, I would assume > > that any function calling it would also be running on the file thread, as the > > next step after calling it is to open the requested file. I see it as a more > > sophisticated version of PathExists(). You call it from the file thread > because > > you know it needs to run on the file thread, and very likely you're about to > use > > the file. The fact that the QM will go off on its own and get the file later > if > > it's not found isn't a detail that the caller needs to be aware of. > > > > > It also makes the assumption that the "file thread" is > > > the same as blocking_pool_ which is confusing. > > > > Is this assumption just in > > DCHECK(blocking_pool()->RunsTasksOnCurrentThread()), > > or is it being made somewhere else that I'm not seeing? The DCHECK is just > meant > > to validate that it's running on the file thread, if there's a better way to > do > > that. I could remove the DCHECK if it's confusion / misused. > > > > > Instead, the API should look something like: > > > > > > quirks::RequestIccProfilePath(id, callback) { > > > if (!g_manager) > > > callback.Run(base::FilePath()); > > > g_manager->RequestIccFilePath(id, callback); > > > } > > > > > > And the implementation something like: > > > > > > QuirksManager::RequestIccFilePath(id, callback) { > > > CHECK(thread_checker_.CalledOnValidThread()); > > > > > > blocking_pool_->PostTaskAndReply( > > > Bind(GetIccFilePathOnBlockingPool, id, callback, any other args), > > > Bind(QuirksManager::CompleteRequestIccFilePath, weak ptr, id, callback); > > > } > > > > > > QuirksManager::CompleteRequestIccFilePath(id, callback, result) { > > > callback.Run(result); > > > } > > > > This seems like it adds to the complexity of the caller, forcing it to > implement > > a callback, which the current system does not. The callback is currently > > optional, only if the system wants to use the downloaded file ASAP. It would > > certainly complicate the code in display_color_manager_chromeos.cc. > Currently, > > we just have > > > > PostTaskAndReplyWithResult(ParseDisplayProfile, UpdateCalibrationData) > > > > With the proposed scheme, we'd have to bounce through > > > > ParseDisplayProfile -> > > RequestIccProfilePath -> > > GetIccFilePathOnBlockingPool -> > > CompleteRequestIccFilePath -> > > ParseDisplayProfileCallback -> > > UpdateCalibrationData > > > > I'm not sure how to do this with the PostTaskAndReplyWithResult model. > > You are correct that this doesn't really work with PostTaskAndReply(WithResult), > but that's just a convenience for running a _synchronous_ method on a background > thread. You can get the same behavior with continuation-passing style: just pass > a callback and a task runner to ParseDisplayProfile. And yes, this would make > the callback non-optional, but I'd consider that a feature 😃 -- it would allow > you to use the same callback regardless of where the file came from (i.e. it > would already address the TODO you have in there). Done. Based on stevenjb's input, I refactored QM so that RequestIccProfilePath() is called from the ui thread with a mandatory callback. https://codereview.chromium.org/1528963002/diff/440001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/440001/components/quirks/quir... components/quirks/quirks_manager.h:88: // Switch to fake URLFetcher creator for tests. On 2016/02/17 23:38:55, stevenjb wrote: > Clarify comment - this sounds like it is only for tests. Done. https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:115: on_finished_for_test_.Run(); On 2016/02/29 20:22:29, stevenjb wrote: > This is awkward. A cleaner way to do this is to make this method a protected > virtual and override it in a derived class in the test. That way we avoid test > code in production code. Done. https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:141: on_finished_for_test_.Run(); On 2016/02/29 20:22:29, stevenjb wrote: > ditto. The derived DisplayColorManagerForTest class could override both of > these. Done. https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_co... ash/display/display_color_manager_chromeos.h:55: void FinishLoadCalibrationForDisplay(int64_t display_id, On 2016/02/29 20:22:29, stevenjb wrote: > Does this need to be public? Public methods should always be commented. Made private. https://codereview.chromium.org/1528963002/diff/480001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.h (right): https://codereview.chromium.org/1528963002/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.h:23: // Quirks Client provided them. On 2016/02/29 20:22:29, stevenjb wrote: > These comments should be in quirks_manager.h in the delegate definition. Any > implementation specific comments should be in the .cc file. Done. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_client.cc:42: VLOG(1) << "Failed to write: " << file_path.value() << ", err = " << errno; On 2016/02/29 20:22:29, stevenjb wrote: > Should this ever happen? If not, LOG(ERROR). (If, e.g., this is expected to fail > on a linux-chromeos build, then we should check for that explicitly and do > something different). Changed. (I don't know of any cases where this would be expected to fail.) https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_client.h:18: // TODO: Can this path reference disappear during the callback? On 2016/02/29 20:22:29, stevenjb wrote: > No. If a reference is bound to a callback using Bind, it will actually be > copied. Acknowledged, thanks! https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:47: delegate->CheckDaysSinceOobe(); On 2016/02/29 20:22:29, stevenjb wrote: > Calling into a delegate from the blocking pool is unusual and error prone. > This could be done before calling this. CheckDaysSinceOobe() calls StartupUtils::GetTimeSinceOobeFlagFileCreation(), which has to run on a file thread. And the delegate is currently the only way to call this function since it lives in chrome/browser/etc. Would deriving the delegate from RefCountedThreadSafe be any better? https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:52: base::FilePath path = delegate->GetBuiltInDisplayProfileDirectory().Append( On 2016/02/29 20:22:29, stevenjb wrote: > delegate->GetBuiltInDisplayProfileDirectory() could be passed to this. Yeah, I had briefly done it that way. But I couldn't figure out any other way of calling StartupUtils::GetTimeSinceOobeFlagFileCreation() except by passing the delegate into the blocking pool, so I just left these next two as they are. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:65: path = delegate->GetDownloadDisplayProfileDirectory().Append( On 2016/02/29 20:22:29, stevenjb wrote: > delegate->GetDownloadDisplayProfileDirectory() could be passed to this. Acknowledged. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:177: if (it != clients_.end()) On 2016/02/29 20:22:29, stevenjb wrote: > Use CHECK or DCHECK when something should be logically impossible, and then skip > any logical test. In this case, since accessing an invalid iterator could be > dangerous, use CHECK and remove the if(). Done. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:201: DCHECK(base::ThreadTaskRunnerHandle::IsSet()); On 2016/02/29 20:22:29, stevenjb wrote: > nit: Put this first so that it is easier to find. Done. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:222: if (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks) On 2016/02/29 20:22:29, stevenjb wrote: > This is running on the ui thread, not the blocking pool, so we would be calling > into the delegate from multiple threads, which could be a problem if the member > was being written to from the blocking pool when this is called. It is > cleaner/safer to make all calls to a delegate_ from a single thread (usually the > construction thread where the delegate is passed in unless otherwise necessary > and clearly documented). As noted above, CheckDaysSinceOobe() can only run from the blocking pool. CheckDaysSinceOobe() is only called from within CheckForIccFileAndNewDevice(), GetDaysSinceOobe() is only called from within OnIccFilePathRequestCompleted(), and calling order is enforced by PostTaskAndReplyWithResult( &CheckForIccFileAndNewDevice, &OnIccFilePathRequestCompleted). I don't believe there is any practical thread safety issue here (tho I could be mistaken), but of course am open to suggestions for improvement! I could return a custom struct with { path, days_since_oobe }, but caching days_since_oobe seemed easier, and it could potentially be used by multiple clients, and only needs to be read once. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.h:57: class Delegate : public base::RefCounted<Delegate> { On 2016/02/29 20:22:29, stevenjb wrote: > Why do we need to refcount the delegate? That's very unusual, please explain in > the comment if necessary. CheckForIccFileAndNewDevice() needs the delegate to access code in chrome/browser/etc. But I didn't want the QM, and thus the delegate, to be destroyed before/during CheckForIccFileAndNewDevice(). My intention was to have the delegate arg hold a reference for as long as CheckForIccFileAndNewDevice() needed it, since the delegate doesn't actually depend on QM still being alive. In short, I felt QM should share ownership of the delegate with this function. Does that make any sense? Is there a better way to do this? https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.h:58: public: On 2016/02/29 20:22:29, stevenjb wrote: > Delegate methods should be commented here. Done.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:222: if (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks) On 2016/03/02 00:00:26, Greg Levin wrote: > On 2016/02/29 20:22:29, stevenjb wrote: > > This is running on the ui thread, not the blocking pool, so we would be > calling > > into the delegate from multiple threads, which could be a problem if the > member > > was being written to from the blocking pool when this is called. It is > > cleaner/safer to make all calls to a delegate_ from a single thread (usually > the > > construction thread where the delegate is passed in unless otherwise necessary > > and clearly documented). > > As noted above, CheckDaysSinceOobe() can only run from the blocking pool. > > CheckDaysSinceOobe() is only called from within CheckForIccFileAndNewDevice(), > GetDaysSinceOobe() is only called from within OnIccFilePathRequestCompleted(), > and calling order is enforced by > PostTaskAndReplyWithResult( > &CheckForIccFileAndNewDevice, &OnIccFilePathRequestCompleted). > I don't believe there is any practical thread safety issue here (tho I could be > mistaken), but of course am open to suggestions for improvement! > > I could return a custom struct with { path, days_since_oobe }, but caching > days_since_oobe seemed easier, and it could potentially be used by multiple > clients, and only needs to be read once. This is similar to the problem we had before. We have an interface (the delegate) where we are making assumptions about which methods get called from which thread with no documentation. i.e. some methods are expected to be called from the main/UI thread and some from a blocking pool. And there is a thread safety issue here. It seems unlikely, but there isn't any protection against CheckDaysSinceOobe() and GetDaysSinceOobe() from being called at the same time on different threads, and they both access days_since_oobe_. There are a couple of ways we can solve this: 1) Document the expectations of the delegate methods and make the delegate itself tread safe (including using ThreadSafeRefCounted). I dislike this because it would be easy to miss a thread safety issue or race, especially down the road. 2) Change and simplify the delegate interface so that it can be called exclusively from the main/UI thread: Get*Directory - these are trivial/safe from the Ui thread. Check/GetDaysSinceOobe() - Eliminate Check and make Get asynchronous. It posts the file io test (if necessary) to a blocking pool and invokes the callback on the main/UI thread. The callback handles the remaining logic int his function. I realize it may seem more complex to add blocking pool logic to the delegate, but keeping the delegate interface simple and robust is more important than reducing the complexity of the delegate implementation. https://codereview.chromium.org/1528963002/diff/500001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:54: chromeos::StartupUtils::GetTimeSinceOobeFlagFileCreation().InDays(); If Chrome remains running for a long time (e.g. weeks), this will never update. Instead we should cache the oobe flag file creation time and compare that to Now() in GetDaysSinceOobe(). https://codereview.chromium.org/1528963002/diff/500001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/500001/components/quirks/quir... components/quirks/quirks_manager.h:59: virtual std::string GetApiKey() const = 0; Used? Needs documentation.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/520001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/540001
Here's the latest draft... please have another look! https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:47: delegate->CheckDaysSinceOobe(); On 2016/02/29 20:22:29, stevenjb wrote: > Calling into a delegate from the blocking pool is unusual and error prone. This > could be done before calling this. UPDATE: After a small refactor, delegate::GetDaysSinceOobe() is called from ui thread (with callback provided), then the delegate makes an internal call to blocking pool for file access. This is done later, only when it's clear that device age will be needed. https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:52: base::FilePath path = delegate->GetBuiltInDisplayProfileDirectory().Append( On 2016/02/29 20:22:29, stevenjb wrote: > delegate->GetBuiltInDisplayProfileDirectory() could be passed to this. UPDATE: Now both file paths (including file name) are passed into CheckForIccFile(). https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:201: DCHECK(base::ThreadTaskRunnerHandle::IsSet()); On 2016/02/29 20:22:29, stevenjb wrote: > nit: Put this first so that it is easier to find. UPDATE (FYI): NeedToCheckServer() is now gone, code has been split between OnIccFilePathRequestCompleted() and OnDaysSinceOobeGotten(). https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.cc:222: if (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks) On 2016/03/02 17:51:08, stevenjb wrote: > On 2016/03/02 00:00:26, Greg Levin wrote: > > On 2016/02/29 20:22:29, stevenjb wrote: > > > This is running on the ui thread, not the blocking pool, so we would be > > calling > > > into the delegate from multiple threads, which could be a problem if the > > member > > > was being written to from the blocking pool when this is called. It is > > > cleaner/safer to make all calls to a delegate_ from a single thread (usually > > the > > > construction thread where the delegate is passed in unless otherwise > necessary > > > and clearly documented). > > > > As noted above, CheckDaysSinceOobe() can only run from the blocking pool. > > > > CheckDaysSinceOobe() is only called from within CheckForIccFileAndNewDevice(), > > GetDaysSinceOobe() is only called from within OnIccFilePathRequestCompleted(), > > and calling order is enforced by > > PostTaskAndReplyWithResult( > > &CheckForIccFileAndNewDevice, &OnIccFilePathRequestCompleted). > > I don't believe there is any practical thread safety issue here (tho I could > be > > mistaken), but of course am open to suggestions for improvement! > > > > I could return a custom struct with { path, days_since_oobe }, but caching > > days_since_oobe seemed easier, and it could potentially be used by multiple > > clients, and only needs to be read once. > > This is similar to the problem we had before. We have an interface (the > delegate) where we are making assumptions about which methods get called from > which thread with no documentation. i.e. some methods are expected to be called > from the main/UI thread and some from a blocking pool. > > And there is a thread safety issue here. It seems unlikely, but there isn't any > protection against CheckDaysSinceOobe() and GetDaysSinceOobe() from being called > at the same time on different threads, and they both access days_since_oobe_. > > There are a couple of ways we can solve this: > 1) Document the expectations of the delegate methods and make the delegate > itself tread safe (including using ThreadSafeRefCounted). I dislike this because > it would be easy to miss a thread safety issue or race, especially down the > road. > > 2) Change and simplify the delegate interface so that it can be called > exclusively from the main/UI thread: > Get*Directory - these are trivial/safe from the Ui thread. > Check/GetDaysSinceOobe() - Eliminate Check and make Get asynchronous. It posts > the file io test (if necessary) to a blocking pool and invokes the callback on > the main/UI thread. The callback handles the remaining logic int his function. > > I realize it may seem more complex to add blocking pool logic to the delegate, > but keeping the delegate interface simple and robust is more important than > reducing the complexity of the delegate implementation. Done, went with (2). https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quir... components/quirks/quirks_manager.h:57: class Delegate : public base::RefCounted<Delegate> { On 2016/03/02 00:00:26, Greg Levin wrote: > On 2016/02/29 20:22:29, stevenjb wrote: > > Why do we need to refcount the delegate? That's very unusual, please explain > in > > the comment if necessary. > > CheckForIccFileAndNewDevice() needs the delegate to access code in > chrome/browser/etc. But I didn't want the QM, and thus the delegate, to be > destroyed before/during CheckForIccFileAndNewDevice(). My intention was to have > the delegate arg hold a reference for as long as CheckForIccFileAndNewDevice() > needed it, since the delegate doesn't actually depend on QM still being alive. > In short, I felt QM should share ownership of the delegate with this function. > > Does that make any sense? Is there a better way to do this? UPDATE: Delegate no longer passed to CheckForIccFile(), so RefCounted has been removed. https://codereview.chromium.org/1528963002/diff/500001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:54: chromeos::StartupUtils::GetTimeSinceOobeFlagFileCreation().InDays(); On 2016/03/02 17:51:08, stevenjb wrote: > If Chrome remains running for a long time (e.g. weeks), this will never update. > Instead we should cache the oobe flag file creation time and compare that to > Now() in GetDaysSinceOobe(). Value is no longer cached by delegate. https://codereview.chromium.org/1528963002/diff/500001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/500001/components/quirks/quir... components/quirks/quirks_manager.h:59: virtual std::string GetApiKey() const = 0; On 2016/03/02 17:51:08, stevenjb wrote: > Used? Needs documentation. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
robert.bradford@intel.com changed reviewers: + robert.bradford@intel.com
I think the flow looks good. Obviously it clashes with my CL under review but I think the rebase either way should be straightforward. https://codereview.chromium.org/1528963002/diff/540001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (left): https://codereview.chromium.org/1528963002/diff/540001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:28: #include "ui/gfx/screen.h" Is it right to trim all these #includes? Last time I checked the coding style it recommended including what you need and not assuming it came in through transitivity. It's possible that some of these may not be needed / should be possibly replaced by forward declarations.
https://codereview.chromium.org/1528963002/diff/540001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (left): https://codereview.chromium.org/1528963002/diff/540001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:28: #include "ui/gfx/screen.h" On 2016/03/08 16:29:24, robert.bradford wrote: > Is it right to trim all these #includes? Last time I checked the coding style it > recommended including what you need and not assuming it came in through > transitivity. > > It's possible that some of these may not be needed / should be possibly replaced > by forward declarations. +1. bind.h e.g. is explicitly used and should be included, along with anything else that a) is used in this file and b) is not in the header directly associated with this file. https://codereview.chromium.org/1528963002/diff/540001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/540001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:55: base::PostTaskAndReplyWithResult(blocking_pool.get(), FROM_HERE, No need to pass blocking_pool to this. There is no requirement that this get called on a specific blocking pool, and this is implemented in src/chrome (i.e. not in a component), so just use content::BrowserThread::GetBlockingPool() here. https://codereview.chromium.org/1528963002/diff/540001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/540001/components/quirks/quir... components/quirks/quirks_manager.cc:110: g_manager = new QuirksManager(delegate.release(), blocking_pool, local_state, Pass this to QuirksManager() as a scoped_ptr to make the ownership transfer more clear. https://codereview.chromium.org/1528963002/diff/540001/components/quirks/quir... components/quirks/quirks_manager.cc:182: // TODO(glevin): Eventually we'll want to check the server for updates even nit: s/Eventually we'll want to check/Check/ https://codereview.chromium.org/1528963002/diff/540001/components/quirks/quir... components/quirks/quirks_manager.cc:194: blocking_pool_, base::Bind(&QuirksManager::OnDaysSinceOobeGotten, nit: "Gotten"? Perhaps just "OnGetDaysSinceOob". (or 'RequestDays...' + 'OnDays...Received').
I delayed all downloads until after a login, in order to respect a forthcoming Quirks-disabling device policy (see Question 15 in go/cros-quirks-client-dd and updated control flow diagram for details). Please have another look! https://codereview.chromium.org/1528963002/diff/540001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (left): https://codereview.chromium.org/1528963002/diff/540001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:28: #include "ui/gfx/screen.h" On 2016/03/08 22:38:06, stevenjb wrote: > On 2016/03/08 16:29:24, robert.bradford wrote: > > Is it right to trim all these #includes? Last time I checked the coding style > it > > recommended including what you need and not assuming it came in through > > transitivity. > > > > It's possible that some of these may not be needed / should be possibly > replaced > > by forward declarations. > > +1. bind.h e.g. is explicitly used and should be included, along with anything > else that a) is used in this file and b) is not in the header directly > associated with this file. Whoops, thanks guys. Looks like I was a little overzealous in my war on #includes. A lot of stuff really wasn't relevant anymore, largely because of code moved into quirks. Hopefully this current list is reflective of what's currently in the file. https://codereview.chromium.org/1528963002/diff/540001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/540001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:55: base::PostTaskAndReplyWithResult(blocking_pool.get(), FROM_HERE, On 2016/03/08 22:38:06, stevenjb wrote: > No need to pass blocking_pool to this. There is no requirement that this get > called on a specific blocking pool, and this is implemented in src/chrome (i.e. > not in a component), so just use content::BrowserThread::GetBlockingPool() here. Done. https://codereview.chromium.org/1528963002/diff/540001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/540001/components/quirks/quir... components/quirks/quirks_manager.cc:110: g_manager = new QuirksManager(delegate.release(), blocking_pool, local_state, On 2016/03/08 22:38:06, stevenjb wrote: > Pass this to QuirksManager() as a scoped_ptr to make the ownership transfer more > clear. Done. https://codereview.chromium.org/1528963002/diff/540001/components/quirks/quir... components/quirks/quirks_manager.cc:182: // TODO(glevin): Eventually we'll want to check the server for updates even On 2016/03/08 22:38:06, stevenjb wrote: > nit: s/Eventually we'll want to check/Check/ Done. https://codereview.chromium.org/1528963002/diff/540001/components/quirks/quir... components/quirks/quirks_manager.cc:194: blocking_pool_, base::Bind(&QuirksManager::OnDaysSinceOobeGotten, On 2016/03/08 22:38:06, stevenjb wrote: > nit: "Gotten"? Perhaps just "OnGetDaysSinceOob". (or 'RequestDays...' + > 'OnDays...Received'). > Gotten is a perfectly cromulent word. But ok, went with OnDaysSinceOobeReceived() (the present tense versions just didn't sound right :-P)
https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quir... components/quirks/quirks_manager.cc:131: for (; iter != g_manager->clients_.end(); ++iter) This should work: for (client : g_manager->clients_) client->StartDownload(); Otherwise you could do this: for (auto iter = g_manager->clients_.begin(); ...) https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quir... components/quirks/quirks_manager.h:86: static void OnLoginCompleted(); // Start queued downloads after login. I think this would be a little more clear as a member function instead of a static. i.e. QuirksManager::Get()->OnLoginCompleted(); This assumes that QuirksManager will always be initialized, which I believe is logically correct? If not, it would be more clear to do something like: if (QuirksManager::IsInitialized()) QuirksManager::Get()->OnLoginCompleted(); with a comment describing the circumstances under which it might not be initialized. This will make it clear that it might not be initialized due to, e.g., a command line flag and not to a timing issue. As the code is currently written, if g_manager is null this will quietly fail. One reason to put this logic into the caller is that the calling code may get moved or re-factored at some point. We want to make sure that doesn't break any assumptions.
Please have another look! https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quir... components/quirks/quirks_manager.cc:131: for (; iter != g_manager->clients_.end(); ++iter) On 2016/03/10 19:29:31, stevenjb wrote: > This should work: > > for (client : g_manager->clients_) > client->StartDownload(); > > Otherwise you could do this: > > for (auto iter = g_manager->clients_.begin(); ...) Done. https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quir... components/quirks/quirks_manager.h:86: static void OnLoginCompleted(); // Start queued downloads after login. On 2016/03/10 19:29:31, stevenjb wrote: > I think this would be a little more clear as a member function instead of a > static. i.e. > > QuirksManager::Get()->OnLoginCompleted(); > > This assumes that QuirksManager will always be initialized, which I believe is > logically correct? If not, it would be more clear to do something like: > > if (QuirksManager::IsInitialized()) > QuirksManager::Get()->OnLoginCompleted(); > > with a comment describing the circumstances under which it might not be > initialized. This will make it clear that it might not be initialized due to, > e.g., a command line flag and not to a timing issue. > > As the code is currently written, if g_manager is null this will quietly fail. > > One reason to put this logic into the caller is that the calling code may get > moved or re-factored at some point. We want to make sure that doesn't break any > assumptions. > > Done. Also made RequestIccProfilePath() a member. As best I can tell, g_manager can't be NULL while ChromeBrowser's main message loop is running (at least, under any non-horrible circumstances).
I don't see a new patch?
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/580001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #29 (id:600001) has been deleted
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/620001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/03/15 23:12:58, stevenjb wrote: > I don't see a new patch? D'oh! I *started* to upload it (serves me right for skipping my afternoon caffeine). Uploaded now, along with another Merge patch.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/640001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/640001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/660001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/660001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks, this is looking a lot nicer! https://codereview.chromium.org/1528963002/diff/660001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/660001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:129: base::Bind(&DisplayColorManager::UpdateCalibrationData, AsWeakPtr(), Nit: If you only need a WeakPtr to yourself, you can have a WeakPtrFactory as a member, so you don't expose it to clients. https://codereview.chromium.org/1528963002/diff/660001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_browsertest.cc (right): https://codereview.chromium.org/1528963002/diff/660001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:87: weak_ptr_factory_.GetWeakPtr())); You could in principle use base::Unretained(this) here, because you know that this object will stay alive until the callback is called. https://codereview.chromium.org/1528963002/diff/660001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/660001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:23: } Nit: empty line before closing the namespace, and maybe // namespace https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... File components/quirks/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... components/quirks/quirks_client.cc:40: base::ThreadRestrictions::AssertIOAllowed(); Doesn't base::WriteFile already do this? Basically, the idea with thread restrictions is that they're checked as far down the call stack as possible, i.e. right before we call into code we don't control anymore. https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... components/quirks/quirks_client.h:22: QuirksManager* manager); You could forward-declare QuirksManager in the header instead of including it. https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... components/quirks/quirks_manager.cc:163: const std::string& url, Pass this as a GURL? https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... components/quirks/quirks_manager.cc:175: DCHECK(base::ThreadTaskRunnerHandle::IsSet()); As I have previously mentioned, this is not the right way to check thread safety. In almost all circumstances you want either thread affinity (everything has to happen on the same thread) or at least sequence affinity (prohibit concurrent access), which you would do with a ThreadChecker or a SequenceChecker, respectively. In this particular case, PrefService is single-threaded, so all accesses to it have to happen on the same thread (which in principle could be any thread, because PrefService doesn't know about the UI thread in Chrome, but in practice always is the UI thread). ThreadTaskRunnerHandle::IsSet() just checks that there is *a* MessageLoop for the current thread, but not that it's the same. Similary for QuirksClient -- it needs sequence affinity, although it will be used from the same (single) thread that this class will be used on, so it could use either a SequenceChecker or a ThreadChecker.
Please have another look! https://codereview.chromium.org/1528963002/diff/660001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/660001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:129: base::Bind(&DisplayColorManager::UpdateCalibrationData, AsWeakPtr(), On 2016/03/17 17:42:25, Bernhard Bauer wrote: > Nit: If you only need a WeakPtr to yourself, you can have a WeakPtrFactory as a > member, so you don't expose it to clients. Done. https://codereview.chromium.org/1528963002/diff/660001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_browsertest.cc (right): https://codereview.chromium.org/1528963002/diff/660001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:87: weak_ptr_factory_.GetWeakPtr())); On 2016/03/17 17:42:25, Bernhard Bauer wrote: > You could in principle use base::Unretained(this) here, because you know that > this object will stay alive until the callback is called. Done. https://codereview.chromium.org/1528963002/diff/660001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/660001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:23: } On 2016/03/17 17:42:25, Bernhard Bauer wrote: > Nit: empty line before closing the namespace, and maybe // namespace Ah, thanks! git cl format had told me to remove the blank line before I put the // namespace there, but now it's fine with it :-) https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... File components/quirks/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... components/quirks/quirks_client.cc:40: base::ThreadRestrictions::AssertIOAllowed(); On 2016/03/17 17:42:25, Bernhard Bauer wrote: > Doesn't base::WriteFile already do this? Basically, the idea with thread > restrictions is that they're checked as far down the call stack as possible, > i.e. right before we call into code we don't control anymore. Got it, thanks. I wasn't sure how much they were programmatic checks, and how much they were just annotations. https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... components/quirks/quirks_client.h:22: QuirksManager* manager); On 2016/03/17 17:42:25, Bernhard Bauer wrote: > You could forward-declare QuirksManager in the header instead of including it. Done. (I included the header here for RequestFinishedCallback, but "using" it in both headers seems to work.) https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... components/quirks/quirks_manager.cc:163: const std::string& url, On 2016/03/17 17:42:26, Bernhard Bauer wrote: > Pass this as a GURL? Done. https://codereview.chromium.org/1528963002/diff/660001/components/quirks/quir... components/quirks/quirks_manager.cc:175: DCHECK(base::ThreadTaskRunnerHandle::IsSet()); On 2016/03/17 17:42:26, Bernhard Bauer wrote: > As I have previously mentioned, this is not the right way to check thread > safety. In almost all circumstances you want either thread affinity (everything > has to happen on the same thread) or at least sequence affinity (prohibit > concurrent access), which you would do with a ThreadChecker or a > SequenceChecker, respectively. > > In this particular case, PrefService is single-threaded, so all accesses to it > have to happen on the same thread (which in principle could be any thread, > because PrefService doesn't know about the UI thread in Chrome, but in practice > always is the UI thread). ThreadTaskRunnerHandle::IsSet() just checks that there > is *a* MessageLoop for the current thread, but not that it's the same. > > Similary for QuirksClient -- it needs sequence affinity, although it will be > used from the same (single) thread that this class will be used on, so it could > use either a SequenceChecker or a ThreadChecker. I put this here during development, for annotation / debugging purposes, and just wanted the same thread validity check that the pref code was doing. There were no concurrency concerns. Does this look correct now? Is it even desirable, given that the pref code is doing the same thing one level down?
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/680001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/680001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:101: const ui::DisplaySnapshot* display) { nit: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI) (These help document how threads are used here) https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:118: bool file_downloaded) { nit: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI) https://codereview.chromium.org/1528963002/diff/680001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/680001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:36: PathService::Get(chromeos::DIR_DEVICE_COLOR_CALIBRATION_PROFILES, &path)); This will fail on a release build since DCHECK() is optimized out entirely. Use if (!PathService::Get(...)) LOG(ERROR) instead. https://codereview.chromium.org/1528963002/diff/680001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:49: directory = directory.Append("display_profiles"); "display_profiles" should be defined as a const in the anon namespace at the top of this file. https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... components/quirks/quirks_client.h:41: // Schedules retry. nit: 'a retry' https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... components/quirks/quirks_client.h:44: // Translate json with base64-encoded data (|result|) into raw |data|. Translates https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... components/quirks/quirks_manager.h:149: std::set<scoped_ptr<QuirksClient>> clients_; nit: Document this
adding net to components/quirks/DEPS lgtm
Please have another look! https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:101: const ui::DisplaySnapshot* display) { On 2016/03/21 17:43:59, stevenjb wrote: > nit: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI) > > (These help document how threads are used here) Done. https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:118: bool file_downloaded) { On 2016/03/21 17:43:59, stevenjb wrote: > nit: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI) Done. https://codereview.chromium.org/1528963002/diff/680001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/680001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:36: PathService::Get(chromeos::DIR_DEVICE_COLOR_CALIBRATION_PROFILES, &path)); On 2016/03/21 17:43:59, stevenjb wrote: > This will fail on a release build since DCHECK() is optimized out entirely. Use > if (!PathService::Get(...)) LOG(ERROR) instead. Ugh, thanks. Don't know what brain fart caused me to put a DCHECK here >_< https://codereview.chromium.org/1528963002/diff/680001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:49: directory = directory.Append("display_profiles"); On 2016/03/21 17:43:59, stevenjb wrote: > "display_profiles" should be defined as a const in the anon namespace at the top > of this file. > Done. https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... components/quirks/quirks_client.h:41: // Schedules retry. On 2016/03/21 17:43:59, stevenjb wrote: > nit: 'a retry' Done. https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... components/quirks/quirks_client.h:44: // Translate json with base64-encoded data (|result|) into raw |data|. On 2016/03/21 17:43:59, stevenjb wrote: > Translates Done. https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/680001/components/quirks/quir... components/quirks/quirks_manager.h:149: std::set<scoped_ptr<QuirksClient>> clients_; On 2016/03/21 17:43:59, stevenjb wrote: > nit: Document this Done.
lgtm!
Thanks! LGTM with a final nit: https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI); Nit: Put the expected value first, for nicer error messages in case of failure.
I have a thread checking question... https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI); On 2016/03/22 09:58:05, Bernhard Bauer wrote: > Nit: Put the expected value first, for nicer error messages in case of failure. Done, but... Is this the right check to be doing here? There's only one other place that I could find that TYPE_UI was being DCHECKed, and that was in actual thread code: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/auto... Should I just remove these, or replace them with a more standard check?
https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI); On 2016/03/22 17:49:29, Greg Levin wrote: > On 2016/03/22 09:58:05, Bernhard Bauer wrote: > > Nit: Put the expected value first, for nicer error messages in case of > failure. > > Done, but... > Is this the right check to be doing here? There's only one other place that I > could find that TYPE_UI was being DCHECKed, and that was in actual thread code: > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/auto... > > Should I just remove these, or replace them with a more standard check? Hm... I would probably require that this class is used on a single thread, so you'd use a ThreadChecker like in QuirksManager. Right now you are saying that you require the method to be called on a thread that has a message loop suitable for running UI, and no platform does UI on more than one thread (right?), but I don't think MessageLoopForUI enforces that, i.e. you could totally use a MessageLoop that _can_ run UI on a background thread (you just wouldn't use UI), and given that QuirksManager is single-threaded and there is only one instance of it, this method can really only be called on a thread where QuirksManager can be used, so it's effectively single-threaded as well.
looks good. Please update the description to have a bit more details, and the BUG=. https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::TYPE_UI, base::MessageLoop::current()->type()); This may not be UI message loop in mustash. If you want to verify the thread, use thread checker. https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos.h:62: scoped_ptr<ColorCalibrationData> data); why virtual? https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos_unittest.cc:122: scoped_ptr<quirks::QuirksManager::Delegate>( make_scoepd_ptr https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:404: scoped_ptr<quirks::QuirksManager::Delegate>( ditto https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_browsertest.cc (right): https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:54: QuirksBrowserTest() : file_existed_(false) {} or initialize inline. up to you. https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:61: // called in ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun(). Can this be in SetUpOnMainThread? https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:108: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:22: base::ThreadRestrictions::AssertIOAllowed(); I thought BlockingPool is allowed to access file system. No? https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_client.h:19: using RequestFinishedCallback = document parameters. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_client.h:32: int64_t product_id() { return product_id_; } const https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.cc:29: QuirksManager* g_manager = nullptr; g_ is used typically for glocal, and I usually avoid for non global (this is file scoped). https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.cc:149: void QuirksManager::ClientFinished(QuirksClient* client) { DCHECK(thread_checker_.CalledOnValidThread()); ? https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.cc:237: const RequestFinishedCallback& on_request_finished) { DCHECK(thread_checker_.CalledOnValidThread()); ? https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.h:78: DISALLOW_ASSIGN(Delegate); FYI: you can omit this for pure interface https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.h:106: Delegate* delegate() const { return delegate_.get(); } remove const, as you're returning non const internal state (hence it'll allow a client to change the internal state of the object). same for the rest.
Description was changed from
==========
Creating a "Quirks Client" to download icc files and other display info
("quirks") from a Quirks Server. These provide display-specific tuning (e.g.
gamma ramps) on a per-monitor basis. For more info, see
go/cros-quirks-client-dd.
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server,
check that file is downloaded to local directory. More details to
follow....
==========
to
==========
Creating a "Quirks Client" to download icc files and other display info
("quirks") from a Quirks Server. These provide display-specific tuning (e.g.
gamma ramps) on a per-monitor basis.
Each Quirks Client handles downloading and writing a single file. The single
Quirks Manager handles external requests for file paths, creates clients when
downloads are needed, and manages their life cycle. For more info, see
go/cros-quirks-client-dd (particularly the "Code Design" section).
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server,
check that file is downloaded to /var/cache/display_profiles. At next startup,
the gamma correction in the icc file should be applied to the display (this will
only be visible to the degree that the gamma correction is large enough to be
noticeable; the correct functioning of the Quirks Client is primarily determined
by the appearance of the file).
==========
https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI); On 2016/03/22 17:49:29, Greg Levin wrote: > On 2016/03/22 09:58:05, Bernhard Bauer wrote: > > Nit: Put the expected value first, for nicer error messages in case of > failure. > > Done, but... > Is this the right check to be doing here? There's only one other place that I > could find that TYPE_UI was being DCHECKed, and that was in actual thread code: > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/auto... > > Should I just remove these, or replace them with a more standard check? Yeah, I think that is probably not the best test. I would suggest using a thread_checker_ instance here just like with QuirksManager. https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::TYPE_UI, base::MessageLoop::current()->type()); On 2016/03/22 20:27:21, oshima wrote: > This may not be UI message loop in mustash. > > If you want to verify the thread, use thread checker. +1. ThreadChecker (like you are using in QuirksManager) definitely seems like the best way to do this. Go ahead and replace all such checks in this file.
Please have another look! https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI); On 2016/03/22 21:29:49, stevenjb wrote: > On 2016/03/22 17:49:29, Greg Levin wrote: > > On 2016/03/22 09:58:05, Bernhard Bauer wrote: > > > Nit: Put the expected value first, for nicer error messages in case of > > failure. > > > > Done, but... > > Is this the right check to be doing here? There's only one other place that I > > could find that TYPE_UI was being DCHECKed, and that was in actual thread > code: > > > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/auto... > > > > Should I just remove these, or replace them with a more standard check? > > Yeah, I think that is probably not the best test. I would suggest using a > thread_checker_ instance here just like with QuirksManager. Done. https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::TYPE_UI, base::MessageLoop::current()->type()); On 2016/03/22 20:27:21, oshima wrote: > This may not be UI message loop in mustash. > > If you want to verify the thread, use thread checker. Done. https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos.h:62: scoped_ptr<ColorCalibrationData> data); On 2016/03/22 20:27:21, oshima wrote: > why virtual? The DCM unit test needs to add some code to these functions to kill the test's wait loop. See https://codereview.chromium.org/1528963002/diff2/480001:500001/ash/display/di... https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos_unittest.cc:122: scoped_ptr<quirks::QuirksManager::Delegate>( On 2016/03/22 20:27:21, oshima wrote: > make_scoepd_ptr Initialize() is expecting a scoped_ptr<quirks::QuirksManager::Delegate>, but make_scoped_ptr creates a scoped_ptr<QuirksManagerDelegateTestImpl>, which causes a "calling a private destructor" error. Discussed in person, acknowledged as fine as is. (Also see https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeo...) https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:404: scoped_ptr<quirks::QuirksManager::Delegate>( On 2016/03/22 20:27:21, oshima wrote: > ditto Same as https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_browsertest.cc (right): https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:54: QuirksBrowserTest() : file_existed_(false) {} On 2016/03/22 20:27:21, oshima wrote: > or initialize inline. up to you. Acknowledged. https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:61: // called in ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun(). On 2016/03/22 20:27:21, oshima wrote: > Can this be in SetUpOnMainThread? Done. https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_browsertest.cc:108: }; On 2016/03/22 20:27:21, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... File chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc (right): https://codereview.chromium.org/1528963002/diff/720001/chrome/browser/chromeo... chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc:22: base::ThreadRestrictions::AssertIOAllowed(); On 2016/03/22 20:27:21, oshima wrote: > I thought BlockingPool is allowed to access file system. No? Yeah, as far as I know. This should've been deleted based on a prior comment anyway... thanks for catching. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... File components/quirks/quirks_client.h (right): https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_client.h:19: using RequestFinishedCallback = On 2016/03/22 20:27:21, oshima wrote: > document parameters. Done (in different file, as noted). https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_client.h:32: int64_t product_id() { return product_id_; } On 2016/03/22 20:27:21, oshima wrote: > const Done. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.cc:29: QuirksManager* g_manager = nullptr; On 2016/03/22 20:27:21, oshima wrote: > g_ is used typically for glocal, and I usually avoid for non global (this > is file scoped). I'll fix this if you have a strong preference, but to me this seems clearer. It being in an anonymous namespace makes it clear that it's only file-global, and the style guide says to use g_ to distinguish from local variables (which it's not (?)). Further, there are numerous examples of this usage in the Chrome code. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.cc:149: void QuirksManager::ClientFinished(QuirksClient* client) { On 2016/03/22 20:27:21, oshima wrote: > DCHECK(thread_checker_.CalledOnValidThread()); > > ? Done. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.cc:237: const RequestFinishedCallback& on_request_finished) { On 2016/03/22 20:27:21, oshima wrote: > DCHECK(thread_checker_.CalledOnValidThread()); > > ? Done. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... File components/quirks/quirks_manager.h (right): https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.h:78: DISALLOW_ASSIGN(Delegate); On 2016/03/22 20:27:21, oshima wrote: > FYI: you can omit this for pure interface Acknowledged :-) https://codereview.chromium.org/1528963002/diff/340001/components/quirks_clie... and https://codereview.chromium.org/1528963002/diff/220001/components/quirks_clie... https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.h:106: Delegate* delegate() const { return delegate_.get(); } On 2016/03/22 20:27:21, oshima wrote: > remove const, as you're returning non const internal state (hence it'll allow a > client to change the internal state of the object). same for the rest. Done.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/740001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/740001
Thread checker changes are definitely better, thanks. Still looks good to me. https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:12: #include "base/message_loop/message_loop.h" nit: still needed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm leaving the office now, but I'll look it again today. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.cc:29: QuirksManager* g_manager = nullptr; On 2016/03/23 00:16:18, Greg Levin wrote: > On 2016/03/22 20:27:21, oshima wrote: > > g_ is used typically for glocal, and I usually avoid for non global (this > > is file scoped). > > I'll fix this if you have a strong preference, but to me this seems clearer. It > being in an anonymous namespace makes it clear that it's only file-global, and > the style guide says to use g_ to distinguish from local variables (which it's > not (?)). This one (https://google.github.io/styleguide/cppguide.html#Variable_Names) is for global variable (and this is not), and there is no suggestion for file scoped variable. One issue is that it gives wrong impress that that's global, even though it's not in global namespace. For example: file a: namespace { g_something } file b: namespace { g_something } are totally different variable, and it's less obvious when you're navigating / searching for the code. The benefit of having g_ only for global namespace is that it'll be unique. (or link will fail) > Further, there are numerous examples of this usage in the Chrome > code. I know, and I'm suggesting to avoid it for the above reason when I saw it. It's not big deal, so if you want to keep it, then fine.
lgtm https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos.h:62: scoped_ptr<ColorCalibrationData> data); On 2016/03/23 00:16:18, Greg Levin wrote: > On 2016/03/22 20:27:21, oshima wrote: > > why virtual? > > The DCM unit test needs to add some code to these functions to kill the test's > wait loop. See > https://codereview.chromium.org/1528963002/diff2/480001:500001/ash/display/di... It looks to me that you can simply wait until the message loop is idle, but I'm fine as is. https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_co... File ash/display/display_color_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_co... ash/display/display_color_manager_chromeos_unittest.cc:50: on_finished_for_test_.Run(); nit: It may be safer to reset after Run. I'll leave it to you.
Patchset #36 (id:760001) has been deleted
https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos.h:62: scoped_ptr<ColorCalibrationData> data); On 2016/03/23 09:06:48, oshima wrote: > On 2016/03/23 00:16:18, Greg Levin wrote: > > On 2016/03/22 20:27:21, oshima wrote: > > > why virtual? > > > > The DCM unit test needs to add some code to these functions to kill the test's > > wait loop. See > > > https://codereview.chromium.org/1528963002/diff2/480001:500001/ash/display/di... > > It looks to me that you can simply wait until the message loop is idle, but I'm > fine as is. I may be misremembering, but I seem to recall QuitWhenIdle() causing problems when the msg loop went idle while blocking pool was doing work. In any case, this is pretty simple, and definitely quits exactly when I want. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quir... components/quirks/quirks_manager.cc:29: QuirksManager* g_manager = nullptr; On 2016/03/23 02:13:27, oshima wrote: > On 2016/03/23 00:16:18, Greg Levin wrote: > > On 2016/03/22 20:27:21, oshima wrote: > > > g_ is used typically for glocal, and I usually avoid for non global (this > > > is file scoped). > > > > I'll fix this if you have a strong preference, but to me this seems clearer. > It > > being in an anonymous namespace makes it clear that it's only file-global, and > > the style guide says to use g_ to distinguish from local variables (which it's > > not (?)). > > This one (https://google.github.io/styleguide/cppguide.html#Variable_Names) is > for global variable (and this is not), and there is no suggestion for file > scoped variable. > > One issue is that it gives wrong impress that that's global, even though it's > not in global namespace. For example: > > file a: > namespace { > g_something > } > > file b: > namespace { > g_something > } > > are totally different variable, and it's less obvious when you're navigating / > searching for the code. > The benefit of having g_ only for global namespace is that it'll be unique. (or > link will fail) > > > Further, there are numerous examples of this usage in the Chrome > > code. > > I know, and I'm suggesting to avoid it for the above reason when I saw it. > It's not big deal, so if you want to keep it, then fine. > > Well, I wanted *something* to distinguish it from a local var, so I went with manager_, as per the example of |detail_| here: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-s... https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_co... File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_co... ash/display/display_color_manager_chromeos.cc:12: #include "base/message_loop/message_loop.h" On 2016/03/23 00:31:41, stevenjb wrote: > nit: still needed? Done. https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_co... File ash/display/display_color_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_co... ash/display/display_color_manager_chromeos_unittest.cc:50: on_finished_for_test_.Run(); On 2016/03/23 09:06:48, oshima wrote: > nit: It may be safer to reset after Run. I'll leave it to you. Done.
ash/display/display_color_manager_chromeos* lgtm I think you should CQ this, let this settle for a while and then i'll rebase my CL on top with the aim to land next week.
Description was changed from
==========
Creating a "Quirks Client" to download icc files and other display info
("quirks") from a Quirks Server. These provide display-specific tuning (e.g.
gamma ramps) on a per-monitor basis.
Each Quirks Client handles downloading and writing a single file. The single
Quirks Manager handles external requests for file paths, creates clients when
downloads are needed, and manages their life cycle. For more info, see
go/cros-quirks-client-dd (particularly the "Code Design" section).
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server,
check that file is downloaded to /var/cache/display_profiles. At next startup,
the gamma correction in the icc file should be applied to the display (this will
only be visible to the degree that the gamma correction is large enough to be
noticeable; the correct functioning of the Quirks Client is primarily determined
by the appearance of the file).
==========
to
==========
Creating a "Quirks Client" to download icc files and other display info
("quirks") from a Quirks Server. These provide display-specific tuning
(e.g. gamma ramps) on a per-monitor basis.
Each Quirks Client handles downloading and writing a single file. The
single Quirks Manager handles external requests for file paths, creates
clients when downloads are needed, and manages their life cycle. For
more info, see go/cros-quirks-client-dd (particularly the "Code Design"
section).
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server, check
that file is downloaded to /var/cache/display_profiles. At next startup,
the gamma correction in the icc file should be applied to the display
(this will only be visible to the degree that the gamma correction is
large enough to be noticeable; the correct functioning of the Quirks
Client is primarily determined by the appearance of the file).
==========
still lgtm https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_co... ash/display/display_color_manager_chromeos.h:62: scoped_ptr<ColorCalibrationData> data); On 2016/03/23 16:09:19, Greg Levin wrote: > On 2016/03/23 09:06:48, oshima wrote: > > On 2016/03/23 00:16:18, Greg Levin wrote: > > > On 2016/03/22 20:27:21, oshima wrote: > > > > why virtual? > > > > > > The DCM unit test needs to add some code to these functions to kill the > test's > > > wait loop. See > > > > > > https://codereview.chromium.org/1528963002/diff2/480001:500001/ash/display/di... > > > > It looks to me that you can simply wait until the message loop is idle, but > I'm > > fine as is. > > I may be misremembering, but I seem to recall QuitWhenIdle() causing problems > when the msg loop went idle while blocking pool was doing work. In any case, > this is pretty simple, and definitely quits exactly when I want. You're right. I misread the code. Please disregard this comment.
The CQ bit was checked by glevin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, blundell@chromium.org, pauljensen@chromium.org, bauerb@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1528963002/#ps780001 (title: "A few small tweaks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528963002/780001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528963002/780001
Message was sent while issue was closed.
Committed patchset #36 (id:780001)
Message was sent while issue was closed.
Description was changed from
==========
Creating a "Quirks Client" to download icc files and other display info
("quirks") from a Quirks Server. These provide display-specific tuning
(e.g. gamma ramps) on a per-monitor basis.
Each Quirks Client handles downloading and writing a single file. The
single Quirks Manager handles external requests for file paths, creates
clients when downloads are needed, and manages their life cycle. For
more info, see go/cros-quirks-client-dd (particularly the "Code Design"
section).
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server, check
that file is downloaded to /var/cache/display_profiles. At next startup,
the gamma correction in the icc file should be applied to the display
(this will only be visible to the degree that the gamma correction is
large enough to be noticeable; the correct functioning of the Quirks
Client is primarily determined by the appearance of the file).
==========
to
==========
Creating a "Quirks Client" to download icc files and other display info
("quirks") from a Quirks Server. These provide display-specific tuning
(e.g. gamma ramps) on a per-monitor basis.
Each Quirks Client handles downloading and writing a single file. The
single Quirks Manager handles external requests for file paths, creates
clients when downloads are needed, and manages their life cycle. For
more info, see go/cros-quirks-client-dd (particularly the "Code Design"
section).
BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server, check
that file is downloaded to /var/cache/display_profiles. At next startup,
the gamma correction in the icc file should be applied to the display
(this will only be visible to the degree that the gamma correction is
large enough to be noticeable; the correct functioning of the Quirks
Client is primarily determined by the appearance of the file).
Committed: https://crrev.com/5dd01a7d4eecdf094fad69b07040fdf9f7ce9721
Cr-Commit-Position: refs/heads/master@{#382962}
==========
Message was sent while issue was closed.
Patchset 36 (id:??) landed as https://crrev.com/5dd01a7d4eecdf094fad69b07040fdf9f7ce9721 Cr-Commit-Position: refs/heads/master@{#382962} |
