Report PPAPI plugin load error code to UMA.

Report PPAPI plugin load error code to UMA.

TBR=sky@chrmium.org
BUG=353886
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259091

[xhwang, 2014-03-20 21:40:02]

This is a prototype CL materializing what I mean in:

https://code.google.com/p/chromium/issues/detail?id=353886#c4

brettw: I added you as the reviewer only because you already reviewed my
previous CL. If you know who is a better reviewer for this change please feel
free to reassign.

https://codereview.chromium.org/206713004/diff/1/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/1/base/native_library.h#newcode58
base/native_library.h:58: uint32* error_code = NULL);
This optional parameter is just trying to avoid update all other call sites
(>10) in this prototype CL. I'll either update all call sites with NULL
|error_code|, or add a new function in this file, e.g.
LoadNativeLibraryAndReportError(). Let me know which one you prefer.

https://codereview.chromium.org/206713004/diff/1/base/native_library_mac.mm
File base/native_library_mac.mm (right):

https://codereview.chromium.org/206713004/diff/1/base/native_library_mac.mm#n...
base/native_library_mac.mm:46: std::string* /* error_message */,
This is tracked in http://crbug.com/353771

[ddorwin, 2014-03-20 22:01:12]

LG T M overall.

https://codereview.chromium.org/206713004/diff/1/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/1/base/native_library.h#newcode55
base/native_library.h:55: // If |error| is not NULL, it may be filled in with an
error message on error.
Similar comment for error_code. We might specifically mention "if the platform
supports it."

https://codereview.chromium.org/206713004/diff/1/base/native_library_win.cc
File base/native_library_win.cc (right):

https://codereview.chromium.org/206713004/diff/1/base/native_library_win.cc#n...
base/native_library_win.cc:50: std::string* /* error_message */,
Shouldn't we try to be as consistent as possible (i.e. for log messages)?

if (!result)
  *error_message = StringPrintf();
return result;

https://codereview.chromium.org/206713004/diff/1/content/ppapi_plugin/ppapi_t...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/206713004/diff/1/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:277: << path.value() << " (error: " <<
error_message << " with error code "
nit: You could also do something like "error 'code' - 'message'"

https://codereview.chromium.org/206713004/diff/1/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:528: // See
http://msdn.microsoft.com/en-us/library/ms681381.aspx
This is based on only Win reporting an error code. That should probably be
mentioned, along with some type of DCHECK that error_code < kMaxLoadErrorCode or
error_code==0 if not OS_WIN.

https://codereview.chromium.org/206713004/diff/1/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:537: base::LinearHistogram::FactoryGet(
Should this be sparse?

[xhwang, 2014-03-21 01:33:44]

comments addressed

[xhwang, 2014-03-21 01:42:58]

https://codereview.chromium.org/206713004/diff/1/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/1/base/native_library.h#newcode55
base/native_library.h:55: // If |error| is not NULL, it may be filled in with an
error message on error.
On 2014/03/20 22:01:12, ddorwin wrote:
> Similar comment for error_code. We might specifically mention "if the platform
> supports it."

Done.

https://codereview.chromium.org/206713004/diff/1/base/native_library_win.cc
File base/native_library_win.cc (right):

https://codereview.chromium.org/206713004/diff/1/base/native_library_win.cc#n...
base/native_library_win.cc:50: std::string* /* error_message */,
On 2014/03/20 22:01:12, ddorwin wrote:
> Shouldn't we try to be as consistent as possible (i.e. for log messages)?
> 
> if (!result)
>   *error_message = StringPrintf();
> return result;

Hmm, then the log message will be (error: 5 - 5) on Windows, which will be a bit
confusing.

https://codereview.chromium.org/206713004/diff/1/content/ppapi_plugin/ppapi_t...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/206713004/diff/1/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:277: << path.value() << " (error: " <<
error_message << " with error code "
On 2014/03/20 22:01:12, ddorwin wrote:
> nit: You could also do something like "error 'code' - 'message'"

Done.

