Display status message for "Add snippets" on chrome://snippets-internals

Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid input, etc (604740). Furthermore, I enlarged the hosts input (605520).

BUG=604740, 605520
Committed: https://crrev.com/ab8e57c751b135139f3666ad900f491e78744fcd
Cr-Commit-Position: refs/heads/master@{#389431}

[jkrcal, 2016-04-21 13:14:27]

Description was changed from

==========
Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid
input, etc.

BUG=604740
==========

to

==========
Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid
input, etc.

BUG=604740, 605520
==========

[jkrcal, 2016-04-21 13:15:31]

Description was changed from

==========
Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid
input, etc.

BUG=604740, 605520
==========

to

==========
Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid input, etc
(604740). Furthermore, I enlarged the hosts input (605520).

BUG=604740, 605520
==========

[jkrcal, 2016-04-21 13:15:49]

jkrcal@chromium.org changed reviewers:
+ treib@chromium.org

[jkrcal, 2016-04-21 13:15:49]

PTAL

[jkrcal, 2016-04-21 13:17:04]

On 2016/04/21 13:15:49, jkrcal wrote:
> PTAL

Let me be more specific: PTAL at components/ntp_snippets/*

[jkrcal, 2016-04-21 13:19:43]

jkrcal@chromium.org changed reviewers:
+ bauerb@chromium.org

[jkrcal, 2016-04-21 13:19:44]

bauerb: PTAL at the three snippets-internals files.

[Marc Treib, 2016-04-21 13:47:21]

https://codereview.chromium.org/1910633005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:166: if
(!ntp_snippets_service_->get_last_status().empty())
nit: Don't call last_status() twice, call it once and keep the result.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:114: std::string response;
nit: Move the definition down to where it's actually used.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:25: base::Callback<void(const
std::string&, const std::string&)>;
I'd add comments ("fake names") for the two string params now.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:322:
snippets_fetcher_last_status_ = status;
Now we also get here if the download failed (and so snippets_json is empty),
right? In that case, there's no point in calling the parser, just call
LoadingSnippetsFinished instead.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:333: bool success =
LoadFromValue(*parsed);
No need for the "success" variable, just say if(LoadFromValue(..))

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:346: LOG(WARNING) << "Received
invalid JSON (" << error << "): " << snippets_json;
Set snippets_fetcher_last_status_ to something meaningful here.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:83: const std::string
get_last_status() const {
Just "last_status", no "get".
Also, return a reference.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:200: std::string
snippets_fetcher_last_status_;
IMO this name is a bit misleading, ad it doesn't always contain a status from
the fetcher. Maybe "last_fetch_status_"?

[Bernhard Bauer, 2016-04-21 16:28:41]

https://codereview.chromium.org/1910633005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:125:
SendString("hosts-status", "");
Nit: Use an empty std::string() constructor instead of a "" literal to save a
handful of instructions.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:25: base::Callback<void(const
std::string&, const std::string&)>;
On 2016/04/21 13:47:20, Marc Treib wrote:
> I'd add comments ("fake names") for the two string params now.

They don't even have to be in comments -- the signature can contain parameter
names.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:228:
snippets_fetcher_last_status_ = "Cannot fetch for empty hosts list.";
Can you extract these into constants and move to the top of the file?

[jkrcal, 2016-04-22 09:30:28]

Thanks for the reviews!

PTAL once again.

https://codereview.chromium.org/1910633005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:125:
SendString("hosts-status", "");
On 2016/04/21 16:28:40, Bernhard Bauer wrote:
> Nit: Use an empty std::string() constructor instead of a "" literal to save a
> handful of instructions.

Done.

https://codereview.chromium.org/1910633005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:166: if
(!ntp_snippets_service_->get_last_status().empty())
On 2016/04/21 13:47:20, Marc Treib wrote:
> nit: Don't call last_status() twice, call it once and keep the result.

Done.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:114: std::string response;
On 2016/04/21 13:47:20, Marc Treib wrote:
> nit: Move the definition down to where it's actually used.

Done.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:25: base::Callback<void(const
std::string&, const std::string&)>;
On 2016/04/21 16:28:40, Bernhard Bauer wrote:
> On 2016/04/21 13:47:20, Marc Treib wrote:
> > I'd add comments ("fake names") for the two string params now.
> 
> They don't even have to be in comments -- the signature can contain parameter
> names.

Done.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:228:
snippets_fetcher_last_status_ = "Cannot fetch for empty hosts list.";
On 2016/04/21 16:28:41, Bernhard Bauer wrote:
> Can you extract these into constants and move to the top of the file?

Done.

What is the reason (apart from having English messages in one place)? Is it okay
to leave LOG and DLOG messages as literals? If so, why?

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:322:
snippets_fetcher_last_status_ = status;
On 2016/04/21 13:47:20, Marc Treib wrote:
> Now we also get here if the download failed (and so snippets_json is empty),
> right? In that case, there's no point in calling the parser, just call
> LoadingSnippetsFinished instead.

Done.

Good point, thanks! This is something I've totally overlooked.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:333: bool success =
LoadFromValue(*parsed);
On 2016/04/21 13:47:20, Marc Treib wrote:
> No need for the "success" variable, just say if(LoadFromValue(..))

Done.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:346: LOG(WARNING) << "Received
invalid JSON (" << error << "): " << snippets_json;
On 2016/04/21 13:47:20, Marc Treib wrote:
> Set snippets_fetcher_last_status_ to something meaningful here.

Done.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:83: const std::string
get_last_status() const {
On 2016/04/21 13:47:21, Marc Treib wrote:
> Just "last_status", no "get".
> Also, return a reference.

Done.

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:200: std::string
snippets_fetcher_last_status_;
On 2016/04/21 13:47:21, Marc Treib wrote:
> IMO this name is a bit misleading, ad it doesn't always contain a status from
> the fetcher. Maybe "last_fetch_status_"?

Done.

[Bernhard Bauer, 2016-04-22 09:42:17]

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:228:
snippets_fetcher_last_status_ = "Cannot fetch for empty hosts list.";
On 2016/04/22 09:30:28, jkrcal wrote:
> On 2016/04/21 16:28:41, Bernhard Bauer wrote:
> > Can you extract these into constants and move to the top of the file?
> 
> Done.
> 
> What is the reason (apart from having English messages in one place)? Is it
okay
> to leave LOG and DLOG messages as literals? If so, why?

It's mostly that the status messages are part of the observable interface of the
class, whereas log messages are only for developers. Even now it feels a bit
strange to me to expose English messages as opposed to e.g. status codes, but if
it's just meant to be shown in the debugging page, I guess that's okay.

https://codereview.chromium.org/1910633005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1910633005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:167: if
(!status.empty()) SendString("hosts-status", "Finished: " + status);
Put the body of the if-statement onto a new line.

https://codereview.chromium.org/1910633005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1910633005/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:117: message = "URLRequestStatus
error " + base::IntToString(status.error());
Can we do the same thing here? Thanks!

https://codereview.chromium.org/1910633005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/1910633005/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:24: // If problems occur
(explained in status_message), snippets_json is empty.
Put variable names in comments between |pipe| symbols.

[Marc Treib, 2016-04-22 09:44:44]

LGTM with Bernhard's remaining comments addressed.

[jkrcal, 2016-04-22 10:56:09]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-22 10:56:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1910633005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1910633005/100001

[jkrcal, 2016-04-22 11:11:59]

PTAL

(I also had to do a rebase-update before the last changes)

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:228:
snippets_fetcher_last_status_ = "Cannot fetch for empty hosts list.";
On 2016/04/22 09:42:17, Bernhard Bauer wrote:
> On 2016/04/22 09:30:28, jkrcal wrote:
> > On 2016/04/21 16:28:41, Bernhard Bauer wrote:
> > > Can you extract these into constants and move to the top of the file?
> > 
> > Done.
> > 
> > What is the reason (apart from having English messages in one place)? Is it
> okay
> > to leave LOG and DLOG messages as literals? If so, why?
> 
> It's mostly that the status messages are part of the observable interface of
the
> class, whereas log messages are only for developers. Even now it feels a bit
> strange to me to expose English messages as opposed to e.g. status codes, but
if
> it's just meant to be shown in the debugging page, I guess that's okay.

Thanks for explaining!

https://codereview.chromium.org/1910633005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/1910633005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:167: if
(!status.empty()) SendString("hosts-status", "Finished: " + status);
On 2016/04/22 09:42:17, Bernhard Bauer wrote:
> Put the body of the if-statement onto a new line.

Done.

https://codereview.chromium.org/1910633005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1910633005/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:117: message = "URLRequestStatus
error " + base::IntToString(status.error());
On 2016/04/22 09:42:17, Bernhard Bauer wrote:
> Can we do the same thing here? Thanks!

Done.

https://codereview.chromium.org/1910633005/diff/80001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/1910633005/diff/80001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:24: // If problems occur
(explained in status_message), snippets_json is empty.
On 2016/04/22 09:42:17, Bernhard Bauer wrote:
> Put variable names in comments between |pipe| symbols.

Done.

[Marc Treib, 2016-04-22 12:02:54]

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:33: } // namespace
nitty nit: two spaces before //

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:128: source->GetResponseCode());
nit: Braces now please, since it's not a single line anymore.

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:415: last_fetch_status_ = "OK";
kStatusMessageOK

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:416: LoadingSnippetsFinished();
Is this required again now? Previously you had removed the line.

[jkrcal, 2016-04-22 12:20:09]

Thanks, Marc!

PTAL, hopefully for the last time :)

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:33: } // namespace
On 2016/04/22 12:02:54, Marc Treib wrote:
> nitty nit: two spaces before //

Done.

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:128: source->GetResponseCode());
On 2016/04/22 12:02:54, Marc Treib wrote:
> nit: Braces now please, since it's not a single line anymore.

Done.

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:415: last_fetch_status_ = "OK";
On 2016/04/22 12:02:54, Marc Treib wrote:
> kStatusMessageOK

Removed the line. 
 (A mistake by manual merging. Thanks!)

https://codereview.chromium.org/1910633005/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:416: LoadingSnippetsFinished();
On 2016/04/22 12:02:54, Marc Treib wrote:
> Is this required again now? Previously you had removed the line.

Argh, you are right, removed!
 (A mistake by manual merging. Thanks!)

[Marc Treib, 2016-04-22 12:28:31]

LGTM again :)

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:338: last_fetch_status_ =
status;
nit: Move this into the else branch? Otherwise it'll be overwritten anyway once
json parsing completes.

[Bernhard Bauer, 2016-04-22 12:54:00]

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:33: }  // namespace
If you use an anonymous namespace here, include the constants below as well.

[jkrcal, 2016-04-22 13:04:37]

Implemented both comments.

PTAL

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.cc:33: }  // namespace
On 2016/04/22 12:54:00, Bernhard Bauer wrote:
> If you use an anonymous namespace here, include the constants below as well.

Done.

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:338: last_fetch_status_ =
status;
On 2016/04/22 12:28:30, Marc Treib wrote:
> nit: Move this into the else branch? Otherwise it'll be overwritten anyway
once
> json parsing completes.

Done.

Nothing gets overwritten (in the if branch, status is always empty). Anyway, I
agree that it is clearer, now.

[Bernhard Bauer, 2016-04-22 14:28:44]

LGTM w/ a nit:

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1910633005/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:338: last_fetch_status_ =
status;
On 2016/04/22 13:04:37, jkrcal wrote:
> On 2016/04/22 12:28:30, Marc Treib wrote:
> > nit: Move this into the else branch? Otherwise it'll be overwritten anyway
> once
> > json parsing completes.
> 
> Done.
> 
> Nothing gets overwritten (in the if branch, status is always empty). 

DCHECK that maybe? 🙂

[jkrcal, 2016-04-25 07:39:33]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-04-25 07:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1910633005/#ps160001
(title: "The final version ;)")

[commit-bot: I haz the power, 2016-04-25 07:39:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1910633005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1910633005/160001

[commit-bot: I haz the power, 2016-04-25 08:17:51]

Description was changed from

==========
Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid input, etc
(604740). Furthermore, I enlarged the hosts input (605520).

BUG=604740, 605520
==========

to

==========
Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid input, etc
(604740). Furthermore, I enlarged the hosts input (605520).

BUG=604740, 605520
==========

[commit-bot: I haz the power, 2016-04-25 08:17:52]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-25 08:20:25]

Description was changed from

==========
Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid input, etc
(604740). Furthermore, I enlarged the hosts input (605520).

BUG=604740, 605520
==========

to

==========
Display status message for "Add snippets" on chrome://snippets-internals

The status message informs of URLRequest / HTTP error codes, invalid input, etc
(604740). Furthermore, I enlarged the hosts input (605520).

BUG=604740, 605520

Committed: https://crrev.com/ab8e57c751b135139f3666ad900f491e78744fcd
Cr-Commit-Position: refs/heads/master@{#389431}
==========

[commit-bot: I haz the power, 2016-04-25 08:20:26]

Patchset 9 (id:??) landed as
https://crrev.com/ab8e57c751b135139f3666ad900f491e78744fcd
Cr-Commit-Position: refs/heads/master@{#389431}