Add methods to add DataPack from open files

Add methods to add DataPack from open files

On Android, renderer sandboxing prevents us from opening files and so
ResourceBundle is initialized with file descriptors passed in at process
creation. This change adds methods to DataPack and ResourceBundle to
support loading/initializing from PlatformFile in addition to the
current FilePath.

Also, the current methods are renamed so that the naming is consistent
between these two files.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145927

[cjhopman, 2012-06-27 21:23:29]

grt: chrome_frame/
brettw: content/, webkit/
sail: ui/base/resource
sky: chrome/, ui/test, ui/aura

[grt (UTC plus 2), 2012-06-28 14:24:05]

chrome_frame lgtm

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc#...
ui/base/resource/data_pack.cc:89:
UMA_HISTOGRAM_ENUMERATION("DataPack.LoadFromFile", INIT_FAILED,
do you have a corresponding change to histograms.xml?

[sail, 2012-06-28 15:48:15]

LGTM!
Looks really good.

[brettw, 2012-06-28 19:38:55]

content/webkit LGTM

[cjhopman, 2012-06-28 20:47:23]

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc#...
ui/base/resource/data_pack.cc:77: UMA_HISTOGRAM_ENUMERATION("DataPack.Load",
INIT_FAILED,
Should I change this key to DataPack.LoadFromPath or leave it as is?

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc#...
ui/base/resource/data_pack.cc:89:
UMA_HISTOGRAM_ENUMERATION("DataPack.LoadFromFile", INIT_FAILED,
On 2012/06/28 14:24:05, grt wrote:
> do you have a corresponding change to histograms.xml?

Done. https://chromereviews.googleplex.com/4614016.

[grt (UTC plus 2), 2012-06-28 21:04:57]

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc#...
ui/base/resource/data_pack.cc:77: UMA_HISTOGRAM_ENUMERATION("DataPack.Load",
INIT_FAILED,
On 2012/06/28 20:47:23, cjhopman wrote:
> Should I change this key to DataPack.LoadFromPath or leave it as is?

i'm not sure.  check with one of the OWNERS of histograms.xml to see what they
recommend.

[cjhopman, 2012-06-28 21:39:10]

jar@: Could you take a look at ui/base/resource/data_pack.cc and suggest an
approach to the histogram stuff.

Maybe leaving all 3 uses as DataPack.Load is the way to go... but I don't really
know.

[jar (doing other things), 2012-06-29 02:20:23]

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc#...
ui/base/resource/data_pack.cc:89:
UMA_HISTOGRAM_ENUMERATION("DataPack.LoadFromFile", INIT_FAILED,
It is a bit strange to have a histogram with *only* this enum ever put into
it... despite having room for all the other errors. :-/

IMO, I'd add a new single enum at the end of your list:

INIT_FAILED_LOADING_FROM_FILE

and then use that in the current DataPackLoad histogram.

You could alternatively just use the INIT_FAILED sample... but if you fear that
it will confuse you (and want the case pulled out), use a new enum (rather than
a whole new histogram).

Best practice is to add enums to the end of the old list, just in front of
LOAD_ERRORS_COUNT (which is never used!!).  You can then just grow the enum list
in the xml file with no problems as well.

[cjhopman, 2012-06-29 20:30:53]

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/10686005/diff/1/ui/base/resource/data_pack.cc#...
ui/base/resource/data_pack.cc:89:
UMA_HISTOGRAM_ENUMERATION("DataPack.LoadFromFile", INIT_FAILED,
On 2012/06/29 02:20:23, jar wrote:
> It is a bit strange to have a histogram with *only* this enum ever put into
> it... despite having room for all the other errors. :-/
> 
> IMO, I'd add a new single enum at the end of your list:
> 
> INIT_FAILED_LOADING_FROM_FILE
> 
> and then use that in the current DataPackLoad histogram.
> 
> You could alternatively just use the INIT_FAILED sample... but if you fear
that
> it will confuse you (and want the case pulled out), use a new enum (rather
than
> a whole new histogram).
> 
> Best practice is to add enums to the end of the old list, just in front of
> LOAD_ERRORS_COUNT (which is never used!!).  You can then just grow the enum
list
> in the xml file with no problems as well.

Done. Thanks for the suggestions.

[jar (doing other things), 2012-06-30 00:49:27]

histogram usage in data_pacck.cc LGTM

[cjhopman, 2012-07-09 23:32:34]

ben@: could you check this for chrome/, ui/test, and ui/aura since sky is OOO.

[Ben Goodger (Google), 2012-07-10 15:08:21]

lgtm

[commit-bot: I haz the power, 2012-07-10 15:59:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/10686005/6002

[commit-bot: I haz the power, 2012-07-10 15:59:48]

Failed to apply patch for ui/base/resource/resource_bundle_aurax11.cc:
While running patch -p1 --forward --force;
patching file ui/base/resource/resource_bundle_aurax11.cc
Hunk #1 FAILED at 39.
1 out of 1 hunk FAILED -- saving rejects to file
ui/base/resource/resource_bundle_aurax11.cc.rej

[commit-bot: I haz the power, 2012-07-10 17:20:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/10686005/5002

[commit-bot: I haz the power, 2012-07-10 19:18:06]

Change committed as 145927