Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2943843002: Convert to CupsPrintJobManagerImpl to TaskScheduler. (Closed)

Created:
1 year, 11 months ago by skau
Modified:
1 year, 10 months ago
Reviewers:
gab, Carlson
CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert to CupsPrintJobManagerImpl to TaskScheduler. Place status polling on a sequenced task runner. Run Cancel as a user visible task. BUG=734280, 667892, 689520 Review-Url: https://codereview.chromium.org/2943843002 Cr-Commit-Position: refs/heads/master@{#483394} Committed: https://chromium.googlesource.com/chromium/src/+/bab2208a02c09baa03cb7cffd1bc3f6c996fa04c

Patch Set 1 #

Patch Set 2 : add an io assertion #

Total comments: 6

Patch Set 3 : ready #

Patch Set 4 : notification must happen on the browser thread #

Total comments: 2

Patch Set 5 : somewhat reasonable #

Total comments: 12

Patch Set 6 : as weak ptr #

Total comments: 4

Patch Set 7 : format #

Total comments: 8

Patch Set 8 : strong pointers and DCHECKs #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -94 lines) Patch
M chrome/browser/chromeos/printing/cups_print_job_manager_impl.h View 1 2 3 4 5 6 7 4 chunks +29 lines, -12 lines 0 comments Download
M chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc View 1 2 3 4 5 6 7 10 chunks +116 lines, -82 lines 5 comments Download

Messages

Total messages: 24 (10 generated)
skau
Please review. Migrating to TaskScheduler. https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (left): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc#oldcode304 chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:304: content::BrowserThread::FILE_USER_BLOCKING, FROM_HERE, This was ...
1 year, 11 months ago (2017-06-20 01:24:17 UTC) #2
Carlson
https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (left): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc#oldcode197 chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:197: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Are you removing the requirement, or just the ...
1 year, 11 months ago (2017-06-20 19:02:09 UTC) #3
gab
lgtm (only potential issue I see is with pre-existing race) https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc#newcode210 ...
1 year, 11 months ago (2017-06-20 21:04:49 UTC) #4
skau
I've been running this code under debug and found a sequencing bug IRT WeakPtrs. I've ...
1 year, 11 months ago (2017-06-24 01:03:45 UTC) #6
Carlson
https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc#newcode219 chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:219: DETACH_FROM_SEQUENCE(sequence_checker_); I don't understand what this does (which is ...
1 year, 10 months ago (2017-06-26 18:42:02 UTC) #7
skau
Addressed most of the comments. Trying to find a solution to the possible use-after-destroy bug. ...
1 year, 10 months ago (2017-06-26 23:15:17 UTC) #8
gab
https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.h File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.h#newcode57 chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:57: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.h#newcode110 chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:110: CupsWrapper cups_wrapper_; To ensure this ...
1 year, 10 months ago (2017-06-27 17:44:18 UTC) #9
skau
Thanks! PTAL. https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.h File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.h#newcode57 chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:57: }; On 2017/06/27 17:44:18, gab wrote: > ...
1 year, 10 months ago (2017-06-27 20:25:41 UTC) #10
gab
https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc#newcode258 chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:258: cups_wrapper_(new CupsWrapper(), base::MakeUnique<CupsWrapper>() https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.h File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.h#newcode41 chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:41: ...
1 year, 10 months ago (2017-06-28 15:55:58 UTC) #11
skau
Thanks for the review. It should be correct now. https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc#newcode258 chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:258: ...
1 year, 10 months ago (2017-06-28 23:59:49 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2943843002/140001
1 year, 10 months ago (2017-06-29 16:55:46 UTC) #19
commit-bot: I haz the power
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/bab2208a02c09baa03cb7cffd1bc3f6c996fa04c
1 year, 10 months ago (2017-06-29 17:00:58 UTC) #22
gab
nits on last patch set https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc#newcode360 chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:360: ->PostDelayedTask(FROM_HERE, BrowserThread::PostDelayedTask(BrowserThread::UI, ...) https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc#newcode360 ...
1 year, 10 months ago (2017-06-29 17:12:20 UTC) #23
skau
1 year, 10 months ago (2017-07-05 22:14:32 UTC) #24
Message was sent while issue was closed.
Addressed nits in https://codereview.chromium.org/2968063002

https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right):

https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:360:
->PostDelayedTask(FROM_HERE,
On 2017/06/29 17:12:20, gab (in-out til july17) wrote:
> BrowserThread::PostDelayedTask(BrowserThread::UI, ...)

Done.

https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:379: // Runs a
query on query_runner_ which will rejoin this sequnece on
On 2017/06/29 17:12:20, gab (in-out til july17) wrote:
> |query_runner_|

Done.

Powered by Google App Engine
This is Rietveld 408576698