https://codereview.chromium.org/206713004/diff/1/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:528: // See
http://msdn.microsoft.com/en-us/library/ms681381.aspx
On 2014/03/20 22:01:12, ddorwin wrote:
> This is based on only Win reporting an error code. That should probably be
> mentioned, along with some type of DCHECK that error_code < kMaxLoadErrorCode
or
> error_code==0 if not OS_WIN.

Not required since we use sparse histogram.

https://codereview.chromium.org/206713004/diff/1/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:537: base::LinearHistogram::FactoryGet(
On 2014/03/20 22:01:12, ddorwin wrote:
> Should this be sparse?

Good point. Done.

[xhwang, 2014-03-21 01:44:38]

Add jar@chromium.org to review changes in UMA reporting:

PTAL at content/ppapi_plugin/ppapi_thread.cc and
tools/metrics/histograms/histograms.xml

[ddorwin, 2014-03-21 04:20:05]

3 comments, one in previous PS.

https://codereview.chromium.org/206713004/diff/1/base/native_library_win.cc
File base/native_library_win.cc (right):

https://codereview.chromium.org/206713004/diff/1/base/native_library_win.cc#n...
base/native_library_win.cc:50: std::string* /* error_message */,
On 2014/03/21 01:42:58, xhwang wrote:
> On 2014/03/20 22:01:12, ddorwin wrote:
> > Shouldn't we try to be as consistent as possible (i.e. for log messages)?
> > 
> > if (!result)
> >   *error_message = StringPrintf();
> > return result;
> 
> Hmm, then the log message will be (error: 5 - 5) on Windows, which will be a
bit
> confusing.

Yes, but if you don't set it, callers that want an error message will need to
check for an empty message and format the error code themselves. Plus, this
behavior would probably need to be documented in the .h file.

I think it's more important to have consistent and expected behavior than to
worry about such a log message.

https://codereview.chromium.org/206713004/diff/20001/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/20001/base/native_library.h#ne...
base/native_library.h:53: // Loads a native library from disk.  Releases it with
UnloadNativeLibrary when
"Release" was correct. Maybe "The caller must release..." would be clearer.

https://codereview.chromium.org/206713004/diff/20001/base/native_library.h#ne...
base/native_library.h:56: // an error message or an error code respectively on
load error if the platform
nit:
, respectively,

[xhwang, 2014-03-21 17:09:05]

comments addressed

[xhwang, 2014-03-21 17:09:17]

https://codereview.chromium.org/206713004/diff/1/base/native_library_win.cc
File base/native_library_win.cc (right):

https://codereview.chromium.org/206713004/diff/1/base/native_library_win.cc#n...
base/native_library_win.cc:50: std::string* /* error_message */,
On 2014/03/21 04:20:06, ddorwin wrote:
> On 2014/03/21 01:42:58, xhwang wrote:
> > On 2014/03/20 22:01:12, ddorwin wrote:
> > > Shouldn't we try to be as consistent as possible (i.e. for log messages)?
> > > 
> > > if (!result)
> > >   *error_message = StringPrintf();
> > > return result;
> > 
> > Hmm, then the log message will be (error: 5 - 5) on Windows, which will be a
> bit
> > confusing.
> 
> Yes, but if you don't set it, callers that want an error message will need to
> check for an empty message and format the error code themselves. Plus, this
> behavior would probably need to be documented in the .h file.
> 
> I think it's more important to have consistent and expected behavior than to
> worry about such a log message.

Done.

https://codereview.chromium.org/206713004/diff/20001/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/20001/base/native_library.h#ne...
base/native_library.h:53: // Loads a native library from disk.  Releases it with
UnloadNativeLibrary when
On 2014/03/21 04:20:06, ddorwin wrote:
> "Release" was correct. Maybe "The caller must release..." would be clearer.

Done.

https://codereview.chromium.org/206713004/diff/20001/base/native_library.h#ne...
base/native_library.h:56: // an error message or an error code respectively on
load error if the platform
On 2014/03/21 04:20:06, ddorwin wrote:
> nit:
> , respectively,

Done.

[jar (doing other things), 2014-03-21 17:21:55]

histograms.xml LGTM... with a nit below on the c++ side.

