Domain Reliability: Add rudimentary WebUI page.

Domain Reliability: Add rudimentary WebUI page.

Add a basic diagnostic page for Domain Reliability at
chrome://domain-reliability-internals/.

BUG=

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

[Deprecated (see juliatuttle), 2014-06-28 00:07:19]

PTAL, davidben.

bauerb, PTAL at chrome/browser/{resources,ui/webui}.

[Bernhard Bauer, 2014-06-28 11:16:35]

https://codereview.chromium.org/357103002/diff/1/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/357103002/diff/1/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:122: <include
name="IDR_DOMAIN_RELIABILITY_INTERNALS_HTML"
file="resources\domain_reliability_internals.html" type="BINDATA"/>
Nit: More common style seems to be to add a space before the /.

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
File chrome/browser/resources/domain_reliability_internals.html (right):

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
chrome/browser/resources/domain_reliability_internals.html:8: <!-- <script
src="chrome://resources/js/load_time_data.js"></script> -->
Remove this?

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
File chrome/browser/resources/domain_reliability_internals.js (right):

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
chrome/browser/resources/domain_reliability_internals.js:5:
cr.define('domain_reliability_internals', function() {
You're using this roughly as a class with static methods, right? In that case,
the name should be in CamelCase.

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
chrome/browser/resources/domain_reliability_internals.js:13:
//alert(JSON.stringify(data));
Remove.

https://codereview.chromium.org/357103002/diff/1/chrome/browser/ui/webui/doma...
File chrome/browser/ui/webui/domain_reliability_internals_ui.cc (right):

https://codereview.chromium.org/357103002/diff/1/chrome/browser/ui/webui/doma...
chrome/browser/ui/webui/domain_reliability_internals_ui.cc:53: NOTREACHED();
If this can't happen, you don't need to deal with it in production code. DCHECK
service instead.

https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
File components/domain_reliability/service.cc (right):

https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
components/domain_reliability/service.cc:75:
network_task_runner_->PostTaskAndReply(FROM_HERE,
I think you can do this with base::PostTaskAndReplyWithResult(). And you can
directly pass a scoped_ptr to a callback with base::Passed().

[Deprecated (see juliatuttle), 2014-06-30 16:58:21]

PTAL.

https://codereview.chromium.org/357103002/diff/1/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/357103002/diff/1/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:122: <include
name="IDR_DOMAIN_RELIABILITY_INTERNALS_HTML"
file="resources\domain_reliability_internals.html" type="BINDATA"/>
On 2014/06/28 11:16:34, Bernhard Bauer wrote:
> Nit: More common style seems to be to add a space before the /.

Done.

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
File chrome/browser/resources/domain_reliability_internals.html (right):

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
chrome/browser/resources/domain_reliability_internals.html:8: <!-- <script
src="chrome://resources/js/load_time_data.js"></script> -->
On 2014/06/28 11:16:34, Bernhard Bauer wrote:
> Remove this?

Done.

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
File chrome/browser/resources/domain_reliability_internals.js (right):

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
chrome/browser/resources/domain_reliability_internals.js:5:
cr.define('domain_reliability_internals', function() {
On 2014/06/28 11:16:34, Bernhard Bauer wrote:
> You're using this roughly as a class with static methods, right? In that case,
> the name should be in CamelCase.

Done.

https://codereview.chromium.org/357103002/diff/1/chrome/browser/resources/dom...
chrome/browser/resources/domain_reliability_internals.js:13:
//alert(JSON.stringify(data));
On 2014/06/28 11:16:34, Bernhard Bauer wrote:
> Remove.

Done.

https://codereview.chromium.org/357103002/diff/1/chrome/browser/ui/webui/doma...
File chrome/browser/ui/webui/domain_reliability_internals_ui.cc (right):

https://codereview.chromium.org/357103002/diff/1/chrome/browser/ui/webui/doma...
chrome/browser/ui/webui/domain_reliability_internals_ui.cc:53: NOTREACHED();
On 2014/06/28 11:16:35, Bernhard Bauer wrote:
> If this can't happen, you don't need to deal with it in production code.
DCHECK
> service instead.

This actually can happen (I've changed the underlying model so the Service is
only created for Profiles that should have a Monitor), so I'm handling it
gracefully.

https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
File components/domain_reliability/service.cc (right):

https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
components/domain_reliability/service.cc:75:
network_task_runner_->PostTaskAndReply(FROM_HERE,
On 2014/06/28 11:16:35, Bernhard Bauer wrote:
> I think you can do this with base::PostTaskAndReplyWithResult(). And you can
> directly pass a scoped_ptr to a callback with base::Passed().

Nah, I can't; it won't let me bind a weak pointer to GetWebUIData, since it has
a non-void return type, so I'd still need at least one wrapper function.

[Bernhard Bauer, 2014-06-30 17:26:54]

https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
File components/domain_reliability/service.cc (right):

https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
components/domain_reliability/service.cc:75:
network_task_runner_->PostTaskAndReply(FROM_HERE,
On 2014/06/30 16:58:21, ttuttle wrote:
> On 2014/06/28 11:16:35, Bernhard Bauer wrote:
> > I think you can do this with base::PostTaskAndReplyWithResult(). And you can
> > directly pass a scoped_ptr to a callback with base::Passed().
> 
> Nah, I can't; it won't let me bind a weak pointer to GetWebUIData, since it
has
> a non-void return type, so I'd still need at least one wrapper function.

:-( Could you write a wrapper function like this:

  scoped_ptr<Value> GetWebUIData(WeakPtr<...> monitor) {
    if (!monitor)
      return NULL;

    return monitor->GetWebUIData();
  }

? That would at least get rid of the second wrapper function, and I think it
reads a bit more intuitively.

[Deprecated (see juliatuttle), 2014-07-01 20:10:42]

On 2014/06/30 17:26:54, Bernhard Bauer wrote:
>
https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
> File components/domain_reliability/service.cc (right):
> 
>
https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
> components/domain_reliability/service.cc:75:
> network_task_runner_->PostTaskAndReply(FROM_HERE,
> On 2014/06/30 16:58:21, ttuttle wrote:
> > On 2014/06/28 11:16:35, Bernhard Bauer wrote:
> > > I think you can do this with base::PostTaskAndReplyWithResult(). And you
can
> > > directly pass a scoped_ptr to a callback with base::Passed().
> > 
> > Nah, I can't; it won't let me bind a weak pointer to GetWebUIData, since it
> has
> > a non-void return type, so I'd still need at least one wrapper function.
> 
> :-( Could you write a wrapper function like this:
> 
>   scoped_ptr<Value> GetWebUIData(WeakPtr<...> monitor) {
>     if (!monitor)
>       return NULL;
> 
>     return monitor->GetWebUIData();
>   }
> 
> ? That would at least get rid of the second wrapper function, and I think it
> reads a bit more intuitively.

The problem with that is that then it tries to pass a const scoped_ptr<Value>&
to the callback instead of a scoped_ptr<const Value> or a const Value&.  (And it
doesn't work with Value instead of scoped_ptr<Value>, since it can't create a
new Value to initialize *result.)

[Deprecated (see juliatuttle), 2014-07-01 20:10:42]

On 2014/06/30 17:26:54, Bernhard Bauer wrote:
>
https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
> File components/domain_reliability/service.cc (right):
> 
>
https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
> components/domain_reliability/service.cc:75:
> network_task_runner_->PostTaskAndReply(FROM_HERE,
> On 2014/06/30 16:58:21, ttuttle wrote:
> > On 2014/06/28 11:16:35, Bernhard Bauer wrote:
> > > I think you can do this with base::PostTaskAndReplyWithResult(). And you
can
> > > directly pass a scoped_ptr to a callback with base::Passed().
> > 
> > Nah, I can't; it won't let me bind a weak pointer to GetWebUIData, since it
> has
> > a non-void return type, so I'd still need at least one wrapper function.
> 
> :-( Could you write a wrapper function like this:
> 
>   scoped_ptr<Value> GetWebUIData(WeakPtr<...> monitor) {
>     if (!monitor)
>       return NULL;
> 
>     return monitor->GetWebUIData();
>   }
> 
> ? That would at least get rid of the second wrapper function, and I think it
> reads a bit more intuitively.

The problem with that is that then it tries to pass a const scoped_ptr<Value>&
to the callback instead of a scoped_ptr<const Value> or a const Value&.  (And it
doesn't work with Value instead of scoped_ptr<Value>, since it can't create a
new Value to initialize *result.)

[Deprecated (see juliatuttle), 2014-07-01 20:35:32]

PTAL.

[Bernhard Bauer, 2014-07-02 10:17:55]

On 2014/07/01 20:10:42, ttuttle wrote:
> On 2014/06/30 17:26:54, Bernhard Bauer wrote:
> >
>
https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
> > File components/domain_reliability/service.cc (right):
> > 
> >
>
https://codereview.chromium.org/357103002/diff/1/components/domain_reliabilit...
> > components/domain_reliability/service.cc:75:
> > network_task_runner_->PostTaskAndReply(FROM_HERE,
> > On 2014/06/30 16:58:21, ttuttle wrote:
> > > On 2014/06/28 11:16:35, Bernhard Bauer wrote:
> > > > I think you can do this with base::PostTaskAndReplyWithResult(). And you
> can
> > > > directly pass a scoped_ptr to a callback with base::Passed().
> > > 
> > > Nah, I can't; it won't let me bind a weak pointer to GetWebUIData, since
it
> > has
> > > a non-void return type, so I'd still need at least one wrapper function.
> > 
> > :-( Could you write a wrapper function like this:
> > 
> >   scoped_ptr<Value> GetWebUIData(WeakPtr<...> monitor) {
> >     if (!monitor)
> >       return NULL;
> > 
> >     return monitor->GetWebUIData();
> >   }
> > 
> > ? That would at least get rid of the second wrapper function, and I think it
> > reads a bit more intuitively.
> 
> The problem with that is that then it tries to pass a const scoped_ptr<Value>&
> to the callback instead of a scoped_ptr<const Value> or a const Value&.  (And
it
> doesn't work with Value instead of scoped_ptr<Value>, since it can't create a
> new Value to initialize *result.)

Really? This works for me: https://codereview.chromium.org/369473004/

[Deprecated (see juliatuttle), 2014-07-07 16:53:33]

Huh, you're right. I figured scoped_ptr wouldn't be that smart.

PTAL?

[Bernhard Bauer, 2014-07-07 17:09:47]

LGTM

On 2014/07/07 16:53:33, ttuttle wrote:
> Huh, you're right. I figured scoped_ptr wouldn't be that smart.
> 
> PTAL?

scoped_ptr -- Surprisingly Smart! (tm)

[Deprecated (see juliatuttle), 2014-07-07 17:14:07]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-07-07 17:15:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ttuttle@chromium.org/357103002/60001

[commit-bot: I haz the power, 2014-07-07 17:29:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-07 17:31:53]

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

[commit-bot: I haz the power, 2014-07-07 17:31:55]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[sky, 2014-07-09 19:22:21]

.grd LGTM

[Deprecated (see juliatuttle), 2014-07-09 20:26:41]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-07-09 20:27:34]

The CQ bit was unchecked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-07-09 20:27:34]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-07-09 20:27:34]

The CQ bit was unchecked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2014-07-09 20:27:36]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-07-09 20:30:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ttuttle@chromium.org/357103002/100001

[commit-bot: I haz the power, 2014-07-10 00:57:13]

Change committed as 282202