Adding an IO Data and keyed service to previews/

Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication model will be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087
Committed: https://crrev.com/41832e5474cea7ddeafefac5872134408fdfefdc
Cr-Commit-Position: refs/heads/master@{#417668}

[RyanSturm, 2016-09-01 22:19:10]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 22:19:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-01 22:27:58]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 22:28:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 22:51:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 22:51:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[RyanSturm, 2016-09-01 23:08:40]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 23:09:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 00:21:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 00:21:05]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-09-02 18:13:29]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 18:13:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-02 18:32:55]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 18:33:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-02 18:33:54]

ryansturm@chromium.org changed reviewers:
+ bengr@chromium.org

[RyanSturm, 2016-09-02 18:33:55]

bengr: PTAL @ *

[RyanSturm, 2016-09-02 18:37:21]

Description was changed from

==========
Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication modle will be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087
==========

to

==========
Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication mode lwill be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087
==========

[RyanSturm, 2016-09-02 18:37:31]

Description was changed from

==========
Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication mode lwill be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087
==========

to

==========
Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication model will be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087
==========

[commit-bot: I haz the power, 2016-09-02 20:12:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 20:12:41]

Dry run: This issue passed the CQ dry run.

[bengr, 2016-09-08 00:19:10]

lgtm with requests for comments and nits.

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
File chrome/browser/previews/previews_service.cc (right):

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:18: void
PreviewsService::set_previews_ui_service(
You should be able to inline both the getter and setter.

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
File chrome/browser/previews/previews_service.h (right):

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_service.h:17: // UI service which owns a
previews::PreviewsUIService.
// Keyed service that owns a previews::PreviewsUIService.

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_service.h:23: // The UI thread portion of
previews/ inter-thread communication.
nit: UI-thread

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
File chrome/browser/previews/previews_service_factory.h (right):

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_service_factory.h:19: class
PreviewsServiceFactory : public BrowserContextKeyedServiceFactory {
Add class comment and comments for public methods.

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:963: previews_io_data_ =
std::move(previews_io_data);
inline? It looks like the method before it could be inlined too.

https://codereview.chromium.org/2300213002/diff/80001/components/previews/pre...
File components/previews/previews_io_data.h (right):

https://codereview.chromium.org/2300213002/diff/80001/components/previews/pre...
components/previews/previews_io_data.h:35: // a weak pointer to |this|.
Virtualized in testing.
in -> for

[RyanSturm, 2016-09-08 18:30:45]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:31:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-08 18:34:32]

ryansturm@chromium.org changed reviewers:
+ mmenke@chromium.org, thestig@chromium.org

[RyanSturm, 2016-09-08 18:34:33]

mmenke: PTAL @ profile*_io_data.*
thestig: PTAL @ chrome/browser/previews/* (and BUILD.GN)

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
File chrome/browser/previews/previews_service.cc (right):

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_service.cc:18: void
PreviewsService::set_previews_ui_service(
On 2016/09/08 00:19:09, bengr wrote:
> You should be able to inline both the getter and setter.

I'm not so sure about the setter, it would at least need to know that
PreviewsUIService has a public destructor, which forward declaring makes
uncertain, so I'll only move the getter.

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
File chrome/browser/previews/previews_service.h (right):

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_service.h:17: // UI service which owns a
previews::PreviewsUIService.
On 2016/09/08 00:19:09, bengr wrote:
> // Keyed service that owns a previews::PreviewsUIService.

Done.

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_service.h:23: // The UI thread portion of
previews/ inter-thread communication.
On 2016/09/08 00:19:09, bengr wrote:
> nit: UI-thread

Done.

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
File chrome/browser/previews/previews_service_factory.h (right):

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_service_factory.h:19: class
PreviewsServiceFactory : public BrowserContextKeyedServiceFactory {
On 2016/09/08 00:19:09, bengr wrote:
> Add class comment and comments for public methods.

Done.

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2300213002/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:963: previews_io_data_ =
std::move(previews_io_data);
On 2016/09/08 00:19:09, bengr wrote:
> inline? It looks like the method before it could be inlined too.

With std::move or reset, you can't only have a forward declared class since the
destructor could be protected/private. I'm pretty sure this wouldn't compile
inlined.

https://codereview.chromium.org/2300213002/diff/80001/components/previews/pre...
File components/previews/previews_io_data.h (right):

https://codereview.chromium.org/2300213002/diff/80001/components/previews/pre...
components/previews/previews_io_data.h:35: // a weak pointer to |this|.
Virtualized in testing.
On 2016/09/08 00:19:09, bengr wrote:
> in -> for

Done.

[mmenke, 2016-09-08 18:44:54]

"previews/" is lo-fi mode?

[RyanSturm, 2016-09-08 18:47:04]

On 2016/09/08 18:44:54, mmenke wrote:
> "previews/" is lo-fi mode?

They are a class of client-side optimizations including offline previews
(serving offline pages on slow networks), client-side LoFi (which uses image
range requests) and others.

[Lei Zhang, 2016-09-08 18:49:37]

Do you think previews will be confused with print preview? 

Can you add OWNERS for the new directories? You can use "file://path/to/OWNERS"
syntax to have one OWNERS file defer to another.

[RyanSturm, 2016-09-08 18:52:50]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[RyanSturm, 2016-09-08 18:53:05]

On 2016/09/08 18:49:37, Lei Zhang wrote:
> Do you think previews will be confused with print preview? 
> 
> Can you add OWNERS for the new directories? You can use
"file://path/to/OWNERS"
> syntax to have one OWNERS file defer to another.

Done.

[commit-bot: I haz the power, 2016-09-08 18:53:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-09-08 18:54:03]

On 2016/09/08 18:53:05, Ryan Sturm wrote:
> On 2016/09/08 18:49:37, Lei Zhang wrote:
> > Do you think previews will be confused with print preview? 
> >

^

> > Can you add OWNERS for the new directories? You can use
> "file://path/to/OWNERS"
> > syntax to have one OWNERS file defer to another.
> 
> Done.

You forgot to add components/previews/OWNERS.

[RyanSturm, 2016-09-08 18:56:41]

On 2016/09/08 18:54:03, Lei Zhang wrote:
> On 2016/09/08 18:53:05, Ryan Sturm wrote:
> > On 2016/09/08 18:49:37, Lei Zhang wrote:
> > > Do you think previews will be confused with print preview? 
> > >
> 
> ^
> 
> > > Can you add OWNERS for the new directories? You can use
> > "file://path/to/OWNERS"
> > > syntax to have one OWNERS file defer to another.
> > 
> > Done.
> 
> You forgot to add components/previews/OWNERS.

components/previews/OWNERS already exists.

[Lei Zhang, 2016-09-08 18:59:20]

On 2016/09/08 18:56:41, Ryan Sturm wrote:
> components/previews/OWNERS already exists.

Ah, indeed. Try to add a '\n' to it next time.

[Lei Zhang, 2016-09-08 19:06:23]

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:856: "previews/previews_service.cc",
Just to confirm, this feature is for all platforms? Bug 639087 does not have any
OS flags set.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/OWNERS (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/OWNERS:1: file://components/previews/OWNERS
Ditto:

diff --git a/chrome/browser/previews/OWNERS b/chrome/browser/previews/OWNERS
new file mode 100644
index
0000000000000000000000000000000000000000..424cc367c417050a01694bd35ad90e1a47a7e5a6
--- /dev/null
+++ b/chrome/browser/previews/OWNERS
@@ -0,0 +1 @@
+file://components/previews/OWNERS
\ No newline at end of file

^

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_service.h (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service.h:17: // Keyed service that owns a
previews::PreviewsUIService.
Can you mention "PreviewsService lives on the UI thread" ?

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service.h:23: // The UI-thread portion of
previews/ inter-thread communication.
This comment doesn't make as much sense.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_service_factory.h (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service_factory.h:8: #include
"base/memory/singleton.h"
Please read the top of singleton.h. Can you use a LazyInstance instead?

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service_factory.h:11: class Profile;
Group this with the PreviewService fwrd decl.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:209:
io_data_->set_previews_io_data(base::WrapUnique(new previews::PreviewsIOData(
This can't be base::MakeUnique?

https://codereview.chromium.org/2300213002/diff/120001/components/previews/pr...
File components/previews/previews_io_data.h (right):

https://codereview.chromium.org/2300213002/diff/120001/components/previews/pr...
components/previews/previews_io_data.h:25: const
scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
Flip the params. In content::BrowserThread, UI comes before IO.

[RyanSturm, 2016-09-08 20:16:43]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 20:17:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-09-08 20:19:10]

thestig: PTAL, thanks

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:856: "previews/previews_service.cc",
On 2016/09/08 19:06:23, Lei Zhang wrote:
> Just to confirm, this feature is for all platforms? Bug 639087 does not have
any
> OS flags set.

Some of the previews optimizations are available on all platforms, so yes.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/OWNERS (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/OWNERS:1: file://components/previews/OWNERS
On 2016/09/08 19:06:23, Lei Zhang wrote:
> Ditto:
> 
> diff --git a/chrome/browser/previews/OWNERS b/chrome/browser/previews/OWNERS
> new file mode 100644
> index
>
0000000000000000000000000000000000000000..424cc367c417050a01694bd35ad90e1a47a7e5a6
> --- /dev/null
> +++ b/chrome/browser/previews/OWNERS
> @@ -0,0 +1 @@
> +file://components/previews/OWNERS
> \ No newline at end of file
> 
> ^

Done.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_service.h (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service.h:17: // Keyed service that owns a
previews::PreviewsUIService.
On 2016/09/08 19:06:23, Lei Zhang wrote:
> Can you mention "PreviewsService lives on the UI thread" ?

Done.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service.h:23: // The UI-thread portion of
previews/ inter-thread communication.
On 2016/09/08 19:06:23, Lei Zhang wrote:
> This comment doesn't make as much sense.

Done.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_service_factory.h (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service_factory.h:8: #include
"base/memory/singleton.h"
On 2016/09/08 19:06:23, Lei Zhang wrote:
> Please read the top of singleton.h. Can you use a LazyInstance instead?

Done.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service_factory.h:11: class Profile;
On 2016/09/08 19:06:23, Lei Zhang wrote:
> Group this with the PreviewService fwrd decl.

Done.

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:209:
io_data_->set_previews_io_data(base::WrapUnique(new previews::PreviewsIOData(
On 2016/09/08 19:06:23, Lei Zhang wrote:
> This can't be base::MakeUnique?

Done.

https://codereview.chromium.org/2300213002/diff/120001/components/previews/pr...
File components/previews/previews_io_data.h (right):

https://codereview.chromium.org/2300213002/diff/120001/components/previews/pr...
components/previews/previews_io_data.h:25: const
scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
On 2016/09/08 19:06:23, Lei Zhang wrote:
> Flip the params. In content::BrowserThread, UI comes before IO.

Done.

[commit-bot: I haz the power, 2016-09-08 20:19:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 20:19:18]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Lei Zhang, 2016-09-08 21:31:58]

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_service_factory.h (right):

https://codereview.chromium.org/2300213002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_service_factory.h:8: #include
"base/memory/singleton.h"
On 2016/09/08 20:19:09, Ryan Sturm wrote:
> On 2016/09/08 19:06:23, Lei Zhang wrote:
> > Please read the top of singleton.h. Can you use a LazyInstance instead?
> 
> Done.

Thanks. I realize someone implemented the first one with a Singleton and
everyone copied it...

https://codereview.chromium.org/2300213002/diff/140001/chrome/browser/preview...
File chrome/browser/previews/previews_service_factory.cc (right):

https://codereview.chromium.org/2300213002/diff/140001/chrome/browser/preview...
chrome/browser/previews/previews_service_factory.cc:14: static
base::LazyInstance<PreviewsServiceFactory> g_previews_factory =
no need for static inside anonymous namespace.

https://codereview.chromium.org/2300213002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2300213002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:209:
io_data_->set_previews_io_data(base::WrapUnique(new previews::PreviewsIOData(
MakeUnique here too?

https://codereview.chromium.org/2300213002/diff/140001/components/previews/pr...
File components/previews/previews_io_data.h (right):

https://codereview.chromium.org/2300213002/diff/140001/components/previews/pr...
components/previews/previews_io_data.h:42: // The IO and UI thread task runners.
|ui_task_runner_| is used to post
Flip order here too.

[RyanSturm, 2016-09-08 22:17:29]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[RyanSturm, 2016-09-08 22:18:26]

thestig: PTAL, thanks

https://codereview.chromium.org/2300213002/diff/140001/chrome/browser/preview...
File chrome/browser/previews/previews_service_factory.cc (right):

https://codereview.chromium.org/2300213002/diff/140001/chrome/browser/preview...
chrome/browser/previews/previews_service_factory.cc:14: static
base::LazyInstance<PreviewsServiceFactory> g_previews_factory =
On 2016/09/08 21:31:57, Lei Zhang wrote:
> no need for static inside anonymous namespace.

Done.

https://codereview.chromium.org/2300213002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2300213002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:209:
io_data_->set_previews_io_data(base::WrapUnique(new previews::PreviewsIOData(
On 2016/09/08 21:31:57, Lei Zhang wrote:
> MakeUnique here too?

Done.

https://codereview.chromium.org/2300213002/diff/140001/components/previews/pr...
File components/previews/previews_io_data.h (right):

https://codereview.chromium.org/2300213002/diff/140001/components/previews/pr...
components/previews/previews_io_data.h:42: // The IO and UI thread task runners.
|ui_task_runner_| is used to post
On 2016/09/08 21:31:57, Lei Zhang wrote:
> Flip order here too.

Done.

[commit-bot: I haz the power, 2016-09-08 22:18:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-09-08 22:20:52]

lgtm

[mmenke, 2016-09-08 22:40:27]

profiles/ LGTM

https://codereview.chromium.org/2300213002/diff/160001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2300213002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:215:
BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)));
If this is currently an Android-only feature, as the docs say, should we just
#ifdef it out elsewhere, to decrease binary size?  Or even add a special define
for it - i.e. #if defined(ENABLE_PREVIEWS) or whatever.

[commit-bot: I haz the power, 2016-09-08 23:37:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 23:37:11]

Dry run: This issue passed the CQ dry run.

[bengr, 2016-09-09 19:13:56]

On 2016/09/08 22:40:27, mmenke wrote:
> profiles/ LGTM
> 
>
https://codereview.chromium.org/2300213002/diff/160001/chrome/browser/profile...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
https://codereview.chromium.org/2300213002/diff/160001/chrome/browser/profile...
> chrome/browser/profiles/profile_impl_io_data.cc:215:
> BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)));
> If this is currently an Android-only feature, as the docs say, should we just
> #ifdef it out elsewhere, to decrease binary size?  Or even add a special
define
> for it - i.e. #if defined(ENABLE_PREVIEWS) or whatever.

I think we should leave it in desktop, because a small decrease in binary size
IMHO is easily outweighed by better bot coverage, and easier development and
debugging. If this were a desktop only feature, I might say otherwise, because
binary size is a bigger deal on mobile.

[RyanSturm, 2016-09-09 19:26:49]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-09-09 19:26:50]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2300213002/#ps160001
(title: "thestig comments")

[commit-bot: I haz the power, 2016-09-09 19:27:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 19:34:27]

Description was changed from

==========
Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication model will be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087
==========

to

==========
Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication model will be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087
==========

[commit-bot: I haz the power, 2016-09-09 19:34:28]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-09 19:37:36]

Description was changed from

==========
Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication model will be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087
==========

to

==========
Adding an IO Data and keyed service to previews/

Due to the need to have inter-thread communication for black listing and
other features of previews/, an IO Data object and KeyedService are
introduced. ProfileIOData owns the PreviewsIOData and a new
KeyedService, PreviewsService owns a PreviewsUIService object. The major
implementation remains in previews/, but the KeyedService is exposed to
TabHelpers (chrome/ based WebContentsObservers) and any other UI objects
that might need to access previews/. The IOData object is similarly
accessible on the IO thread, typically by embedding a raw pointer into
objects that need it (e.g., OfflinePageRequestInterceptor,
PreviewsNetworkDelegate).

The inter-thread communication model will be consumed by the previews/
opt-out black list, which will need to access the black list on the IO
thread for synchronous calls and access previews/ on the UI thread with
asynchronous calls.

BUG=639087

Committed: https://crrev.com/41832e5474cea7ddeafefac5872134408fdfefdc
Cr-Commit-Position: refs/heads/master@{#417668}
==========

[commit-bot: I haz the power, 2016-09-09 19:37:37]

Patchset 9 (id:??) landed as
https://crrev.com/41832e5474cea7ddeafefac5872134408fdfefdc
Cr-Commit-Position: refs/heads/master@{#417668}