Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(137)

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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1301 lines, -88 lines) Patch
M ash/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +3 lines, -0 lines 0 comments Download
M ash/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M ash/ash.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +2 lines, -0 lines 0 comments Download
M ash/display/display_color_manager_chromeos.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 3 chunks +18 lines, -9 lines 0 comments Download
M ash/display/display_color_manager_chromeos.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 6 chunks +50 lines, -43 lines 0 comments Download
M ash/display/display_color_manager_chromeos_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 8 chunks +91 lines, -29 lines 0 comments Download
M chrome/app/chromeos_strings.grdp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/chrome_browser_main_chromeos.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +10 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/display/quirks_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +139 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/display/quirks_manager_delegate_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +34 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/display/quirks_manager_delegate_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +62 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/login/session/user_session_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/prefs/browser_prefs.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +3 lines, -1 line 0 comments Download
M chrome/chrome_browser_chromeos.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +1 line, -0 lines 0 comments Download
M chromeos/chromeos_paths.h View 1 2 3 4 5 6 1 chunk +4 lines, -2 lines 0 comments Download
M chromeos/chromeos_paths.cc View 2 chunks +8 lines, -2 lines 0 comments Download
M chromeos/chromeos_pref_names.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chromeos/chromeos_pref_names.cc View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M components/OWNERS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +2 lines, -0 lines 0 comments Download
M components/components.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
A components/quirks.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +38 lines, -0 lines 0 comments Download
A components/quirks/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +27 lines, -0 lines 0 comments Download
A + components/quirks/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -1 line 0 comments Download
A + components/quirks/OWNERS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
A components/quirks/pref_names.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +16 lines, -0 lines 0 comments Download
A components/quirks/pref_names.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +15 lines, -0 lines 0 comments Download
A components/quirks/quirks_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +80 lines, -0 lines 0 comments Download
A components/quirks/quirks_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +171 lines, -0 lines 0 comments Download
A components/quirks/quirks_export.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +29 lines, -0 lines 0 comments Download
A components/quirks/quirks_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +178 lines, -0 lines 0 comments Download
A components/quirks/quirks_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 1 chunk +257 lines, -0 lines 0 comments Download
A components/quirks/switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +20 lines, -0 lines 0 comments Download
A components/quirks/switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +14 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 154 (52 generated)
Greg Levin
This is just a draft to facilitate discussion. It is not ready for submitting, or ...
5 years ago (2015-12-15 23:09:13 UTC) #2
Greg Levin
Moved code from /chromeos to /components, but it stopped working. Uploaded this draft to get ...
4 years, 11 months ago (2016-01-05 18:23:15 UTC) #3
Greg Levin
https://codereview.chromium.org/1528963002/diff/40001/components/quirks_client/quirks_client.cc File components/quirks_client/quirks_client.cc (right): https://codereview.chromium.org/1528963002/diff/40001/components/quirks_client/quirks_client.cc#newcode386 components/quirks_client/quirks_client.cc:386: base::ThreadTaskRunnerHandle::IsSet(); Since moving QC to components/, each of base::MessageLoopForUI::IsCurrent() ...
4 years, 11 months ago (2016-01-11 21:02:55 UTC) #4
Greg Levin
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 ...
4 years, 11 months ago (2016-01-12 17:13:55 UTC) #6
stevenjb
https://codereview.chromium.org/1528963002/diff/80001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/80001/ash/display/display_color_manager_chromeos.cc#newcode153 ash/display/display_color_manager_chromeos.cc:153: blocking_pool_, FROM_HERE, request, I don't understand how this was ...
4 years, 11 months ago (2016-01-12 19:48:34 UTC) #8
commit-bot: I haz the power
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
4 years, 11 months ago (2016-01-26 22:26:20 UTC) #10
commit-bot: I haz the power
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_chromium_gn_compile_dbg/builds/13212) android_clang_dbg_recipe on ...
4 years, 11 months ago (2016-01-26 22:31:04 UTC) #12
commit-bot: I haz the power
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
4 years, 11 months ago (2016-01-27 16:17:35 UTC) #14
commit-bot: I haz the power
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_presubmit/builds/139571)
4 years, 11 months ago (2016-01-27 16:27:59 UTC) #16
commit-bot: I haz the power
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
4 years, 11 months ago (2016-01-27 17:54:48 UTC) #18
commit-bot: I haz the power
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_presubmit/builds/139615)
4 years, 11 months ago (2016-01-27 18:12:15 UTC) #20
Greg Levin
Hey guys. Here's a working and, as far as I can tell, clean draft. There ...
4 years, 11 months ago (2016-01-27 21:26:09 UTC) #21
Greg Levin
Hey guys. Sorry for the change after the review request, but I think this makes ...
4 years, 10 months ago (2016-01-28 18:54:30 UTC) #22
oshima
https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_color_manager_chromeos.cc#newcode37 ash/display/display_color_manager_chromeos.cc:37: quirks_client::QuirksClient::DownloadFinishedCallback* callback = nullptr; do you need this variable? ...
4 years, 10 months ago (2016-01-28 20:16:31 UTC) #23
stevenjb
https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_color_manager_chromeos.cc#newcode45 ash/display/display_color_manager_chromeos.cc:45: << " for display id: " << display_id; Since ...
4 years, 10 months ago (2016-01-28 21:05:59 UTC) #24
Greg Levin
Please have another look! https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/200001/ash/display/display_color_manager_chromeos.cc#newcode37 ash/display/display_color_manager_chromeos.cc:37: quirks_client::QuirksClient::DownloadFinishedCallback* callback = nullptr; On ...
4 years, 10 months ago (2016-02-01 23:17:44 UTC) #25
oshima
https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeos/display/quirks_client_delegate_impl.h File chrome/browser/chromeos/display/quirks_client_delegate_impl.h (right): https://codereview.chromium.org/1528963002/diff/200001/chrome/browser/chromeos/display/quirks_client_delegate_impl.h#newcode21 chrome/browser/chromeos/display/quirks_client_delegate_impl.h:21: int GetDaysSinceOobe() override; On 2016/02/01 23:17:43, Greg Levin wrote: ...
4 years, 10 months ago (2016-02-02 18:29:13 UTC) #26
stevenjb
https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_color_manager_chromeos.cc#newcode20 ash/display/display_color_manager_chromeos.cc:20: typedef scoped_ptr<DisplayColorManager::ColorCalibrationData> ColorDataPtr; typedefing scoped_ptr<> like this is uncommon ...
4 years, 10 months ago (2016-02-02 23:45:52 UTC) #27
Greg Levin
Patch Set 12 adds a browser test, and makes only the needed code changes to ...
4 years, 10 months ago (2016-02-04 20:51:11 UTC) #28
commit-bot: I haz the power
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
4 years, 10 months ago (2016-02-09 00:52:46 UTC) #30
commit-bot: I haz the power
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_arm64_dbg_recipe/builds/19120) android_chromium_gn_compile_dbg on ...
4 years, 10 months ago (2016-02-09 01:21:20 UTC) #32
Greg Levin
Fixed most comments. There are still 3 old ones (as noted in comments in new ...
4 years, 10 months ago (2016-02-09 18:56:39 UTC) #33
commit-bot: I haz the power
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
4 years, 10 months ago (2016-02-09 19:12:53 UTC) #35
commit-bot: I haz the power
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_rel/builds/64069)
4 years, 10 months ago (2016-02-09 19:20:50 UTC) #37
commit-bot: I haz the power
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
4 years, 10 months ago (2016-02-09 23:28:24 UTC) #39
commit-bot: I haz the power
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_presubmit/builds/144480)
4 years, 10 months ago (2016-02-09 23:41:51 UTC) #41
oshima
sorry, I should have responded earlier. In light of mus-t-ash work, it's better to minimize ...
4 years, 10 months ago (2016-02-10 01:34:00 UTC) #42
stevenjb
Sorry, forgot to publish these. Some might be out of date now. https://codereview.chromium.org/1528963002/diff/220001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc ...
4 years, 10 months ago (2016-02-11 00:05:56 UTC) #44
commit-bot: I haz the power
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
4 years, 10 months ago (2016-02-11 13:30:57 UTC) #46
commit-bot: I haz the power
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_presubmit/builds/145119)
4 years, 10 months ago (2016-02-11 13:39:57 UTC) #48
Greg Levin
Need reviews for smaller pieces, please take a look! (For context on this CL, see ...
4 years, 10 months ago (2016-02-11 17:30:25 UTC) #50
pauljensen
On 2016/02/11 17:30:25, Greg Levin wrote: > pauljensen@: > Include various '+net/' in components/quirks_client/DEPS I've ...
4 years, 10 months ago (2016-02-11 18:13:39 UTC) #52
oshima
https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/340001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode384 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/chromeos/display/quirks_client_browsertest.cc File chrome/browser/chromeos/display/quirks_client_browsertest.cc (right): ...
4 years, 10 months ago (2016-02-11 20:05:52 UTC) #53
Ilya Sherman
histograms.xml lgtm
4 years, 10 months ago (2016-02-11 21:17:26 UTC) #54
Greg Levin
On 2016/02/11 18:13:39, pauljensen wrote: > On 2016/02/11 17:30:25, Greg Levin wrote: > > pauljensen@: ...
4 years, 10 months ago (2016-02-12 01:20:47 UTC) #55
Greg Levin
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_color_manager_chromeos.cc ...
4 years, 10 months ago (2016-02-12 04:48:05 UTC) #56
commit-bot: I haz the power
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
4 years, 10 months ago (2016-02-12 05:40:29 UTC) #58
commit-bot: I haz the power
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_presubmit/builds/145533)
4 years, 10 months ago (2016-02-12 05:52:40 UTC) #60
blundell
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_client/DEPS ...
4 years, 10 months ago (2016-02-12 08:48:09 UTC) #61
Bernhard Bauer
Usage of preferences is mostly fine here, but I found some issues with thread safety ...
4 years, 10 months ago (2016-02-12 14:51:22 UTC) #62
Greg Levin
Will address largest three comments from last review in subsequent patch. https://codereview.chromium.org/1528963002/diff/360001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): ...
4 years, 10 months ago (2016-02-15 21:53:53 UTC) #63
Greg Levin
Patch #19 simplifies the naming of Quirks classes and files as follows: Directory quirks_client/ -> ...
4 years, 10 months ago (2016-02-15 23:53:33 UTC) #64
Greg Levin
Moved external access functions RequestIccProfilePath() and IdToHexString() from client to manager file. QuirksClient is no ...
4 years, 10 months ago (2016-02-16 03:14:46 UTC) #65
stevenjb
I agree that there are some issues here with threading that I think can be ...
4 years, 10 months ago (2016-02-16 20:07:52 UTC) #66
Bernhard Bauer
https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quirks_manager.cc File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quirks_manager.cc#newcode47 components/quirks/quirks_manager.cc:47: // Must be called on file thread. On 2016/02/16 ...
4 years, 10 months ago (2016-02-17 17:27:40 UTC) #67
Greg Levin
https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_color_manager_chromeos.cc#newcode41 ash/display/display_color_manager_chromeos.cc:41: DCHECK(base::PathExists(path)); // Quirks should ensure this. On 2016/02/16 20:07:51, ...
4 years, 10 months ago (2016-02-17 23:01:52 UTC) #68
stevenjb
https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_color_manager_chromeos.cc#newcode41 ash/display/display_color_manager_chromeos.cc:41: DCHECK(base::PathExists(path)); // Quirks should ensure this. On 2016/02/17 23:01:50, ...
4 years, 10 months ago (2016-02-17 23:38:55 UTC) #69
Bernhard Bauer
https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quirks_manager.cc File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/420001/components/quirks/quirks_manager.cc#newcode47 components/quirks/quirks_manager.cc:47: // Must be called on file thread. On 2016/02/17 ...
4 years, 10 months ago (2016-02-18 12:17:07 UTC) #70
commit-bot: I haz the power
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
4 years, 10 months ago (2016-02-26 01:31:10 UTC) #72
commit-bot: I haz the power
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_presubmit/builds/150464)
4 years, 10 months ago (2016-02-26 01:48:49 UTC) #74
commit-bot: I haz the power
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
4 years, 10 months ago (2016-02-26 21:38:16 UTC) #76
commit-bot: I haz the power
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_presubmit/builds/150948)
4 years, 10 months ago (2016-02-26 21:54:43 UTC) #78
stevenjb
Getting close. https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/480001/ash/display/display_color_manager_chromeos.cc#newcode115 ash/display/display_color_manager_chromeos.cc:115: on_finished_for_test_.Run(); This is awkward. A cleaner way ...
4 years, 9 months ago (2016-02-29 20:22:30 UTC) #79
Greg Levin
stevenjb, here are some changes and answers to your questions. Please take a look! https://codereview.chromium.org/1528963002/diff/420001/ash/display/display_color_manager_chromeos.cc ...
4 years, 9 months ago (2016-03-02 00:00:27 UTC) #80
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-02 00:02:02 UTC) #82
commit-bot: I haz the power
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_presubmit/builds/152235)
4 years, 9 months ago (2016-03-02 00:16:28 UTC) #84
stevenjb
https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quirks_manager.cc File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quirks_manager.cc#newcode222 components/quirks/quirks_manager.cc:222: if (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks) On 2016/03/02 00:00:26, Greg Levin ...
4 years, 9 months ago (2016-03-02 17:51:08 UTC) #85
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-04 22:22:30 UTC) #87
commit-bot: I haz the power
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_presubmit/builds/153575)
4 years, 9 months ago (2016-03-04 22:37:51 UTC) #89
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-07 16:33:45 UTC) #91
Greg Levin
Here's the latest draft... please have another look! https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quirks_manager.cc File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/480001/components/quirks/quirks_manager.cc#newcode47 components/quirks/quirks_manager.cc:47: delegate->CheckDaysSinceOobe(); ...
4 years, 9 months ago (2016-03-07 16:34:38 UTC) #92
commit-bot: I haz the power
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_presubmit/builds/153888)
4 years, 9 months ago (2016-03-07 16:41:22 UTC) #94
robert.bradford
I think the flow looks good. Obviously it clashes with my CL under review but ...
4 years, 9 months ago (2016-03-08 16:29:24 UTC) #96
stevenjb
https://codereview.chromium.org/1528963002/diff/540001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (left): https://codereview.chromium.org/1528963002/diff/540001/ash/display/display_color_manager_chromeos.cc#oldcode28 ash/display/display_color_manager_chromeos.cc:28: #include "ui/gfx/screen.h" On 2016/03/08 16:29:24, robert.bradford wrote: > Is ...
4 years, 9 months ago (2016-03-08 22:38:06 UTC) #97
Greg Levin
I delayed all downloads until after a login, in order to respect a forthcoming Quirks-disabling ...
4 years, 9 months ago (2016-03-09 22:46:17 UTC) #98
stevenjb
https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quirks_manager.cc File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quirks_manager.cc#newcode131 components/quirks/quirks_manager.cc:131: for (; iter != g_manager->clients_.end(); ++iter) This should work: ...
4 years, 9 months ago (2016-03-10 19:29:31 UTC) #99
Greg Levin
Please have another look! https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quirks_manager.cc File components/quirks/quirks_manager.cc (right): https://codereview.chromium.org/1528963002/diff/560001/components/quirks/quirks_manager.cc#newcode131 components/quirks/quirks_manager.cc:131: for (; iter != g_manager->clients_.end(); ...
4 years, 9 months ago (2016-03-15 18:12:57 UTC) #100
stevenjb
I don't see a new patch?
4 years, 9 months ago (2016-03-15 23:12:58 UTC) #101
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-16 13:37:28 UTC) #103
commit-bot: I haz the power
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_ninja/builds/145440) ios_rel_device_gn on ...
4 years, 9 months ago (2016-03-16 13:39:56 UTC) #105
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-16 18:19:11 UTC) #108
commit-bot: I haz the power
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_presubmit/builds/157664)
4 years, 9 months ago (2016-03-16 18:32:49 UTC) #110
Greg Levin
On 2016/03/15 23:12:58, stevenjb wrote: > I don't see a new patch? D'oh! I *started* ...
4 years, 9 months ago (2016-03-16 19:42:17 UTC) #111
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-16 21:20:03 UTC) #113
commit-bot: I haz the power
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_presubmit/builds/157758)
4 years, 9 months ago (2016-03-16 21:32:50 UTC) #115
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-17 12:59:51 UTC) #117
commit-bot: I haz the power
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_presubmit/builds/158024)
4 years, 9 months ago (2016-03-17 13:10:11 UTC) #119
Bernhard Bauer
Thanks, this is looking a lot nicer! https://codereview.chromium.org/1528963002/diff/660001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/660001/ash/display/display_color_manager_chromeos.cc#newcode129 ash/display/display_color_manager_chromeos.cc:129: base::Bind(&DisplayColorManager::UpdateCalibrationData, AsWeakPtr(), ...
4 years, 9 months ago (2016-03-17 17:42:26 UTC) #120
Greg Levin
Please have another look! https://codereview.chromium.org/1528963002/diff/660001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/660001/ash/display/display_color_manager_chromeos.cc#newcode129 ash/display/display_color_manager_chromeos.cc:129: base::Bind(&DisplayColorManager::UpdateCalibrationData, AsWeakPtr(), On 2016/03/17 17:42:25, ...
4 years, 9 months ago (2016-03-18 00:35:47 UTC) #121
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-18 00:36:48 UTC) #123
commit-bot: I haz the power
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_presubmit/builds/158338)
4 years, 9 months ago (2016-03-18 00:49:07 UTC) #125
stevenjb
https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_color_manager_chromeos.cc#newcode101 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 ...
4 years, 9 months ago (2016-03-21 17:43:59 UTC) #126
pauljensen
adding net to components/quirks/DEPS lgtm
4 years, 9 months ago (2016-03-21 18:21:20 UTC) #127
Greg Levin
Please have another look! https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/680001/ash/display/display_color_manager_chromeos.cc#newcode101 ash/display/display_color_manager_chromeos.cc:101: const ui::DisplaySnapshot* display) { On ...
4 years, 9 months ago (2016-03-21 21:12:45 UTC) #128
stevenjb
lgtm!
4 years, 9 months ago (2016-03-21 21:14:59 UTC) #129
Bernhard Bauer
Thanks! LGTM with a final nit: https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc#newcode102 ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI); Nit: ...
4 years, 9 months ago (2016-03-22 09:58:05 UTC) #130
Greg Levin
I have a thread checking question... https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc#newcode102 ash/display/display_color_manager_chromeos.cc:102: DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI); On ...
4 years, 9 months ago (2016-03-22 17:49:29 UTC) #131
Bernhard Bauer
https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc#newcode102 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: > ...
4 years, 9 months ago (2016-03-22 18:46:34 UTC) #132
oshima
looks good. Please update the description to have a bit more details, and the BUG=. ...
4 years, 9 months ago (2016-03-22 20:27:22 UTC) #133
stevenjb
https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc#newcode102 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: > ...
4 years, 9 months ago (2016-03-22 21:29:49 UTC) #135
Greg Levin
Please have another look! https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc (right): https://codereview.chromium.org/1528963002/diff/700001/ash/display/display_color_manager_chromeos.cc#newcode102 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, ...
4 years, 9 months ago (2016-03-23 00:16:19 UTC) #136
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-23 00:17:25 UTC) #138
stevenjb
Thread checker changes are definitely better, thanks. Still looks good to me. https://codereview.chromium.org/1528963002/diff/740001/ash/display/display_color_manager_chromeos.cc File ash/display/display_color_manager_chromeos.cc ...
4 years, 9 months ago (2016-03-23 00:31:41 UTC) #139
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 9 months ago (2016-03-23 01:35:51 UTC) #141
oshima
I'm leaving the office now, but I'll look it again today. https://codereview.chromium.org/1528963002/diff/720001/components/quirks/quirks_manager.cc File components/quirks/quirks_manager.cc (right): ...
4 years, 9 months ago (2016-03-23 02:13:28 UTC) #142
oshima
lgtm https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_color_manager_chromeos.h File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_color_manager_chromeos.h#newcode62 ash/display/display_color_manager_chromeos.h:62: scoped_ptr<ColorCalibrationData> data); On 2016/03/23 00:16:18, Greg Levin wrote: ...
4 years, 9 months ago (2016-03-23 09:06:48 UTC) #143
Greg Levin
https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_color_manager_chromeos.h File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_color_manager_chromeos.h#newcode62 ash/display/display_color_manager_chromeos.h:62: scoped_ptr<ColorCalibrationData> data); On 2016/03/23 09:06:48, oshima wrote: > On ...
4 years, 9 months ago (2016-03-23 16:09:19 UTC) #145
robert.bradford
ash/display/display_color_manager_chromeos* lgtm I think you should CQ this, let this settle for a while and ...
4 years, 9 months ago (2016-03-23 16:53:41 UTC) #146
oshima
still lgtm https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_color_manager_chromeos.h File ash/display/display_color_manager_chromeos.h (right): https://codereview.chromium.org/1528963002/diff/720001/ash/display/display_color_manager_chromeos.h#newcode62 ash/display/display_color_manager_chromeos.h:62: scoped_ptr<ColorCalibrationData> data); On 2016/03/23 16:09:19, Greg Levin ...
4 years, 9 months ago (2016-03-23 17:10:46 UTC) #148
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-23 21:59:28 UTC) #151
commit-bot: I haz the power
Committed patchset #36 (id:780001)
4 years, 9 months ago (2016-03-23 23:08:24 UTC) #152
commit-bot: I haz the power
4 years, 9 months ago (2016-03-23 23:10:16 UTC) #154
Message was sent while issue was closed.
Patchset 36 (id:??) landed as
https://crrev.com/5dd01a7d4eecdf094fad69b07040fdf9f7ce9721
Cr-Commit-Position: refs/heads/master@{#382962}

Powered by Google App Engine
This is Rietveld 408576698