ContentAPI change - Post DomStorage tasks via a SequencedTaskRunner instead of directly to WEBKIT_DE

ContentAPI change - Post DomStorage tasks via a SequencedTaskRunner instead of directly to WEBKIT_DEPRECATED.

BUG=106763
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126720

[michaeln, 2012-03-14 02:12:59]

ptal at this content layer api change and corresponding mods to existing
consumers

[michaeln, 2012-03-14 05:17:26]

http://codereview.chromium.org/9700007/diff/1/chrome/browser/browsing_data_lo...
File chrome/browser/browsing_data_local_storage_helper.cc (right):

http://codereview.chromium.org/9700007/diff/1/chrome/browser/browsing_data_lo...
chrome/browser/browsing_data_local_storage_helper.cc:91: void
BrowsingDataLocalStorageHelper::FetchLocalStorageInfoInWebKitThread() {
i'll change these methods names to read <DoX>InSequencedTask

http://codereview.chromium.org/9700007/diff/1/chrome/browser/browsing_data_lo...
File chrome/browser/browsing_data_local_storage_helper.h (left):

http://codereview.chromium.org/9700007/diff/1/chrome/browser/browsing_data_lo...
chrome/browser/browsing_data_local_storage_helper.h:146: mutable base::Lock
lock_;
n/a since this class is now single-threaded

http://codereview.chromium.org/9700007/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/9700007/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.h:251: content::DOMStorageContext*
dom_storage_context);
switched to a ptr because there's no reason to pass by value... passing
scoped_refptr<T> by value should be banned in our style-guide afaict

http://codereview.chromium.org/9700007/diff/1/content/browser/browser_context.cc
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/9700007/diff/1/content/browser/browser_context...
content/browser/browser_context.cc:255:
context->task_runner()->ReleaseSoon(FROM_HERE, context);
to make the browser/ui/unittest gods happy, may have to call Release() if
ReleaseSoon returns false?

[michaeln, 2012-03-14 05:18:58]

http://codereview.chromium.org/9700007/diff/1/content/browser/browser_context.cc
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/9700007/diff/1/content/browser/browser_context...
content/browser/browser_context.cc:255:
context->task_runner()->ReleaseSoon(FROM_HERE, context);
On 2012/03/14 05:17:26, michaeln wrote:
> to make the browser/ui/unittest gods happy, may have to call Release() if
> ReleaseSoon returns false?

or maybe all relevant tests have been marked flaky-->disabled so it's all swept
under the rugs already

[michaeln, 2012-03-14 18:52:27]

new snapshot

i've removed changes to content/browser/browser_context.cc from this CL which
just addresses the public content api change and fixes up consumers in chrome

[jam, 2012-03-14 18:59:40]

it seems a little awkward to have an interface and say you must call the
functions on a taskrunner from it. why not just have this be an internal
implementation detail, and have the callers call on their main thread?

[michaeln, 2012-03-14 19:06:12]

agreed its not the best api (the functions return values directly instead of
providing an async completion callback type of interface)

i'm not looking to 'fix' that aspect of things right now... i'm working behind
the existing interfaces and swapping something new under them... the current
interface required "CALL ON WEBKIT THREAD ONLY"... i can't work behind that, so
i have to change at least that much of the public api (and that's all)

[jam, 2012-03-14 19:09:40]

What is the motivation in doing this change? It's hard for me to understand
that  without knowing what else you're planning on doing. i.e. why not make
this include a switch to async callbacks?

On Wed, Mar 14, 2012 at 12:06 PM, <michaeln@chromium.org> wrote:

> agreed its not the best api (the functions return values directly instead
> of
> providing an async completion callback type of interface)
>
> i'm not looking to 'fix' that aspect of things right now... i'm working
> behind
> the existing interfaces and swapping something new under them... the
> current
> interface required "CALL ON WEBKIT THREAD ONLY"... i can't work behind
> that, so
> i have to change at least that much of the public api (and that's all)
>
>
https://chromiumcodereview.**appspot.com/9700007/<https://chromiumcodereview....
>

[michaeln, 2012-03-14 19:16:25]

On 2012/03/14 19:09:40, John Abd-El-Malek wrote:
> What is the motivation in doing this change? It's hard for me to understand
> that  without knowing what else you're planning on doing. i.e. why not make
> this include a switch to async callbacks?

See the bug http://code.google.com/p/chromium/issues/detail?id=106763

Because time is short and larger changes to the content layer api are orthogonal
and unnecessary to meet my m19 goal.

[michaeln, 2012-03-14 19:40:41]

This change is on my critical path...
https://chromiumcodereview.appspot.com/9695013/

I don't want to be distracted from that.

[jam, 2012-03-14 19:52:28]

as discussed with Michael, I'm not a big fan of adding exposing the task runner
through the interface. since he's under time pressure, lgtm for now and i'll
convert this class to use callbacks after he lands.

[michaeln, 2012-03-14 20:19:17]

On 2012/03/14 19:52:28, John Abd-El-Malek wrote:
> ...and i'll convert this class to use callbacks after he lands.

thnx john, if and when you take a stab at improving the content layer api, in
addition to relaxing the threading requirements and making the interfaces
async... another wart to remove is around the datatypes used in the interface...
'origins' should be expressed as GURLs and FilePath should be abstracted out...
the latter is to allow a later change from sqlite to leveldb backing in which
case there will be one leveldb file containing everything.

  struct UsageInfo {
    GURL origin;
    size_t data_size;
    base::Time last_modified_time;
  }

  // Returns info about all local storage usage.
  virtual std::vector<UsageInfo> GetAllUsageInfo() = 0;

  // Deletes the local storage file for the given origin.
  virtual void DeleteForOrigin(const GURL& origin) = 0;

  // Delete any local storage files that have been touched since the cutoff
  // date that's supplied. Protected origins, per the SpecialStoragePolicy,
  // are not deleted by this method.
  virtual void DeleteDataModifiedSince(const base::Time& cutoff) = 0;