https://codereview.chromium.org/206713004/diff/20001/content/ppapi_plugin/ppa...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/206713004/diff/20001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:528: std::ostringstream histogram_name;
nit: I think that cleaner (probably faster) and very readable to just use a
std::string, and then use the concatenation operator "+" to pull the pieces
together.

If you really cared about efficiency, you could give a size to the std::string
constructor.

https://codereview.chromium.org/206713004/diff/20001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:534:
UMA_HISTOGRAM_SPARSE_SLOWLY(histogram_name.str(), error_code);
nit: Don't turn this into a str().  Just pass the std::string.

You are correct that this macro avoids the use of a static, but is best used
when the range is very wide and unpredictable i.e., not for a simple enum in the
range of 1-4).

Assuming this is really a broad error code (I think you get it from Windows as a
GetLastError())... then this is a perfect use of this macro.

[ddorwin, 2014-03-21 17:24:59]

https://codereview.chromium.org/206713004/diff/40001/base/native_library_win.cc
File base/native_library_win.cc (right):

https://codereview.chromium.org/206713004/diff/40001/base/native_library_win....
base/native_library_win.cc:18: NativeLibrary LoadNativeLibraryHelper(const
FilePath& library_path,
Not related to this CL, but this should be static/anonymous.

https://codereview.chromium.org/206713004/diff/40001/base/native_library_win....
base/native_library_win.cc:38: if (!module && error_code) {
This should be "...&& (error_code || error_message)" - or maybe something else
depending on the outcome below.

You'll then need to check error_code at ine 42.

https://codereview.chromium.org/206713004/diff/40001/base/native_library_win....
base/native_library_win.cc:41: *error_message = StringPrintf("%u", last_error);
You need to check for a NULL error_message.

Actually, since we just convert the error code to a string and one of the
callers doesn't want this, I was thinking this could be done in the public
function. That makes the function slightly more complex, but it keeps the
responsibilities of this one to a minimum. WDYT?

OTOH, it's possible that error_code was not passed to the public function, so
you'd need to create a local variable to get it out, so maybe doing it here is
better.

[xhwang, 2014-03-21 19:00:54]

ddorwin: I changed error_message/error_code to NativeLibraryLoadError which I
feel is cleaner. Let me know what you think about it.

jar: Changed to use std::string. Thanks for the review!

https://codereview.chromium.org/206713004/diff/20001/content/ppapi_plugin/ppa...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/206713004/diff/20001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:528: std::ostringstream histogram_name;
On 2014/03/21 17:21:56, jar wrote:
> nit: I think that cleaner (probably faster) and very readable to just use a
> std::string, and then use the concatenation operator "+" to pull the pieces
> together.
> 
> If you really cared about efficiency, you could give a size to the std::string
> constructor.

Done.

https://codereview.chromium.org/206713004/diff/20001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:534:
UMA_HISTOGRAM_SPARSE_SLOWLY(histogram_name.str(), error_code);
On 2014/03/21 17:21:56, jar wrote:
> nit: Don't turn this into a str().  Just pass the std::string.
> 
> You are correct that this macro avoids the use of a static, but is best used
> when the range is very wide and unpredictable i.e., not for a simple enum in
the
> range of 1-4).
> 
> Assuming this is really a broad error code (I think you get it from Windows as
a
> GetLastError())... then this is a perfect use of this macro.

Done.

https://codereview.chromium.org/206713004/diff/40001/base/native_library_win.cc
File base/native_library_win.cc (right):

https://codereview.chromium.org/206713004/diff/40001/base/native_library_win....
base/native_library_win.cc:18: NativeLibrary LoadNativeLibraryHelper(const
FilePath& library_path,
On 2014/03/21 17:24:59, ddorwin wrote:
> Not related to this CL, but this should be static/anonymous.

Done.

https://codereview.chromium.org/206713004/diff/40001/base/native_library_win....
base/native_library_win.cc:38: if (!module && error_code) {
On 2014/03/21 17:24:59, ddorwin wrote:
> This should be "...&& (error_code || error_message)" - or maybe something else
> depending on the outcome below.
> 
> You'll then need to check error_code at ine 42.

Now this is obsolete.

https://codereview.chromium.org/206713004/diff/40001/base/native_library_win....
base/native_library_win.cc:41: *error_message = StringPrintf("%u", last_error);
On 2014/03/21 17:24:59, ddorwin wrote:
> You need to check for a NULL error_message.
> 
> Actually, since we just convert the error code to a string and one of the
> callers doesn't want this, I was thinking this could be done in the public
> function. That makes the function slightly more complex, but it keeps the
> responsibilities of this one to a minimum. WDYT?
> 
> OTOH, it's possible that error_code was not passed to the public function, so
> you'd need to create a local variable to get it out, so maybe doing it here is
> better.

I think the new NativeLibraryLoadError helps avoid all these issues. WDYT?

https://codereview.chromium.org/206713004/diff/60001/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/60001/base/native_library.h#ne...
base/native_library.h:57: #endif  // OS_WIN
I like this more now because it makes call sites more cleaner.

[ddorwin, 2014-03-21 20:06:32]

https://codereview.chromium.org/206713004/diff/50008/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/50008/base/native_library.h#ne...
base/native_library.h:54: typedef DWORD NativeLibraryLoadError;
While more explicit about what to expect, this forces the caller to ifdef its
code.
On the other hand, this is most often used for LOG statements, which happily
cast and work fine.

It seems like an odd thing to expose to all of Chromium (it's in base/). Also,
there is the issue of initialization. To address that, this could be a class
with a operator().

https://codereview.chromium.org/206713004/diff/50008/base/native_library.h#ne...
base/native_library.h:61: // If |error| is not NULL, it may be filled in on load
error if the platform
I think we can drop the "if the platform..." part. Maybe we can replace "may"
with "can" when the Mac implementation is fixed.

https://codereview.chromium.org/206713004/diff/50008/chrome/browser/load_libr...
File chrome/browser/load_library_perf_test.cc (right):

https://codereview.chromium.org/206713004/diff/50008/chrome/browser/load_libr...
chrome/browser/load_library_perf_test.cc:31: base::NativeLibraryLoadError error
= 0;
This compiles on platforms other than Windows, but it hardly does what is
expected.

https://codereview.chromium.org/206713004/diff/50008/chrome/renderer/chrome_r...
File chrome/renderer/chrome_render_process_observer.cc (right):

https://codereview.chromium.org/206713004/diff/50008/chrome/renderer/chrome_r...
chrome/renderer/chrome_render_process_observer.cc:298:
base::LoadNativeLibrary(base::FilePath(L"crypt32.dll"), &error);
just pass NULL

[xhwang, 2014-03-21 22:33:28]

https://codereview.chromium.org/206713004/diff/50008/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/50008/base/native_library.h#ne...
base/native_library.h:54: typedef DWORD NativeLibraryLoadError;
On 2014/03/21 20:06:32, ddorwin wrote:
> While more explicit about what to expect, this forces the caller to ifdef its
> code.
> On the other hand, this is most often used for LOG statements, which happily
> cast and work fine.
> 
> It seems like an odd thing to expose to all of Chromium (it's in base/). Also,
> there is the issue of initialization. To address that, this could be a class
> with a operator().

I fixed the initialization issue with a struct. Most callers don't need to care
about ifdef. ppapi_thread.cc is the only exception because UMA doesn't support
string very well.

https://codereview.chromium.org/206713004/diff/50008/base/native_library.h#ne...
base/native_library.h:61: // If |error| is not NULL, it may be filled in on load
error if the platform
On 2014/03/21 20:06:32, ddorwin wrote:
> I think we can drop the "if the platform..." part. Maybe we can replace "may"
> with "can" when the Mac implementation is fixed.

Done.

https://codereview.chromium.org/206713004/diff/50008/chrome/browser/load_libr...
File chrome/browser/load_library_perf_test.cc (right):

https://codereview.chromium.org/206713004/diff/50008/chrome/browser/load_libr...
chrome/browser/load_library_perf_test.cc:31: base::NativeLibraryLoadError error
= 0;
On 2014/03/21 20:06:32, ddorwin wrote:
> This compiles on platforms other than Windows, but it hardly does what is
> expected.

Fixed.

https://codereview.chromium.org/206713004/diff/50008/chrome/renderer/chrome_r...
File chrome/renderer/chrome_render_process_observer.cc (right):

https://codereview.chromium.org/206713004/diff/50008/chrome/renderer/chrome_r...
chrome/renderer/chrome_render_process_observer.cc:298:
base::LoadNativeLibrary(base::FilePath(L"crypt32.dll"), &error);
On 2014/03/21 20:06:32, ddorwin wrote:
> just pass NULL

Done.

[xhwang, 2014-03-21 22:41:15]

brettw: I think this CL is ready for full review.

Please compare PS3 and PS7.

In PS3 we explicitly pass in two params: error_message and error_code. This is
nice that we have less platform specific code, but sadly all callers needs to
use one extra parameter (or NULL). Note that in PS3 most callers are not
updated.

In PS7 I added a struct NativeLibraryLoadError which encapsulates all details in
base/ so that caller don't need to change much.

PTAL. Thanks!

[ddorwin, 2014-03-21 22:47:00]

This LG with one suggestion.

https://codereview.chromium.org/206713004/diff/150001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/206713004/diff/150001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:524: void PpapiThread::ReportLoadErrorCode(
Maybe comment that this is only reported on Windows because it is the only
platform with a numeric code.

[xhwang, 2014-03-21 23:02:04]

https://codereview.chromium.org/206713004/diff/150001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/206713004/diff/150001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:524: void PpapiThread::ReportLoadErrorCode(
On 2014/03/21 22:47:01, ddorwin wrote:
> Maybe comment that this is only reported on Windows because it is the only
> platform with a numeric code.

Done.

[brettw, 2014-03-24 17:21:24]

https://codereview.chromium.org/206713004/diff/210001/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/210001/base/native_library.h#n...
base/native_library.h:94: BASE_EXPORT std::ostream& operator<<(std::ostream&
out,
The style guide permits operator<< for logging but says "you should try to avoid
these whenever possible". We do this for some of the very common classes we have
that need to be logged frequently. But in this case, it would seem to be just as
clear to add a GetError() member to the NativeLibraryLoadError that returns a
string that people can use for the log messages.

Then you can also remove the extra includes for ios stuff as well.

[xhwang, 2014-03-24 17:53:20]

brettw: Comment addressed. PTAL again. Thanks!

https://codereview.chromium.org/206713004/diff/210001/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/206713004/diff/210001/base/native_library.h#n...
base/native_library.h:94: BASE_EXPORT std::ostream& operator<<(std::ostream&
out,
On 2014/03/24 17:21:25, brettw wrote:
> The style guide permits operator<< for logging but says "you should try to
avoid
> these whenever possible". We do this for some of the very common classes we
have
> that need to be logged frequently. But in this case, it would seem to be just
as
> clear to add a GetError() member to the NativeLibraryLoadError that returns a
> string that people can use for the log messages.
> 
> Then you can also remove the extra includes for ios stuff as well.

Done.

[xhwang, 2014-03-24 17:56:35]

Also, I was debating about GetError() and ToString(). Maybe ToString() is more
accurate about what this function is doing. WDYT?

[brettw, 2014-03-24 18:03:44]

lgtm

[xhwang, 2014-03-24 18:12:05]

TBRing sky@chrmium.org on chrome/ mojo/ and ui/ changes.

[xhwang, 2014-03-24 18:12:13]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-03-24 18:12:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/206713004/250001

[commit-bot: I haz the power, 2014-03-24 18:24:25]

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

[commit-bot: I haz the power, 2014-03-24 18:24:26]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[xhwang, 2014-03-24 18:35:27]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-03-24 18:35:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/206713004/250001

[commit-bot: I haz the power, 2014-03-24 18:42:05]

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

[commit-bot: I haz the power, 2014-03-24 18:42:06]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[xhwang, 2014-03-24 20:08:35]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-03-24 20:09:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/206713004/270001

[commit-bot: I haz the power, 2014-03-24 21:06:38]

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

[commit-bot: I haz the power, 2014-03-24 21:06:38]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[xhwang, 2014-03-24 21:50:36]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-03-24 21:51:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/206713004/290001

[commit-bot: I haz the power, 2014-03-25 01:59:12]

Change committed as 259091