Report an error message when an update job results in an error.

New javascript console logging related to the appcache.
* Log an error message when an update job results in an error.
* Log info messages for progress events and other update status changed events.
* Consistently use "Application Cache" in the console messages.

TEST=existing unit tests apply for functional correctness, manual testing to see the new console output
BUG=none


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

[michaeln, 2010-07-12 21:31:13]

not ready for review quite yet

[michaeln, 2010-07-12 23:59:56]

I think this is ready for a look. There are some changes of substance are in
appcache_update_job.cc and in web_application_cache_host_impl.cc (see the new
calls to OnLogMessage()). The rest is just plumbing to widen the error event
data path to carry a message parameter.

[michaeln, 2010-07-13 00:39:02]

Fixed the compile error and switched to using "ApplicationCache" consistently in
the messages.

[michaeln, 2010-07-13 00:54:04]

Additional tweeks to the error messages like we talked about, less punctuation.

[michaeln, 2010-07-13 01:12:37]

Fixed up unittests to override the new AppCacheFrontend pure virtual method.

[Kavita Kanetkar, 2010-07-13 06:37:44]

I have some questions. Please see below.

Thanks,
Kavita

http://codereview.chromium.org/2910007/diff/60001/28024
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/2910007/diff/60001/28024#newcode320
webkit/appcache/appcache_host.cc:320: "Document was loaded from ApplicationCache
with manifest %s";
Application Cache [space]

http://codereview.chromium.org/2910007/diff/60001/28028
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/2910007/diff/60001/28028#newcode109
webkit/appcache/appcache_update_job.cc:109: void SendErrorNotifications(const
std::string& error_message) {
ToFrontend? Or ErrorNotifyRenderer ? Or something? 
This is optional.

http://codereview.chromium.org/2910007/diff/60001/28028#newcode257
webkit/appcache/appcache_update_job.cc:257: // 6.9.4 cache failure steps 2-8.
What does this refer to?

http://codereview.chromium.org/2910007/diff/60001/28028#newcode258
webkit/appcache/appcache_update_job.cc:258: DCHECK(internal_state_ !=
CACHE_FAILURE);
Can you please add  << internal_state_ ?

http://codereview.chromium.org/2910007/diff/60001/28028#newcode265
webkit/appcache/appcache_update_job.cc:265: internal_state_ = COMPLETED;
Earlier the internal_state_ remained CACHE_FAILURE? Now it goes to COMPLETED?

http://codereview.chromium.org/2910007/diff/60001/28028#newcode530
webkit/appcache/appcache_update_job.cc:530: std::string message =
StringPrintf(kFormatString, response_code,
Can this be const std::string& ?

http://codereview.chromium.org/2910007/diff/60001/28028#newcode568
webkit/appcache/appcache_update_job.cc:568: std::string message =
StringPrintf(kFormatString,
same.

http://codereview.chromium.org/2910007/diff/60001/28028#newcode571
webkit/appcache/appcache_update_job.cc:571: LOG(INFO) << message;
Is this LOG intentional?

http://codereview.chromium.org/2910007/diff/60001/28028#newcode611
webkit/appcache/appcache_update_job.cc:611: if (request->status().is_success()
&& (response_code / 100 == 2)) {
Why don't we say response_code == 200 ?

http://codereview.chromium.org/2910007/diff/60001/28028#newcode628
webkit/appcache/appcache_update_job.cc:628: LOG(INFO) << "Request status: " <<
request->status().status()
Where are these printed?

http://codereview.chromium.org/2910007/diff/60001/28028#newcode632
webkit/appcache/appcache_update_job.cc:632: const char* kFormatString =
"Resource fetch failed (%d) %s";
Do we want to add request status?

[michaeln, 2010-07-13 07:31:03]

http://codereview.chromium.org/2910007/diff/60001/28024
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/2910007/diff/60001/28024#newcode320
webkit/appcache/appcache_host.cc:320: "Document was loaded from ApplicationCache
with manifest %s";
On 2010/07/13 06:37:44, Kavita Kanetkar wrote:
> Application Cache [space]

will do for all

http://codereview.chromium.org/2910007/diff/60001/28028
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/2910007/diff/60001/28028#newcode257
webkit/appcache/appcache_update_job.cc:257: // 6.9.4 cache failure steps 2-8.
html5 draft specification section numbers

http://codereview.chromium.org/2910007/diff/60001/28028#newcode258
webkit/appcache/appcache_update_job.cc:258: DCHECK(internal_state_ !=
CACHE_FAILURE);
On 2010/07/13 06:37:44, Kavita Kanetkar wrote:
> Can you please add  << internal_state_ ?

Typcially we haven't been doing that with DCHECKs in this library. The output
contains info about the location and check that failed as is. Why do you want to
add it?

http://codereview.chromium.org/2910007/diff/60001/28028#newcode265
webkit/appcache/appcache_update_job.cc:265: internal_state_ = COMPLETED;
On 2010/07/13 06:37:44, Kavita Kanetkar wrote:
> Earlier the internal_state_ remained CACHE_FAILURE? Now it goes to COMPLETED?

We're no longer calling MaybeCompleteUpdate() to finish things up in the failure
case. These actions used to be taken by that method in this code path. This
stuff has been hoisted into this method now. And MaybeCompleteUpdate has been
modified to not handle this case anymore. All failure code paths now go thru
this method.

I have to look more closely at the test results for any bad consequences caused
by this particular code change.

http://codereview.chromium.org/2910007/diff/60001/28028#newcode530
webkit/appcache/appcache_update_job.cc:530: std::string message =
StringPrintf(kFormatString, response_code,
On 2010/07/13 06:37:44, Kavita Kanetkar wrote:
> Can this be const std::string& ?

You mean kFormatString or message? I'll look more closely at the StringPrintf
signature to understand what the first parameter wants to be. I'll see about
both.

http://codereview.chromium.org/2910007/diff/60001/28028#newcode571
webkit/appcache/appcache_update_job.cc:571: LOG(INFO) << message;
On 2010/07/13 06:37:44, Kavita Kanetkar wrote:
> Is this LOG intentional?

Yes, retaining the pre-existing logging is all.

http://codereview.chromium.org/2910007/diff/60001/28028#newcode611
webkit/appcache/appcache_update_job.cc:611: if (request->status().is_success()
&& (response_code / 100 == 2)) {
On 2010/07/13 06:37:44, Kavita Kanetkar wrote:
> Why don't we say response_code == 200 ?

Spec compliance.

http://codereview.chromium.org/2910007/diff/60001/28028#newcode628
webkit/appcache/appcache_update_job.cc:628: LOG(INFO) << "Request status: " <<
request->status().status()
On 2010/07/13 06:37:44, Kavita Kanetkar wrote:
> Where are these printed?

Same place DCHECK output goes i think :)

http://codereview.chromium.org/2910007/diff/60001/28028#newcode632
webkit/appcache/appcache_update_job.cc:632: const char* kFormatString =
"Resource fetch failed (%d) %s";
On 2010/07/13 06:37:44, Kavita Kanetkar wrote:
> Do we want to add request status?

Not sure it adds much value, as coded the console will see either -1 when a
platform specific non http error has occurred or an http response code.

[michaeln, 2010-07-13 18:38:57]

New snapshot...

> > Application Cache [space]
done


> > Can this be const std::string& ?
done

It can't be a reference (somebody has to provide storage for it), but it can be
const. kFormatString is left as const char* since StringPrintf takes C strings
are parameters.

[Kavita Kanetkar, 2010-07-13 19:00:41]

LGTM.