localStorage: Aggressively flush to disk for Android WebView.

localStorage: Aggressively flush to disk for Android WebView.

Android WebView does not save data during shutdown like Chrome does.
Adding an --enable-aggressive-domstorage-flushing flag to return the
DOM Storage implementation to the prior implementation which had a 1
sec. timer to flush the commit queue.

BUG=479767
Committed: https://crrev.com/a1f8d834ce9fc89e5e409e08bb00173c37854329
Cr-Commit-Position: refs/heads/master@{#329070}

[cmumford, 2015-05-07 21:31:57]

cmumford@chromium.org changed reviewers:
+ benm@chromium.org, davidben@chromium.org, jam@chromium.org,
michaeln@chromium.org

[cmumford, 2015-05-07 21:31:57]

michaeln@ for content/browser/dom_storage/*
jam@ for content/browser/browser_main_loop.cc
benm@ for android_webview/*
davidben@ for content/public/common/*

Wow - six files and four reviewers!

[boliu, 2015-05-07 21:36:04]

On 2015/05/07 21:31:57, cmumford wrote:
> michaeln@ for content/browser/dom_storage/*
> jam@ for content/browser/browser_main_loop.cc
> benm@ for android_webview/*

android_webview rubberstamp lgtm

> davidben@ for content/public/common/*
> 
> Wow - six files and four reviewers!

[michaeln, 2015-05-07 23:19:03]

lgtm

https://codereview.chromium.org/1129233003/diff/20001/android_webview/lib/mai...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/1129233003/diff/20001/android_webview/lib/mai...
android_webview/lib/main/aw_main_delegate.cc:94: // for Chrome to aggressively
persist DOM Storage to minimize data loss.
might be nice to put the link to the android specific crbug here instead of in
the more widely used content lib file

https://codereview.chromium.org/1129233003/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1129233003/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:98: // flushing will commence.
maybe put the comment in the .h file

https://codereview.chromium.org/1129233003/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:405: // More info at
http://crbug.com/479767
nit: Given the nicely named methods and members, much of the comment isn't
really needed and we generally prefer early returns to else where possible. In
the spirit of less is more...

if (s_aggressive_flushing_enabled_)
  return base::TimeDelta::FromSeconds(1);

[cmumford, 2015-05-08 16:02:48]

michaeln's issues addressed. Still waiting on reviews for these files:

  content/public/common/content_switches.(cc|h)
  content/browser/browser_main_loop.cc

https://codereview.chromium.org/1129233003/diff/20001/android_webview/lib/mai...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/1129233003/diff/20001/android_webview/lib/mai...
android_webview/lib/main/aw_main_delegate.cc:94: // for Chrome to aggressively
persist DOM Storage to minimize data loss.
On 2015/05/07 23:19:03, michaeln wrote:
> might be nice to put the link to the android specific crbug here instead of in
> the more widely used content lib file

Done.

https://codereview.chromium.org/1129233003/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1129233003/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:98: // flushing will commence.
On 2015/05/07 23:19:03, michaeln wrote:
> maybe put the comment in the .h file

Done.

https://codereview.chromium.org/1129233003/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:405: // More info at
http://crbug.com/479767
On 2015/05/07 23:19:03, michaeln wrote:
> nit: Given the nicely named methods and members, much of the comment isn't
> really needed and we generally prefer early returns to else where possible. In
> the spirit of less is more...
> 
> if (s_aggressive_flushing_enabled_)
>   return base::TimeDelta::FromSeconds(1);

Done.

[cmumford, 2015-05-08 21:08:17]

cmumford@chromium.org changed reviewers:
+ nasko@chromium.org
- benm@chromium.org

[cmumford, 2015-05-08 21:08:18]

nasko@ : I'd like to finish today, do you have time to review three files?

  content/public/common/content_switches.(cc|h)
  content/browser/browser_main_loop.cc

[nasko, 2015-05-08 21:19:00]

https://codereview.chromium.org/1129233003/diff/40001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1129233003/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:589:
DOMStorageArea::EnableAggressiveCommitDelay();
I wonder if there is a better place to initialize this later in the startup
sequence. I'll try to look around.

In the meantime, please add a TRACE_EVEN macro, so this code can be traced and
tracked in the startup sequence.

[cmumford, 2015-05-08 21:25:46]

Thx nasko.

I added the trace. I'm unsure of the message - it's not really a subsystem, but
didn't see a better value to trace.

That method just sets a boolean value, and we want it done early - at least
before anybody has a change to use DOM Storage.

[nasko, 2015-05-08 22:42:20]

I couldn't find any better place for this to live. nick@ had a very good idea of
following this as an example:
https://code.google.com/p/chromium/codesearch#chromium/src/base/sys_info.cc&l=38,
but it requires some template generalization and refactoring, so it doesn't seem
fit for this CL. I would suggest looking into that as a follow up though.

LGTM

[cmumford, 2015-05-08 22:57:23]

The CQ bit was checked by cmumford@chromium.org

[cmumford, 2015-05-08 22:57:24]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1129233003/#ps60001
(title: "Added trace event.")

[commit-bot: I haz the power, 2015-05-08 22:59:22]

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

[commit-bot: I haz the power, 2015-05-09 03:29:34]

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

[commit-bot: I haz the power, 2015-05-09 03:29:36]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[cmumford, 2015-05-09 14:46:15]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2015-05-09 14:46:30]

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

[commit-bot: I haz the power, 2015-05-09 18:52:46]

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

[commit-bot: I haz the power, 2015-05-09 18:52:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[cmumford, 2015-05-09 20:44:50]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2015-05-09 20:46:29]

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

[commit-bot: I haz the power, 2015-05-10 16:44:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-10 16:46:06]

Patchset 4 (id:??) landed as
https://crrev.com/a1f8d834ce9fc89e5e409e08bb00173c37854329
Cr-Commit-Position: refs/heads/master@{#329070}