Fix IsPrecachingAllowed() to check Sync instead of Data Saver.

Fix IsPrecachingAllowed() to check Sync instead of Data Saver.

Fixed PrecacheManager::IsPrecachingAllowed() to check for the ability to sync
tabs rather than to check that Data Saver is enabled. This was used only by
ChromeNetworkDelegate to determine whether to report some request metrics to
UMA. This definition of the function became out-of-date as of commit dd40ab5.

Got rid of the internal IsSyncTabsEnabled() function, and had PrecacheLauncher
call PrecacheManager::IsPrecachingAllowed() instead. This should eliminate this
class of error in the future.

BUG=309216
Committed: https://crrev.com/85f978746ea9ad414b4fc710a99e8f81d55f7a22
Cr-Commit-Position: refs/heads/master@{#335835}

[twifkak, 2015-06-11 02:16:59]

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

[twifkak, 2015-06-11 02:17:00]

This the correct one. Please review this.

[twifkak, 2015-06-12 21:07:57]

On 2015/06/11 02:17:00, twifkak wrote:
> This the correct one. Please review this.

Hold off on this until I make the changes per the thread with chrome-sync-dev.

[bengr, 2015-06-15 19:54:00]

On 2015/06/12 21:07:57, twifkak wrote:
> On 2015/06/11 02:17:00, twifkak wrote:
> > This the correct one. Please review this.
> 
> Hold off on this until I make the changes per the thread with chrome-sync-dev.

Let me know when to take a look.

[twifkak, 2015-06-16 03:12:24]

+maxbogue in case he wants to double-check my sync logic (discussed with
chrome-sync-dev@)

[twifkak, 2015-06-16 03:12:56]

bengr: PTAL.

[maxbogue, 2015-06-16 19:51:26]

maxbogue@chromium.org changed reviewers:
+ maxbogue@chromium.org

[maxbogue, 2015-06-16 19:51:27]

https://codereview.chromium.org/1176253002/diff/40001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1176253002/diff/40001/components/precache/con...
components/precache/content/precache_manager.cc:69: return sync_service_ &&
sync_service_->IsSyncEnabledAndLoggedIn() &&
This logic lgtm. FYI, http://crrev.com/1175243009 has landed so you're going to
have to change this line when you rebase.

[twifkak, 2015-06-16 20:56:42]

https://codereview.chromium.org/1176253002/diff/40001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1176253002/diff/40001/components/precache/con...
components/precache/content/precache_manager.cc:69: return sync_service_ &&
sync_service_->IsSyncEnabledAndLoggedIn() &&
On 2015/06/16 19:51:27, maxbogue wrote:
> This logic lgtm. FYI, http://crrev.com/1175243009 has landed so you're going
to
> have to change this line when you rebase.

Done; thanks for the heads up.

[bengr, 2015-06-17 00:47:24]

lgtm

[twifkak, 2015-06-17 01:02:13]

twifkak@chromium.org changed reviewers:
+ battre@chromium.org, thestig@chromium.org, yfriedman@chromium.org

[twifkak, 2015-06-17 01:02:14]

yfriedman@chromium.org: Please review changes in
chrome/browser/android/precache/precache_launcher.cc

battre@chromium.org: Please review changes in
chrome/browser/net/chrome_network_delegate.cc

thestig@chromium.org: Please review changes in chrome/browser/precache/ (moved
from components/precache/content/)

[Lei Zhang, 2015-06-17 01:24:33]

So is still a reason to have components/precache/content/, which will only have
PrecacheManager. Especially considering its factory is being moved to chrome/ ?

https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:610: 'browser/precache/precache_manager_factory.cc',
Is this actually Android only code? Should we be not building it on desktop
platforms? Feel free to address this comment in another CL.

[Yaron, 2015-06-17 01:56:17]

On 2015/06/17 01:24:33, Lei Zhang wrote:
> So is still a reason to have components/precache/content/, which will only
have
> PrecacheManager. Especially considering its factory is being moved to chrome/
?
> 
>
https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.gypi
> File chrome/chrome_browser.gypi (right):
> 
>
https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.g...
> chrome/chrome_browser.gypi:610:
'browser/precache/precache_manager_factory.cc',
> Is this actually Android only code? Should we be not building it on desktop
> platforms? Feel free to address this comment in another CL.

lgtm

[twifkak, 2015-06-17 03:41:20]

On 2015/06/17 01:24:33, Lei Zhang wrote:
> So is still a reason to have components/precache/content/, which will only
have
> PrecacheManager. Especially considering its factory is being moved to chrome/
?

The majority of precache files lives in components/precache/core/, so it seems
like components/precache/ is the place where code should be put if it can. My
general impression is that code needs a reason NOT to be in a component rather
than the opposite; do I have that wrong? PrecacheManagerFactory is being moved
because it needs access to ProfileSyncServiceFactory. That said, the only
dependent of PrecacheManager is PrecacheManagerFactory, so it should be easy to
move.

[twifkak, 2015-06-17 03:49:04]

https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:610: 'browser/precache/precache_manager_factory.cc',
On 2015/06/17 01:24:33, Lei Zhang wrote:
> Is this actually Android only code? Should we be not building it on desktop
> platforms? Feel free to address this comment in another CL.

Nothing about its implementation is Android-dependent, AFAIK. However, it only
runs on Android. The two places that include precache_manager_factory.h are only
compiled into the Android build:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

This will be true for the foreseeable future -- our experiment is focusing
exclusively on mobile.

[battre, 2015-06-17 11:34:33]

chrome/browser/net/chrome_network_delegate.cc LGTM

[Lei Zhang, 2015-06-17 21:41:52]

https://codereview.chromium.org/1176253002/diff/60001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/1176253002/diff/60001/components/precache/con...
components/precache/content/precache_manager.h:59: explicit
PrecacheManager(content::BrowserContext* browser_context,
nit: no longer explicit

https://codereview.chromium.org/1176253002/diff/60001/components/precache/con...
components/precache/content/precache_manager.h:105: sync_driver::SyncService*
sync_service_;
This can be sync_driver::SyncService* const, since it never changes.

[Lei Zhang, 2015-06-17 21:44:50]

On 2015/06/17 03:41:20, twifkak wrote:
> On 2015/06/17 01:24:33, Lei Zhang wrote:
> > So is still a reason to have components/precache/content/, which will only
> have
> > PrecacheManager. Especially considering its factory is being moved to
chrome/
> ?
> 
> The majority of precache files lives in components/precache/core/, so it seems
> like components/precache/ is the place where code should be put if it can. My
> general impression is that code needs a reason NOT to be in a component rather
> than the opposite; do I have that wrong? PrecacheManagerFactory is being moved
> because it needs access to ProfileSyncServiceFactory. That said, the only
> dependent of PrecacheManager is PrecacheManagerFactory, so it should be easy
to
> move.

Well, I suppose if someone else wants to use PrecacheManager as a component,
they can write their own factory or just instantiate it as they see fit, so I
guess having more bits as a component is better.

lgtm

[Lei Zhang, 2015-06-17 21:46:29]

On 2015/06/17 03:49:04, twifkak wrote:
>
https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.gypi
> File chrome/chrome_browser.gypi (right):
> 
>
https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.g...
> chrome/chrome_browser.gypi:610:
'browser/precache/precache_manager_factory.cc',
> On 2015/06/17 01:24:33, Lei Zhang wrote:
> > Is this actually Android only code? Should we be not building it on desktop
> > platforms? Feel free to address this comment in another CL.
> 
> Nothing about its implementation is Android-dependent, AFAIK. However, it only
> runs on Android. The two places that include precache_manager_factory.h are
only
> compiled into the Android build:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...
> 
> This will be true for the foreseeable future -- our experiment is focusing
> exclusively on mobile.

Right, so if we know it's going to be mobile only for the foreseeable future,
can we stop building it on the desktop? The size impact is likely small, but
1000 paper cuts, yada yada...

[twifkak, 2015-06-17 22:04:57]

On 2015/06/17 21:46:29, Lei Zhang wrote:
> On 2015/06/17 03:49:04, twifkak wrote:
> >
>
https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.gypi
> > File chrome/chrome_browser.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1176253002/diff/60001/chrome/chrome_browser.g...
> > chrome/chrome_browser.gypi:610:
> 'browser/precache/precache_manager_factory.cc',
> > On 2015/06/17 01:24:33, Lei Zhang wrote:
> > > Is this actually Android only code? Should we be not building it on
desktop
> > > platforms? Feel free to address this comment in another CL.
> > 
> > Nothing about its implementation is Android-dependent, AFAIK. However, it
only
> > runs on Android. The two places that include precache_manager_factory.h are
> only
> > compiled into the Android build:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...
> > 
> > This will be true for the foreseeable future -- our experiment is focusing
> > exclusively on mobile.
> 
> Right, so if we know it's going to be mobile only for the foreseeable future,
> can we stop building it on the desktop? The size impact is likely small, but
> 1000 paper cuts, yada yada...

Oh, sorry, I misunderstood -- thought you were asking me to _add_ it to the
desktop build. OK, I will address this in a future CL.

[twifkak, 2015-06-17 22:12:01]

twifkak@chromium.org changed reviewers:
+ zea@chromium.org

[twifkak, 2015-06-17 22:12:02]

zea@chromium.org: Please review changes in components/precache/content/DEPS
(+sync_driver)

https://codereview.chromium.org/1176253002/diff/60001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/1176253002/diff/60001/components/precache/con...
components/precache/content/precache_manager.h:59: explicit
PrecacheManager(content::BrowserContext* browser_context,
On 2015/06/17 21:41:52, Lei Zhang wrote:
> nit: no longer explicit

Done.

https://codereview.chromium.org/1176253002/diff/60001/components/precache/con...
components/precache/content/precache_manager.h:105: sync_driver::SyncService*
sync_service_;
On 2015/06/17 21:41:52, Lei Zhang wrote:
> This can be sync_driver::SyncService* const, since it never changes.

Done (et al.).

[Nicolas Zea, 2015-06-18 18:25:49]

https://codereview.chromium.org/1176253002/diff/80001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1176253002/diff/80001/components/precache/con...
components/precache/content/precache_manager.cc:73:
!sync_service_->GetEncryptedDataTypes().Has(syncer::SESSIONS);
One correction (which I think was overlooked in the email):
GetEncryptedDataTypes will return the minimal set (just passwords) if sync's
backend hasn't initialized yet, and will also not take into account any pending
configuration changes (that haven't been finalized). Given the goal of this, I
think it's safer to ensure sync's backend is loaded and check
EncryptEverythingEnabled.

As such, the condition should be (instead of GetEncryptedDataTypes):
sync_service->backend_initialized() && !sync_service->EncryptEverythingEnabled()

It's yet uglier, but this is starting to dig into the internal of sync state,
which has to be concerned with transient issues. Really, the safest approach
would be the one that checks runtime conditions rather than preferences:
sync_service && sync_service->GetActiveDataTypes().Has(syncer::SESSIONS) &&
!sync_service->EncryptEverythingEnabled().

This does have the effect of meaning precaching would be disabled during the
time sync hasn't yet loaded or is shutting down, but from a privacy standpoint
I'm guessing that's okay?

[twifkak, 2015-06-18 19:37:02]

On 2015/06/18 18:25:49, Nicolas Zea wrote:
>
https://codereview.chromium.org/1176253002/diff/80001/components/precache/con...
> File components/precache/content/precache_manager.cc (right):
> 
>
https://codereview.chromium.org/1176253002/diff/80001/components/precache/con...
> components/precache/content/precache_manager.cc:73:
> !sync_service_->GetEncryptedDataTypes().Has(syncer::SESSIONS);
> One correction (which I think was overlooked in the email):
> GetEncryptedDataTypes will return the minimal set (just passwords) if sync's
> backend hasn't initialized yet, and will also not take into account any
pending
> configuration changes (that haven't been finalized). Given the goal of this, I
> think it's safer to ensure sync's backend is loaded and check
> EncryptEverythingEnabled.
> 
> As such, the condition should be (instead of GetEncryptedDataTypes):
> sync_service->backend_initialized() &&
!sync_service->EncryptEverythingEnabled()
> 
> It's yet uglier, but this is starting to dig into the internal of sync state,
> which has to be concerned with transient issues. Really, the safest approach
> would be the one that checks runtime conditions rather than preferences:
> sync_service && sync_service->GetActiveDataTypes().Has(syncer::SESSIONS) &&
> !sync_service->EncryptEverythingEnabled().
> 
> This does have the effect of meaning precaching would be disabled during the
> time sync hasn't yet loaded or is shutting down, but from a privacy standpoint
> I'm guessing that's okay?

From a privacy standpoint I think it's okay. For about how long is sync
not-yet-loaded or shutting down? What conditions trigger these states on
Android?

[Nicolas Zea, 2015-06-18 19:48:16]

On 2015/06/18 19:37:02, twifkak wrote:
> On 2015/06/18 18:25:49, Nicolas Zea wrote:
> >
>
https://codereview.chromium.org/1176253002/diff/80001/components/precache/con...
> > File components/precache/content/precache_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/1176253002/diff/80001/components/precache/con...
> > components/precache/content/precache_manager.cc:73:
> > !sync_service_->GetEncryptedDataTypes().Has(syncer::SESSIONS);
> > One correction (which I think was overlooked in the email):
> > GetEncryptedDataTypes will return the minimal set (just passwords) if sync's
> > backend hasn't initialized yet, and will also not take into account any
> pending
> > configuration changes (that haven't been finalized). Given the goal of this,
I
> > think it's safer to ensure sync's backend is loaded and check
> > EncryptEverythingEnabled.
> > 
> > As such, the condition should be (instead of GetEncryptedDataTypes):
> > sync_service->backend_initialized() &&
> !sync_service->EncryptEverythingEnabled()
> > 
> > It's yet uglier, but this is starting to dig into the internal of sync
state,
> > which has to be concerned with transient issues. Really, the safest approach
> > would be the one that checks runtime conditions rather than preferences:
> > sync_service && sync_service->GetActiveDataTypes().Has(syncer::SESSIONS) &&
> > !sync_service->EncryptEverythingEnabled().
> > 
> > This does have the effect of meaning precaching would be disabled during the
> > time sync hasn't yet loaded or is shutting down, but from a privacy
standpoint
> > I'm guessing that's okay?
> 
> From a privacy standpoint I think it's okay. For about how long is sync
> not-yet-loaded or shutting down? What conditions trigger these states on
> Android?

Sync.Startup.TimeDeferred2 captures how long we're delayed in UMA. For Android
the median is around 2 seconds (the user starting to navigate will trigger sync
starting up). Once started, Sync doesn't shut down unless either Chrome gets
killed, the user signs out, or an error occurred (which happens in <.1% of
clients).

[twifkak, 2015-06-18 20:13:07]

On 2015/06/18 19:48:16, Nicolas Zea wrote:
> On 2015/06/18 19:37:02, twifkak wrote:
> > On 2015/06/18 18:25:49, Nicolas Zea wrote:
> > > which has to be concerned with transient issues. Really, the safest
approach
> > > would be the one that checks runtime conditions rather than preferences:
> > > sync_service && sync_service->GetActiveDataTypes().Has(syncer::SESSIONS)
&&
> > > !sync_service->EncryptEverythingEnabled().
> > > 
> > > This does have the effect of meaning precaching would be disabled during
the
> > > time sync hasn't yet loaded or is shutting down, but from a privacy
> standpoint
> > > I'm guessing that's okay?
> > 
> > From a privacy standpoint I think it's okay. For about how long is sync
> > not-yet-loaded or shutting down? What conditions trigger these states on
> > Android?
> 
> Sync.Startup.TimeDeferred2 captures how long we're delayed in UMA. For Android
> the median is around 2 seconds (the user starting to navigate will trigger
sync
> starting up). Once started, Sync doesn't shut down unless either Chrome gets
> killed, the user signs out, or an error occurred (which happens in <.1% of
> clients).

How easily/quickly does Chrome get killed? This service only runs when the phone
is on-power, on-wifi, and non-interactive for at least 15 minutes. I imagine
this is going to cause a lot of sync tasks to start (Facebook, G+, Play Store,
...) and increase the chance of OOMing Chrome, and that's assuming that Chrome
wasn't OOMed while the user was using their phone.

This code is called immediately after
BrowserStartupController.startBrowserProcessesSync() is finished, which I
imagine means sync is not yet set up. This means the alarm will go back to sleep
to try again in another 15 minutes, and hope Chrome wasn't killed in the
meantime. This also lowers the chance of this triggering if the user plugs in
only briefly, e.g. mid-day while at work or at an internet cafe, since those
extra 15 minutes may be crucial.

If it's okay with you, I'd rather take the uglier approach (see latest patch
set), since that increases our likelihood of triggering without loss of privacy.

[twifkak, 2015-06-18 20:21:09]

On 2015/06/18 20:13:07, twifkak wrote:
> On 2015/06/18 19:48:16, Nicolas Zea wrote:
> > On 2015/06/18 19:37:02, twifkak wrote:
> > > On 2015/06/18 18:25:49, Nicolas Zea wrote:
> > > > which has to be concerned with transient issues. Really, the safest
> approach
> > > > would be the one that checks runtime conditions rather than preferences:
> > > > sync_service && sync_service->GetActiveDataTypes().Has(syncer::SESSIONS)
> &&
> > > > !sync_service->EncryptEverythingEnabled().
> > > > 
> > > > This does have the effect of meaning precaching would be disabled during
> the
> > > > time sync hasn't yet loaded or is shutting down, but from a privacy
> > standpoint
> > > > I'm guessing that's okay?
> > > 
> > > From a privacy standpoint I think it's okay. For about how long is sync
> > > not-yet-loaded or shutting down? What conditions trigger these states on
> > > Android?
> > 
> > Sync.Startup.TimeDeferred2 captures how long we're delayed in UMA. For
Android
> > the median is around 2 seconds (the user starting to navigate will trigger
> sync
> > starting up). Once started, Sync doesn't shut down unless either Chrome gets
> > killed, the user signs out, or an error occurred (which happens in <.1% of
> > clients).
> 
> How easily/quickly does Chrome get killed? This service only runs when the
phone
> is on-power, on-wifi, and non-interactive for at least 15 minutes. I imagine
> this is going to cause a lot of sync tasks to start (Facebook, G+, Play Store,
> ...) and increase the chance of OOMing Chrome, and that's assuming that Chrome
> wasn't OOMed while the user was using their phone.
> 
> This code is called immediately after
> BrowserStartupController.startBrowserProcessesSync() is finished, which I
> imagine means sync is not yet set up. This means the alarm will go back to
sleep
> to try again in another 15 minutes, and hope Chrome wasn't killed in the
> meantime. This also lowers the chance of this triggering if the user plugs in
> only briefly, e.g. mid-day while at work or at an internet cafe, since those
> extra 15 minutes may be crucial.
> 
> If it's okay with you, I'd rather take the uglier approach (see latest patch
> set), since that increases our likelihood of triggering without loss of
privacy.

I should add: If this is important to chrome-sync-team, I am open to trying the
prettier approach later and adding some metrics to see how often it affects
users. But for the purposes of an experiment to validate whether wifi-prefetch
is worth pursuing more, I'd rather increase our coverage wherever it's easy to
do so.

[twifkak, 2015-06-22 23:19:55]

bengr and zea: PTAL.

A couple notes:
- EncryptEverythingEnabled() does not exist on the SyncService interface.
However, it appears that GetEncryptedDataTypes() is an effective proxy: "If
|encrypt_everything| is true, |encrypted_types| will be the set of all known
types." [1]
- I'm punting on the async issue for a future CL. [2]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/sync/internal_api/...
is the source of the comment.
https://code.google.com/p/chromium/codesearch#chromium/src/sync/internal_api/...
seems like a relevant line of implementation, but I can't be sure.

[2] For the others: The async issue is that the encryption preference is not
stored locally, so it is dependent on the backend having been initialized first.
The current version is privacy-safe, because GetActiveDataTypes() is empty
before initialization. However, there is a risk of a false negative. If the
browser process is killed (e.g. OOMed) before the precache service runs, then
the precache service starts up the browser and immediately queries
IsPrecachingAllowed. This will return false and cause the prefetch run to abort.
To do this properly, I will need to move to an async model where PrecacheManager
waits on sync_service_->backend_initialized() before deciding whether to
continue precaching. Async is error-prone enough that I prefer to work on it in
a future CL, rather than drag this one on any longer. I believe the privacy bug
is fixed, and the recall issue is a lesser priority.

[Nicolas Zea, 2015-06-23 20:40:00]

sync usage LGTM

[bengr, 2015-06-23 22:16:19]

lgtm with a few nits.

https://codereview.chromium.org/1176253002/diff/120001/chrome/browser/android...
File chrome/browser/android/precache/precache_launcher.cc (right):

https://codereview.chromium.org/1176253002/diff/120001/chrome/browser/android...
chrome/browser/android/precache/precache_launcher.cc:45:
DCHECK(precache_manager);
I'm 99.999% sure this will never be called from incognito, but if it is,
precache_manager will be null and the DCHECK won't be in a release.

https://codereview.chromium.org/1176253002/diff/120001/components/precache/co...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1176253002/diff/120001/components/precache/co...
components/precache/content/precache_manager.cc:45: sync_driver::SyncService*
sync_service)
Should this be const?

https://codereview.chromium.org/1176253002/diff/120001/components/precache/co...
File components/precache/content/precache_manager_unittest.cc (right):

https://codereview.chromium.org/1176253002/diff/120001/components/precache/co...
components/precache/content/precache_manager_unittest.cc:128: factory_(NULL,
Mind changing this to nullptr too?

[twifkak, 2015-06-23 23:54:11]

The CQ bit was checked by twifkak@chromium.org

[twifkak, 2015-06-23 23:54:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org, thestig@chromium.org, yfriedman@chromium.org,
battre@chromium.org, bengr@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1176253002/#ps140001
(title: "Tweaks per bengr.")

[twifkak, 2015-06-23 23:54:52]

Thanks, all! +CQ

https://codereview.chromium.org/1176253002/diff/120001/chrome/browser/android...
File chrome/browser/android/precache/precache_launcher.cc (right):

https://codereview.chromium.org/1176253002/diff/120001/chrome/browser/android...
chrome/browser/android/precache/precache_launcher.cc:45:
DCHECK(precache_manager);
On 2015/06/23 22:16:19, bengr wrote:
> I'm 99.999% sure this will never be called from incognito, but if it is,
> precache_manager will be null and the DCHECK won't be in a release.

Okay, added a null check at line 68.

https://codereview.chromium.org/1176253002/diff/120001/components/precache/co...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1176253002/diff/120001/components/precache/co...
components/precache/content/precache_manager.cc:45: sync_driver::SyncService*
sync_service)
On 2015/06/23 22:16:19, bengr wrote:
> Should this be const?

Yes. Done.

https://codereview.chromium.org/1176253002/diff/120001/components/precache/co...
File components/precache/content/precache_manager_unittest.cc (right):

https://codereview.chromium.org/1176253002/diff/120001/components/precache/co...
components/precache/content/precache_manager_unittest.cc:128: factory_(NULL,
On 2015/06/23 22:16:19, bengr wrote:
> Mind changing this to nullptr too?

Not at all. Done.

[commit-bot: I haz the power, 2015-06-23 23:56:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176253002/140001

[commit-bot: I haz the power, 2015-06-24 01:00:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-24 01:01:14]

Patchset 8 (id:??) landed as
https://crrev.com/85f978746ea9ad414b4fc710a99e8f81d55f7a22
Cr-Commit-Position: refs/heads/master@{#335835}